Bug 226456

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?

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

Yes, fixed.
 
> 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

Done.

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

Fixed.

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

It isn't, afaict.  Nuked.

> 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?

Fixed.

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

Done.

> 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?

I _really_ want to not own this package.

The vast majority of the bugs against s-c-d are about dualhead sucking.  There's
really no good way to fix that short of RANDR 1.2 support in Xorg, so I've been
trying to focus on that first.

I certainly wouldn't object if people wanted to help, but thus far no one has.

> 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?

Xconfigurator was never in Fedora, and r-c-x was only in FC1, so I'm fine with
dropping these.

> 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?

Maybe?  I have no idea what noreplace semantics are or why I'd want them.
 
> 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?

Oh probably, but since they're 0644 in the upstream source, something must be
making them executable.  No harm though, just added #! lines upstream too.

> e)
> W: system-config-display dangerous-command-in-%preun rm
> 
> Remove the preun?

Yep, done.

> f)
> W: system-config-display prereq-use gtk2 >= 2.6
> 
> Is that prereq needed? Can it be a Requires instead?

I think so, yeah, changed.

> g)
> W: system-config-display prereq-use hicolor-icon-theme
> 
> Should this be "Requires(postun)" and "Requires(post)"

I think it's just Requires:, since h-i-t should exist to own the directories,
but if it doesn't, then when we remove the package we should check if the icon
dir exists before trying to update the icon cache.

Changed to Requires and added the check, but I'm still not sure about this.

> h)
> E: system-config-display no-cleaning-of-buildroot %install 
> 
> See point 6.

Fixed.

> 9. Is the "Requires: metacity" really needed? why?
 
Yes.  It needs a window manager to manage dialog placement when run from the
command line (and therefore when starting its own X server), and currently it
hardcodes metacity as the name of that wm.  Until that changes, the Requires is
needed.

(In reply to comment #2)
> > 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?
> 
> Maybe?  I have no idea what noreplace semantics are or why I'd want them.
>

Files marked as config are usually intended to be edited by the user to
customize the application. In that case, you mark them as %config, so rpm does
not nuke them during an update, but copies them to a %filename.rpmsave backup
file before creating the new file.

The %config(noreplace) variant tells rpm to give priority to the user modified
config file so it's the new updated config file which is created as %filename.rpmnew

>I _really_ want to not own this package.

:)
Perhaps it's worth mailing the maintainers list and asking if anyone is interested?

>The vast majority of the bugs against s-c-d are about dualhead sucking.  There's
>really no good way to fix that short of RANDR 1.2 support in Xorg, so I've been
>trying to focus on that first.

Yeah. Might be worth making a "Dual head support is bad now until xrandr 1.2"
bug and closing a bunch of them as dups against that one?

(I'm really looking forward to xrandr 1.2. ;)

> I certainly wouldn't object if people wanted to help, but thus far no one has.

Yeah. ;(

>> All those should be mode 644?
>
>Oh probably, but since they're 0644 in the upstream source, something must be
>making them executable.  No harm though, just added #! lines upstream too.

Well, are they scripts that can be run by themselves?
If not, it would make more sense to not have the #! and make them mode 644.

>> g)
>> W: system-config-display prereq-use hicolor-icon-theme
>>
>> Should this be "Requires(postun)" and "Requires(post)"
>
>I think it's just Requires:, since h-i-t should exist to own the directories,
>but if it doesn't, then when we remove the package we should check if the icon
>dir exists before trying to update the icon cache.
>
>Changed to Requires and added the check, but I'm still not sure about this.

Looks like it shouldn't be anything at all...

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

says: "Note that no dependencies should be added for this. 
If gtk-update-icon-cache is not available, there's nothing that would be needing
the cache update. Not adding the dependency on gtk-update-icon-cache (ie. gtk2
>= 2.6.0) makes it easier 
to use the package (or the same specfile) on systems where it's not available
nor needed, 
such as older distro versions or (very) trimmed down installations."

Hey Ajax. Any replies to the stuff in comment #4? 
I think this is close to done. Perhaps you could also try again on the devel
list for someone to take it over. ;) 

Any further news here Ajax? 

This package no longer exists. ;) 

Closing.