Bug 678442 - Review Request: os-prober - Probes disks on the system for installed operating systems
Review Request: os-prober - Probes disks on the system for installed operatin...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jerry James
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 678456
  Show dependency treegraph
 
Reported: 2011-02-17 17:48 EST by Hedayat Vatankhah
Modified: 2011-05-09 16:54 EDT (History)
5 users (show)

See Also:
Fixed In Version: os-prober-1.46-2.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-05-09 00:04:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
loganjerry: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hedayat Vatankhah 2011-02-17 17:48:25 EST
Spec URL: http://hedayat.fedorapeople.org/reviews/os-prober/os-prober.spec
SRPM URL: http://hedayat.fedorapeople.org/reviews/os-prober/os-prober-1.42-1.fc14.src.rpm
Description: 
This package detects other OSes available on a system and outputs the results
in a generic machine-readable format. Support for new OSes and Linux
distributions can be added easily. 
This package is needed by grub2 to detect other operating systems installed on the system.
Comment 1 Hedayat Vatankhah 2011-02-17 17:50:08 EST
Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2847785
Comment 2 Hedayat Vatankhah 2011-02-17 19:15:23 EST
Sorry, this is the correct Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2848074
Comment 3 Sergio Belkin 2011-02-21 13:28:42 EST
Hi Hedayat,

I am (still) not from packager group, however I hope you find my quick review useful:

Did you run rpmlint on rpm files?

1. Either you should add she-bang on top of /usr/share/os-prober/common.sh or better (in this case viewing that are functions) remove executable bits for that file: 

Consider that eg: /etc/init.d/functions has no executable bits and  http://fedoraproject.org/wiki/Common_Rpmlint_issues#script-without-shebang

2. Scripts lacks of man pages, that's not mandatory but perhaps you can talk and help with the author Joey Hess about it.

HTH

Greetings
Comment 4 Hedayat Vatankhah 2011-02-21 14:04:50 EST
Hi Sergio,
Thank you for reviewing the package. Yes, I've run rpmlint, and saw the issue. But I was not sure about the solution. It doesn't seem to need a shebang since it is usually used by others, on the other hand it's a script. I wonder which is a better solution: adding a shebang to the file or removing the executable bit?

About the man pages, I'm probably unable to help currently, but I'll contact him to see if he is interested.

Thanks again :)
Comment 5 Sergio Belkin 2011-02-21 15:12:33 EST
Hi again Hedayat,

If I were you I'd remove it the executable bits from script, AFAIS it's not an antonomous but a functions file. Does make sense run seperately /usr/share/os-prober/common.sh ? I think it doesn't.

Greets!
Comment 6 Hedayat Vatankhah 2011-02-21 17:22:49 EST
Yes, it doesn't. Thanks. :)
This is the updated files:
Spec: http://hedayat.fedorapeople.org/reviews/os-prober/1.42-2/os-prober.spec
SRPM: http://hedayat.fedorapeople.org/reviews/os-prober/1.42-2/os-prober-1.42-2.fc14.src.rpm
Comment 7 Mehdi Fattahi 2011-04-22 10:47:20 EDT
Trying to install the RPM I got the following error message:


mahdi@mahdi-TECRA-A8 Downloads$ sudo rpm -Uvh os-prober-1.42-2.fc14.src.rpm 
   1:os-prober              warning: user hedayat does not exist - using root
warning: group hedayat does not exist - using root
warning: user hedayat does not exist - using root)
warning: group hedayat does not exist - using root
Comment 8 Hedayat Vatankhah 2011-04-22 16:32:40 EDT
You should not install src.rpm package. Instead, build the src.rpm using rpmbuild command:
rpmbuild -bb os-prober-1.42-2.fc14.src.rpm

Then, it'll build appropriate RPM packages inside ~/rpmbuild/RPMS or /usr/src/redhat/RPMS (IIRC), and you can install generated rpm packages then.
Comment 10 Jerry James 2011-05-03 12:18:20 EDT
I'll take this review.
Comment 11 Jerry James 2011-05-03 13:27:06 EDT
Here are a few general comments before I get to the usual review items.  First, if this spec file will only be used for Fedora, then the following elements of the spec file are unnecessary:
- The BuildRoot tag
- rm -rf %{buildroot} at the top of the %install script
- The %clean script
- %defattr(-,root,root,-) at the top of %files

Second, there are some debian-isms in these script files.  For example, /usr/libexec/os-probes/init/10filesystems contains references to anna-install and /lib/debian-installer, and /usr/share/os-prober/common.sh contains references to mapdevfs.  Will the absence of these on Fedora lead to problems?

Third, from poking around in the scripts a little, I see uses of binaries in the following packages: coreutils, cryptsetup-luks, dmraid, grep, lvm2, module-init-tools, udev, and util-linux-ng.  None of them are Requires of this package.  Should they be?

Output from rpmlint:
os-prober.x86_64: W: no-manual-page-for-binary os-prober
os-prober.x86_64: W: no-manual-page-for-binary linux-boot-prober
os-prober-debuginfo.x86_64: E: empty-debuginfo-package
2 packages and 1 specfiles checked; 1 errors, 2 warnings.

The debuginfo package is empty because the newns binary is not in a location where rpmbuild expects an ELF object.  Is there any way that binary could be put directly in /usr/libexec; i.e., not in a subdirectory?

+: OK
-: must be fixed
=: needs attention
N: not applicable

