[Bug 226456] Merge Review: system-config-display

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: system-config-display


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226456


kevin at tummy.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody at fedoraproject.org    |kevin at tummy.com
               Flag|                            |fedora-review?




------- Additional Comments From kevin at tummy.com  2007-03-20 23:59 EST -------

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:
OK - Package needs ExcludeArch
 - 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 has rm -rf RPM_BUILD_ROOT at top of %install
OK - 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.
OK - Package doesn't own any directories other packages own.
OK - 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 have dist tag
OK - Should package latest version
73 bugs outstanding - check for outstanding bugs on package.

Issues:

1. The URL here seems to point to a 404. Is there some better page
to point the URL to?

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

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

4. Why is this preun needed?
%preun
if [ -d /usr/share/system-config-display ] ; then
  rm -rf /usr/share/system-config-display/*.pyc
fi

5. Some of the translation files say:
po/lt.po:# This file is distributed under the same license as the PACKAGE package.
Would be nice to say "system-config-display" there instead of PACKAGE?

6. Should add a
rm -rf $RPM_BUILD_ROOT
to the top of the %install section.

7. 73 outstanding bugs. Perhaps it would be good to do some triage on those
and see if anything can easily be addressed as part of this review cleanup?
Or, given the number perhaps we could get a group together to assist you at
some point?

Particular to packaging:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=216471
and
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212911
should be fixed by removing the preun (point 4)

8. rpmlint says:

a)
E: system-config-display obsolete-not-provided redhat-config-xfree86
E: system-config-display obsolete-not-provided Xconfigurator
W: system-config-display unversioned-explicit-obsoletes redhat-config-xfree86
W: system-config-display unversioned-explicit-obsoletes Xconfigurator

Are these old Obsoletes needed anymore?

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

Should those be noreplace?

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

Ignore

d)
E: system-config-display script-without-shebang
/usr/share/system-config-display/xConfigDialog.py
E: system-config-display script-without-shebang
/usr/share/system-config-display/display.glade
E: system-config-display script-without-shebang
/usr/share/system-config-display/screenSizePreview.py
E: system-config-display script-without-shebang
/usr/share/system-config-display/monitorDialog.py
E: system-config-display script-without-shebang
/usr/share/system-config-display/videocardDialog.py

All those should be mode 644?

e)
W: system-config-display dangerous-command-in-%preun rm

Remove the preun?

f)
W: system-config-display prereq-use gtk2 >= 2.6

Is that prereq needed? Can it be a Requires instead?

g)
W: system-config-display prereq-use hicolor-icon-theme

Should this be "Requires(postun)" and "Requires(post)"

h)
E: system-config-display no-cleaning-of-buildroot %install 

See point 6.

9. Is the "Requires: metacity" really needed? why?


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.