Bug 678442

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2847785

Sorry, this is the correct Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2848074

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

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 :)

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!

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

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

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.

Updated to the latest upstream version:
SPEC: http://hedayat.fedorapeople.org/reviews/os-prober/1.46-1/os-prober.spec
SRPM: http://hedayat.fedorapeople.org/reviews/os-prober/1.46-1/os-prober-1.46-1.fc14.src.rpm

Koji Build (Rawhide): http://koji.fedoraproject.org/koji/taskinfo?taskID=3036600
Koji Build (F14): http://koji.fedoraproject.org/koji/taskinfo?taskID=3036606

I'll take this review.

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

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

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.

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:

Git done (by process-git-requests).

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

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

os-prober-1.46-2.fc15 has been pushed to the Fedora 15 testing repository.

os-prober-1.46-2.fc15 has been pushed to the Fedora 15 stable repository.

os-prober-1.46-2.fc14 has been pushed to the Fedora 14 stable repository.