MUST:
[-]: rpmlint output (posted above, shows the -debuginfo problem)
[+]: package name meets naming guidelines
[+]: spec file name matches base package name
[+]: package meets the packaging guidelines
[+]: package meets the licensing guidelines
[+]: license field matches the actual license
[N]: include the license text file in %doc, if one exists
[+]: spec file is written in American English
[+]: spec file is legible
[+]: sources match upstream: both have md5sum b49d98e33da4c2c2534fff6badc2013c
[+]: package builds into binary rpm on at least one primary arch (x86_64)
[N]: appropriate use of ExcludeArch
[+]: all build dependencies listed in BuildRequires
[+]: proper locale handling
[N]: invoke ldconfig if libraries are installed
[+]: no bundled copies of system libraries
[N]: no relocatable packages
[+]: package owns all directories that it creates
[+]: no duplicate entries in %files
[+]: proper permissions on files
[+]: consistent use of macros
[+]: code or permissible content
[N]: large documentation goes in a -doc subpackage
[+]: no runtime dependencies in %doc
[N]: header files in -devel
[N]: static libraries in -static
[N]: .so files in -devel
[N]: -devel requires the main package
[+]: no libtool archives
[N]: GUI applications install a .desktop file
[+]: package does not own files/dirs owned by other packages
[+]: UTF-8 filenames

SHOULD:
[=]: if not license text file, packager should query upstream
[N]: description and summary contain available translations
[+]: package builds in mock (tested with fedora-rawhide-i386)
[+]: package builds on all supported arches
[=]: package functions as described (minimal testing only, as I have only Fedora linux disks)
[+]: sane scriptlets
[N]: subpackages require the main package
[N]: placement of pkgconfig files
[+]: package dependencies instead of file dependencies
[=]: man pages for binaries: there are none
Comment 12 Hedayat Vatankhah 2011-05-03 20:41:37 EDT
Thanks for your through review. 

(In reply to comment #11)
> Here are a few general comments before I get to the usual review items.  First,
> if this spec file will only be used for Fedora, then the following elements of
> the spec file are unnecessary:
> - The BuildRoot tag
> - rm -rf %{buildroot} at the top of the %install script
> - The %clean script
> - %defattr(-,root,root,-) at the top of %files
Done.


> 
> Second, there are some debian-isms in these script files.  For example,
> /usr/libexec/os-probes/init/10filesystems contains references to anna-install
> and /lib/debian-installer, and /usr/share/os-prober/common.sh contains
> references to mapdevfs.  Will the absence of these on Fedora lead to problems?
No, those will be used only when this package is used inside debian installer. They'll be ignored otherwise.


> 
> Third, from poking around in the scripts a little, I see uses of binaries in
> the following packages: coreutils, cryptsetup-luks, dmraid, grep, lvm2,
> module-init-tools, udev, and util-linux-ng.  None of them are Requires of this
> package.  Should they be?
Yes, you are right. I've added a number of them as Requires, but leaving a few of them out since they don't look to be really useful, and will be silently ignored if not available.


> 
> Output from rpmlint:
> os-prober.x86_64: W: no-manual-page-for-binary os-prober
> os-prober.x86_64: W: no-manual-page-for-binary linux-boot-prober
> os-prober-debuginfo.x86_64: E: empty-debuginfo-package
> 2 packages and 1 specfiles checked; 1 errors, 2 warnings.
> 
> The debuginfo package is empty because the newns binary is not in a location
> where rpmbuild expects an ELF object.  Is there any way that binary could be
> put directly in /usr/libexec; i.e., not in a subdirectory?
Done.


> -: must be fixed
> =: needs attention
>
> MUST:
> [-]: rpmlint output (posted above, shows the -debuginfo problem)
Addressed above.

[...]
> SHOULD:
> [=]: if not license text file, packager should query upstream
> [=]: package functions as described (minimal testing only, as I have only
> Fedora linux disks)
> [=]: man pages for binaries: there are none
I'll contact upstream about both man page(s) and license text.

New files:
SPEC: http://hedayat.fedorapeople.org/reviews/os-prober/1.46-2/os-prober.spec
SRPM: http://hedayat.fedorapeople.org/reviews/os-prober/1.46-2/os-prober-1.46-2.fc15.src.rpm
Comment 13 Jerry James 2011-05-03 23:33:57 EDT
Excellent, the new package looks good.  I'll leave to your discretion whether to wait for upstream on the license and man page issues, as those are only SHOULD.  All of the MUST elements are now satisfied, so this package is approved.
Comment 14 Hedayat Vatankhah 2011-05-04 02:33:30 EDT
New Package SCM Request
=======================
Package Name: os-prober
Short Description: Detects OSes installed on the system with their corresponding boot description
Owners: hedayat
Branches: f14 f15
InitialCC:
Comment 15 Jason Tibbitts 2011-05-05 11:36:01 EDT
Git done (by process-git-requests).
Comment 16 Fedora Update System 2011-05-05 16:42:53 EDT
os-prober-1.46-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/os-prober-1.46-2.fc15
Comment 17 Fedora Update System 2011-05-05 16:42:59 EDT
os-prober-1.46-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/os-prober-1.46-2.fc14
Comment 18 Fedora Update System 2011-05-05 17:53:15 EDT
os-prober-1.46-2.fc15 has been pushed to the Fedora 15 testing repository.
Comment 19 Fedora Update System 2011-05-09 00:03:57 EDT
os-prober-1.46-2.fc15 has been pushed to the Fedora 15 stable repository.
Comment 20 Fedora Update System 2011-05-09 16:54:10 EDT
os-prober-1.46-2.fc14 has been pushed to the Fedora 14 stable repository.

Note You need to log in before you can comment on or make changes to this bug.