Bug 226573

Summary: Merge Review: xorg-x11-drivers
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Adam Jackson <ajax>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ajax, fedora
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: 2007-02-26 22:43:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

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

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

Comment 1 Thorsten Leemhuis 2007-02-03 22:58:46 UTC
* I understand the reasons for this package, but it looks like a maintaince
nightmare as somebody needs to make sure this package is updated each time new
drivers get added to the tree. That sucks :-/ -- for example currently there are
at least xorg-x11-drv-amd and xorg-x11-drv-tek4957 available on i386, but not
requires by this (I don't think that's on purpose).

 Suggestions to improve it (just for discussion, I'm unsure what the proper
solution is): ship a script that generates the template for the specfile from
cvs or "yum list" automatically. Or use something similar to the (ugly)trick
that is used in the kmod packages to run a script and include it's output in the
spec file instead of hardcoding the output in the spec -- Then a simple rebuild
should do everything correctly. 

 BTW, in case we stick to the current solution: the script mentioned in the
comment would actually be more useful if one would know what what
"xorg-all-drivers.txt" is or how it can be generated

* Quoting the spec
{{{
# This should match the list of architectures we build the Xorg server for.
# Note the lack of s390{,x}.
ExclusiveArch: %{ix86} x86_64 ia64 ppc ppc64 alpha sparc sparc64
}}} 
 and all those
{{{
%ifarch foo
Requires: bar
%endif
}}}
 It IMHO would me wiser if we'd could use a ExcludeArch and ifnarch those
packages and archs where we now those drivers don't exisit. But that's just my
opinion and a detail and probably not worth the work...

* rpmlint
rpmlint on ./xorg-x11-drivers-7.1-3.i386.rpm
W: xorg-x11-drivers invalid-license MIT/X11
-> MIT would be correct; But I fail what precisely is MIT licensed here...

E: xorg-x11-drivers obsolete-not-provided xorg-x11
-> Why is that obsolete there in any case? 

E: xorg-x11-drivers no-binary
- acceptable in this case

W: xorg-x11-drivers no-documentation
- might be a good idea to just create a small README that exaplains the purpose
of this package (any maybe what might happen if you remove it)

rpmlint on ./xorg-x11-drivers-7.1-3.src.rpm
W: xorg-x11-drivers invalid-license MIT/X11
-> see above

W: xorg-x11-drivers unversioned-explicit-obsoletes xorg-x11
-> if the obsoletes needs to stay better provide one with a version number --
that way we might be able to create a package with that name n the future

* MISC:

 * "URL: http://www.redhat.com", I don't think that's helpful (might even be
confusing), so maybe it should be removed

 * dist-tags are no must, but might be nice to use

* Besides the stuff outlines above:
 package meets naming and packaging guidelines.
 specfile is properly named, is cleanly written and uses macros consistently.
 BuildRequires are proper.
 no shared libraries are present.
 package is not relocatable.
 no duplicates in %files.
 file permissions are appropriate.
 %clean is present.
 %check is present and all tests pass:
        (include the summary from the test suite, if any)
 no scriptlets present.
 code, not content.
 documentation is small, so no -docs subpackage is necessary.
 %docs are not necessary for the proper functioning of the package.
 no headers.
 no pkgconfig files.
 no libtool .la droppings.
 not a GUI app.
 not a web app.
 no open bugs

Comment 2 Thorsten Leemhuis 2007-02-03 23:00:58 UTC
> no open bugs

Argh, no, that's untrue -- especially fixing Bug 198294 would be nice. The other
should probably just moved where they belong or closed

Comment 3 Adam Jackson 2007-02-19 18:59:32 UTC
Okay, did some cleanup.

First, Xorg is built on all arches except s390{,x}, so I flipped the
ExclusiveArch at the top of the file to be ExcludeArch.  In general I'd prefer
to use ExcludeArch, since there's no intrinsic reason for most of X to be
arch-specific.

I did the same to all the driver packages I could, with the exception of a few
that really are arch-specific.  There's only ~8 of these, and they're clearly
listed at the bottom of the Requires list now, along with a short comment for
each explaining why they're %ifarch'd.

