Bug 226453 - Merge Review: system-config-boot
Merge Review: system-config-boot
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:04 EST by Nobody's working on this, feel free to take it
Modified: 2009-09-21 16:30 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-05 15:18:24 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:04:35 EST
Fedora Merge Review: system-config-boot

http://cvs.fedora.redhat.com/viewcvs/devel/system-config-boot/
Initial Owner: harald@redhat.com
Comment 1 Kevin Fenzi 2007-03-14 01:16:48 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
See below - Package needs ExcludeArch
OK - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

See below - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
See below - Package doesn't own any directories other packages own.
See below- Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have dist tag
OK - Should package latest version
2 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Minor: might include a copy of the GPL.

2. Since redhat/fedora is upstream for this, can you make a note in the spec
as suggested in:
http://www.fedoraproject.org/wiki/Packaging/SourceURL#head-413e1c297803cfa9de0cc4c56f3ac384bff5dc9e

3. I assume the reason it only builds on ix86/x86_64 is that it only understands
lilo/grub? Might be worth filing a bug and noting it in the spec and see if
some ppc folk are interested in contributing yaboot support.

4. Please use one of the preferred buildroots, such as:
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

5. Do not use %makeinstall. See:
http://www.fedoraproject.org/wiki/PackagingDrafts/MakeInstall

6. The desktop file is missing a valid Main Category, see:
http://standards.freedesktop.org/menu-spec/latest/apa.html
Suggest: System or Settings be added.
Without this, this tool shows up under a "Other" menu in Xfce.

7. The buildrequires are not all needed, suggest changing:
BuildRequires: python >= 0:2.2, perl, gettext, glibc-devel, gcc,
desktop-file-utils, yelp, perl-XML-Parser

to

BuildRequires: python, gettext, desktop-file-utils, yelp, perl-XML-Parser

8. Shouldn't the firstboot package own
%dir /usr/share/firstboot/
%dir /usr/share/firstboot/modules
and not this package?

9. 2 outstanding bugs. Might look if either can be resolved easily.

10. rpmlint says:

a)
E: system-config-boot no-binary

I assume this is not noarch since it can be only run on ix86/x86_64?

b)
W: system-config-boot conffile-without-noreplace-flag /etc/pam.d/system-config-boot
W: system-config-boot conffile-without-noreplace-flag
/etc/security/console.apps/system-config-boot

Suggest: should those be (noreplace)?

c)
W: system-config-boot no-documentation

No docs available?

d)
E: system-config-boot non-executable-script
/usr/share/firstboot/modules/boot_gui.py 0644
E: system-config-boot non-executable-script
/usr/share/system-config-boot/system-config-boot.py 0644
Suggest: remove the #!/usr/bin/python from those.

e)
W: system-config-boot unversioned-explicit-obsoletes redhat-config-boot

Suggest: add a version here? or just remove it at this point?

f)
W: system-config-boot rpm-buildroot-usage %prep rm -rf $RPM_BUILD_ROOT
E: system-config-boot no-cleaning-of-buildroot %install

Suggest: Move the rm from prep to the top of install?

g)
W: system-config-boot macro-in-%changelog dist
W: system-config-boot macro-in-%changelog dist

Suggest: Change occurances of %dist in the changelog with %%dist

h)
E: system-config-boot-debuginfo empty-debuginfo-package

I guess you need to add
 %define debug_package %{nil}
if this really has to be an arch package.
Comment 2 Harald Hoyer 2007-03-23 08:36:09 EDT
> 1. Minor: might include a copy of the GPL.
done

> 2. Since redhat/fedora is upstream for this, can you make a note in the spec
> as suggested in:
>
http://www.fedoraproject.org/wiki/Packaging/SourceURL#head-413e1c297803cfa9de0cc4c56f3ac384bff5dc9e

done

> 3. I assume the reason it only builds on ix86/x86_64 is that it only understands
> lilo/grub? Might be worth filing a bug and noting it in the spec and see if
> some ppc folk are interested in contributing yaboot support.

what about sparc, s390 and the others?

> 4. Please use one of the preferred buildroots, such as:
>  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

done

> 5. Do not use %makeinstall. See:
> http://www.fedoraproject.org/wiki/PackagingDrafts/MakeInstall

