Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.

Bug 226638

Summary: Merge Review: xorg-x11-filesystem
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Adam Jackson <ajax>
Status: CLOSED NOTABUG QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ajax, fedora, redhat-bugzilla, tcallawa
Target Milestone: ---Flags: fedora: fedora-review-
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-08-04 17:52:03 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Nobody's working on this, feel free to take it 2007-01-31 21:32:31 UTC
Fedora Merge Review: xorg-x11-filesystem

http://cvs.fedora.redhat.com/viewcvs/devel/xorg-x11-filesystem/
Initial Owner: ajackson@redhat.com

Comment 1 Thorsten Leemhuis 2007-02-04 15:38:00 UTC
* I'm a bit unsure about this package in general -- is it really still needed?
FC5 has modular X already, and we don't support from older releases anymore
iirc. RHEL5 should have this package, too, and RHEL6 probably should not need it
anymore, too.

* why doesn't this package simply own some of the other important directorys
like /usr/lib/xorg/modules/

* Stuff like "cat > "$RPM_BUILD_ROOT/${UPGRADE_CMD}" <<'EOF'" is disliked; it
should live in a separate file that it included as source

* Quoting the spec
{{{
# NOTE: Do not replace these with _libdir or _includedir macros, they are
#       intentionally explicit.
}}}
Nice, the comment helps -- but it would help more if the reason why its
"intentionally explicit" would be mentioned ;-) Ohh, it's explained later in the
spec; Not importatn, but maybe mention in once at the top of the spec file
properly might be the best

* rpmlint:
rpmlint on ./xorg-x11-filesystem-7.1-2.fc7.noarch.rpm
W: xorg-x11-filesystem incoherent-version-in-changelog 7.1-2.fc6 7.1-2.fc7
-> simply avoid mention the disttag in the changelog

W: xorg-x11-filesystem invalid-license MIT/X11
-> Would be MIT, but what actualy is licenced under MIT/X11 ? 

W: xorg-x11-filesystem no-documentation
-> acceptable

E: xorg-x11-filesystem standard-dir-owned-by-package /usr/lib/X11
-> owned by package "filesystem", so not needed

W: xorg-x11-filesystem dangerous-command-in-%pre rm

rpmlint on ./xorg-x11-filesystem-7.1-2.fc7.src.rpm
W: xorg-x11-filesystem invalid-license MIT/X11
-> see above

E: xorg-x11-filesystem hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/X11"
E: xorg-x11-filesystem hardcoded-library-path in /usr/lib/X11
E: xorg-x11-filesystem hardcoded-library-path in /usr/lib/X11
E: xorg-x11-filesystem hardcoded-library-path in /usr/lib/X11
E: xorg-x11-filesystem hardcoded-library-path in /usr/lib/X11
E: xorg-x11-filesystem hardcoded-library-path in /usr/lib/X11
-> accpetable in this case

W: xorg-x11-filesystem no-%build-section
-> accpetable in this case

Stopping reviewing here for now until it becomes clear this is still needed

Comment 2 Tom "spot" Callaway 2009-01-16 16:01:41 UTC
Alright, lets pick this old merge review up, because I think we can beat it into shape.

The biggest item that I see here is that there is an embedded update "script". That would make a lot more sense to have it live as a Source file, especially since it is not using any rpm macros. It would also simplify the rpm spec file quite a bit.

There is the question as to whether this script (and the %pre copy) are still necessary in Fedora. If you think so, please keep them, if not, please remove them both from the package.

Please add an empty %build section.

Also, %dir %{_bindir}/xorg-x11-filesystem-upgrade is just wrong. That's a script, not a directory.

The last issue is that there seems to be fair bit of directory ownership duplication in the xorg stack. 

/usr/lib/X11: filesystem, xorg-x11-filesystem

/usr/include/X11/: xorg-x11-filesystem, libfontenc-devel, libxkbfile-devel, libXdmcp-devel, libXfixes-devel, libICE-devel, libSM-devel, libXau-devel, libXt-devel, libXpm-devel, libXmu-devel, libXft-devel, libXv-devel, libXcursor-devel, libXvMC-devel, libXaw-devel, libXevie-devel, libXres-devel, libXfont-devel, libXcomposite-devel, libXrender-devel, libXdamage-devel, xorg-x11-xtrans-devel, libX11-devel, libXrandr-devel, xorg-x11-proto-devel

/usr/share/X11: xorg-x11-filesystem, xorg-x11-server-utils, xorg-x11-font-utils, xorg-x11-utils, imake, libX11, xkeyboard-config

If we don't need the upgrade script anymore, do we need this package anymore? Could we let filesystem own /usr/lib/X11 and /usr/share/X11, xorg-x11-proto-devel own /usr/include/X11 (and all those other dupes should Require: xorg-x11-proto-devel)

Comment 3 Adam Jackson 2009-07-23 14:06:28 UTC
Took another look at this, and I'm pretty sure we can just drop this package outright by now.  I've started removing all the explicit deps on xorg-x11-filesystem, and added /usr/share/X11 to filesystem.

The only question I have is how (or whether) xorg-x11-filesystem should be obsoleted so that it gets uninstalled from any existing systems.  I don't think it's strictly necessary, since it's not like it _does_ anything...

Comment 4 Tom "spot" Callaway 2009-07-23 14:11:51 UTC
Probably should just let filesystem Provide/Obsolete: xorg-x11-filesystem.

Comment 5 Adam Jackson 2009-08-04 17:52:03 UTC
Dead in rawhide, nothing Requires: it anymore and filesystem Prov/Obs it as suggested in comment #4.