Removed the Obsoletes: and fixed the License to be MIT.  It's just a
metapackage, there's really nothing to license, so picking the same license
class as the drivers it's meta for seems appropriate.

Which leaves us with:

% rpmlint xorg-x11-drivers-7.2-2.fc7.src.rpm
W: xorg-x11-drivers no-url-tag
% rpmlint i686/xorg-x11-drivers-7.2-2.fc7.i686.rpm
W: xorg-x11-drivers no-url-tag
E: xorg-x11-drivers no-binary
W: xorg-x11-drivers no-documentation

As above, not having a binary is fine for this package.  Removing the URL now
generates an rpmlint warning for not having a URL.  Eh.  I think it's better
without one.  Also I don't see the point in making a trivial doc payload, given
that it'd just be a repeat of the %description.

Comment 4 Dennis Gilmore 2007-02-19 19:08:58 UTC
i would like to see #199381 fixed with this also 

Comment 5 Adam Jackson 2007-02-20 16:19:57 UTC
(In reply to comment #4)
> i would like to see #199381 fixed with this also 

Do we even have those packaged yet?

Comment 6 Thorsten Leemhuis 2007-02-25 12:14:03 UTC
(In reply to comment #3)
> Okay, did some cleanup.

thx
 
> I did the same to all the driver packages I could, with the exception of a few
> that really are arch-specific. [...]

thx, looks a lot better. But I think I spotted a bug: xorg-x11-drv-i810 should
be required on x86_64, too, as there are x86_64 compatible boards with intel
graphic chipsets out there.

> [...]
> Which leaves us with:
> 
> % rpmlint xorg-x11-drivers-7.2-2.fc7.src.rpm
> W: xorg-x11-drivers no-url-tag
> % rpmlint i686/xorg-x11-drivers-7.2-2.fc7.i686.rpm
> W: xorg-x11-drivers no-url-tag
> E: xorg-x11-drivers no-binary
> W: xorg-x11-drivers no-documentation

> Also I don't see the point in making a trivial doc payload, given
> that it'd just be a repeat of the %description.

I'd say it would be worth the trouble.

Regarding the bugs:

- Fixing Bug 199381 would be nice and shouldn't do any harm, even if those
drivers are are not (yet) packaged in main Fedora -- but I don't consider this a
blocker (sorry dgilmore -- I'm counting on ajax cooperation in this case). 

- Bug 198294 will be fixed in another package

APPROVED

Comment 7 Adam Jackson 2007-02-26 16:41:45 UTC
(In reply to comment #6)

> > I did the same to all the driver packages I could, with the exception of a few
> > that really are arch-specific. [...]
> 
> thx, looks a lot better. But I think I spotted a bug: xorg-x11-drv-i810 should
> be required on x86_64, too, as there are x86_64 compatible boards with intel
> graphic chipsets out there.

Worse than that, there are ia64 boards with i810 chips too.  Fixed now.

> Regarding the bugs:
> 
> - Fixing Bug 199381 would be nice and shouldn't do any harm, even if those
> drivers are are not (yet) packaged in main Fedora -- but I don't consider this a
> blocker (sorry dgilmore -- I'm counting on ajax cooperation in this case). 

Until I know that the drivers are actually packaged, I don't see the need to
break the metapackage on sparc.

Comment 8 Thorsten Leemhuis 2007-02-27 05:59:26 UTC
(In reply to comment #7)
> > Regarding the bugs:
> > - Fixing Bug 199381 would be nice and shouldn't do any harm, even if those
> > drivers are are not (yet) packaged in main Fedora -- but I don't consider this 
> > a blocker (sorry dgilmore -- I'm counting on ajax cooperation in this case). 
> Until I know that the drivers are actually packaged, I don't see the need to
> break the metapackage on sparc.

Well, Fedora is not build for Sparc afaics, so no deps will be broken in our
tree. But the community guys that are building Fedora for Sparc ask for this --
so I suppose they know what they are doing and have those drivers packaged in
their tree, so deps will probably be satisfied there. So when they ask for it
I'd say that it is a good sign of collaboration with the community to add this
deps, as it doesn't do any harm to us, and saves them the work to adjust this
specfile in their tree.