Bug 226577 - Merge Review: xorg-x11-drv-apm
Merge Review: xorg-x11-drv-apm
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:22 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-20 13:36:22 EDT
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)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:22:20 EST
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-05 23:24:51 EDT
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 14:39:10 EDT
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 14:41:53 EDT
I'll take a closer look later today.
Comment 4 Jason Tibbitts 2007-05-25 01:02:53 EDT
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 15:14:14 EDT
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 16:20:46 EDT
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 15:11:36 EDT
Can we close this?
Comment 8 Adam Jackson 2007-06-20 13:36:22 EDT
Yes we can!

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