[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





------- Additional Comments From ajackson at redhat.com  2007-03-22 16:13 EST -------
> 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.

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