Bug 226577 - Merge Review: xorg-x11-drv-apm
Summary: Merge Review: xorg-x11-drv-apm
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:22 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-20 17:36:22 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora-review+


Attachments (Terms of Use)

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

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

Comment 1 Jason Tibbitts 2007-05-06 03:24:51 UTC
Well, this the first driver (alphabetically) which will build on x86_64, so I
figured I'd take a look.

A cursory inspection only turns up two issues:

W: xorg-x11-drv-apm invalid-license MIT/X11
  which is kind of an odd way to state it, but I guess it's not incorrect.

The other issue is ownership of /usr/lib64/xorg/modules and
/usr/lib64/xorg/modules/drivers.  I thought there was a discussion at Fudcon
about this between you and spot, but I paid only enough attention to look out
for airborne furniture.

Minor stuff:

%define cvsdate xxxxxxx
  Not used in this spec, and aren't these things in git anyway?

Jesse stuck in a weird changelog line:
  * Wed Jul 12 2006 Jesse Keating <jkeating@redhat.com> - sh: line 0: fg: no job
control
Looks like a script took a bit of a dump there.

Umm, I guess that's about it.  Needless to say, I have absolutely no way to test
this; I'm not even sure what "apm" means in the context of video.

Comment 2 Adam Jackson 2007-05-24 18:39:10 UTC
Alliance ProMotion.  You don't have one, no one does.  Even _I_ don't, and I've
got nearly everything.

I was eventually convinced of the directory ownership thing; server owns,
drivers don't.  The license should be "MIT" instead, and the cvsdate thing was
just boilerplate from the initial creation.

The changelog thing, yeah, bad script, not my fault.

Fixed all but the changelog thing in 1.1.1-4 since I don't know what %revision
that corresponded to.  Once this passes review I'll mass-update all the other
drivers, which should make subsequent reviews more automatic.

Comment 3 Jason Tibbitts 2007-05-24 18:41:53 UTC
I'll take a closer look later today.

Comment 4 Jason Tibbitts 2007-05-25 05:02:53 UTC
OK, I pulled the latest source and built.  This BS rpmlint complaint squeaked in:
  W: xorg-x11-drv-apm mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 3)
I usually just ignore these.

Blame Jesse for the bogus changelog, I guess.  It's certainly not anything to
block on.

I recall from the vermillion review that you decided these drivers should grow a
dependency on hwdata.

At first I was going to complain about the COPYING file not getting into the
final package, but then I read the file:
  This is a stub file.  This package has not yet had its complete licensing
  information compiled.
so I don't suppose there would be much point.

I'm kind of surprised xorg-x11-server-sdk doesn't have a dependency on
pkgconfig.  It looks like it should, since it included a .pc file.  That would
mean that you don't need to have the separate build-time dependency in all of
these drivers.

So really this is fine except for /usr/share/hwdata/videoaliases being unowned.

* source files match upstream:
   09d7e6cf30b94f141f8ebe2560b301a058f645e74108edc28c908e750865dcec  
   xf86-video-apm-1.1.1.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint has only ignoreable errors.
* final provides and requires are sane:
   apm_drv.so()(64bit)
   xorg-x11-drv-apm = 1.1.1-4.fc7
  =
   xorg-x11-server-Xorg >= 1.0.99.901
* %check is not present; not possible to automatically test, and hardware to
  test this is hard to come by in any case.
* no shared libraries are added to the regular linker search paths.
X places a file in /usr/share/hwdata/videoaliases but doesn't own it or depend 
  on anything that does.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* 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 static libraries.
* no libtool .la droppings.

Comment 5 Adam Jackson 2007-05-26 19:14:14 UTC
I fixed xorg-x11-server-sdk 1.3.0.0-6 to Requires: pkgconfig, so now this driver
doesn't have to.  Also added a requires on hwdata.

So, 1.1.1-5 should be golden.

Comment 6 Jason Tibbitts 2007-05-26 20:20:46 UTC
Yes, indeed, I think everything's good now.

APPROVED

Just ping (email, IRC, whatever) when you think the other drivers are ready and
I'll start going through them.

Comment 7 Jason Tibbitts 2007-06-14 19:11:36 UTC
Can we close this?

Comment 8 Adam Jackson 2007-06-20 17:36:22 UTC
Yes we can!


Note You need to log in before you can comment on or make changes to this bug.