done

> 6. The desktop file is missing a valid Main Category, see:
> http://standards.freedesktop.org/menu-spec/latest/apa.html
> Suggest: System or Settings be added.
> Without this, this tool shows up under a "Other" menu in Xfce.

Categories=System;Application;SystemSetup;X-Red-Hat-Base;

> 7. The buildrequires are not all needed, suggest changing:
> BuildRequires: python >= 0:2.2, perl, gettext, glibc-devel, gcc,
> desktop-file-utils, yelp, perl-XML-Parser

done

> 8. Shouldn't the firstboot package own
> %dir /usr/share/firstboot/
> %dir /usr/share/firstboot/modules
> and not this package?

done

> 9. 2 outstanding bugs. Might look if either can be resolved easily.

bug #134548 and bug #181749 are not easily fixable

> 10. rpmlint says:
>
> a) E: system-config-boot no-binary
> I assume this is not noarch since it can be only run on ix86/x86_64?

yep

> b)
> W: system-config-boot conffile-without-noreplace-flag 
> /etc/pam.d/system-config-boot
> W: system-config-boot conffile-without-noreplace-flag
> /etc/security/console.apps/system-config-boot
> Suggest: should those be (noreplace)?

done

> c) W: system-config-boot no-documentation 
> No docs available?

no .)

> d)
> Suggest: remove the #!/usr/bin/python from those.

done

> e) W: system-config-boot unversioned-explicit-obsoletes redhat-config-boot
> Suggest: add a version here? or just remove it at this point?

added version

> f)
> W: system-config-boot rpm-buildroot-usage %prep rm -rf $RPM_BUILD_ROOT
> E: system-config-boot no-cleaning-of-buildroot %install
> Suggest: Move the rm from prep to the top of install?

done

> g)
> W: system-config-boot macro-in-%changelog dist
> W: system-config-boot macro-in-%changelog dist
> 
> Suggest: Change occurances of %dist in the changelog with %%dist

done

> h) E: system-config-boot-debuginfo empty-debuginfo-package
> I guess you need to add
>  %define debug_package %{nil}
> if this really has to be an arch package.

done

please check system-config-boot-0.2.15-1.fc7
Comment 3 Kevin Fenzi 2007-03-23 23:12:31 EDT
>> 3. I assume the reason it only builds on ix86/x86_64 is that it only understands
>> lilo/grub? Might be worth filing a bug and noting it in the spec and see if
>> some ppc folk are interested in contributing yaboot support.
>
>what about sparc, s390 and the others?

Sure. I suppose there could be one bug about "non x86/x86_64" support?

>> 6. The desktop file is missing a valid Main Category, see:
>> http://standards.freedesktop.org/menu-spec/latest/apa.html
>> Suggest: System or Settings be added.
>> Without this, this tool shows up under a "Other" menu in Xfce.
>
>Categories=System;Application;SystemSetup;X-Red-Hat-Base;

Humm...my mistake. I thought it didn't have "System" in there.

>> g)
>> W: system-config-boot macro-in-%changelog dist
>> W: system-config-boot macro-in-%changelog dist
>>
>> Suggest: Change occurances of %dist in the changelog with %%dist
>
>done

I still see dist in the changelog. Looking further at it, I think
they shouldn't be there at all. You don't need to have them in the
version string per release.

Thanks for the quick fixes!
Comment 4 Kevin Fenzi 2007-05-29 23:07:10 EDT
Sorry for the delay in looking back at this. 

About the point #3, we are supposed to file a bug on excluding other primary
arches. This allows folks interested in those arches to see items they might
want to work on fixing. So, I think it might be good to have a ppc bug filed
blocking the ppcexcludearch blocker, so ppc hackers might add yaboot support.
Other platforms aren't primary arches, so I don't think we need to worry about
them right now. 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=FE-ExcludeArch-ppc

I guess removing the %{?dist} from the changelog is cosmetic, but if you get a
chance it should be easy to do. 

Provided you file that bug and fix the dist the next time you need to push a
update I will go ahead and approve this now. Feel free to close this rawhide
once those things are done.  
Comment 5 Kevin Fenzi 2007-08-05 15:18:24 EDT
This can be closed. 

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