Bug 240808 - Review Request: xorg-x11-drv-vermilion - Intel Vermilion driver
Summary: Review Request: xorg-x11-drv-vermilion - Intel Vermilion driver
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-05-21 21:06 UTC by Adam Jackson
Modified: 2007-11-30 22:12 UTC (History)
0 users

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-05-26 17:54:53 UTC
Type: ---
Embargoed:
kevin: fedora-review+
tcallawa: fedora-cvs+


Attachments (Terms of Use)

Description Adam Jackson 2007-05-21 21:06:09 UTC
Spec URL: http://people.redhat.com/ajackson/xorg-x11-drv-vermilion.spec
SRPM URL: http://people.redhat.com/ajackson/xorg-x11-drv-vermilion-1.0.0-1.src.rpm
Description: Xorg driver for Intel Vermilion chips.

Apparently this is used in some new mobile platforms.  Don't have the hardware to test yet, but someone surely does.

Comment 2 Adam Jackson 2007-05-22 15:10:23 UTC
In case anyone's wondering, the 'no-wfb' patch disables the code path that goes
through the wfb rendering core, which doesn't exist in xserver 1.3.  It'll pop
into existence in 1.4 though.

As a side effect, the driver only works at 24bpp until wfb exists.

Comment 3 Kevin Fenzi 2007-05-22 18:47:00 UTC
Here's a review: 


OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (MIT)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
? - Sources match upstream md5sum:

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
See below - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Non blocker. Might ask upstream to include that this is released under
the MIT license in the COPYING file instead of that saying it's just a stub file.

2. Non blocker. rpmlint says:
W: xorg-x11-drv-vermilion mixed-use-of-spaces-and-tabs (spaces: line 3, tab:
line 12)
Fix if you get a chance.

3. Shouldn't this package "Require: hwdata" ? It puts a file under
/usr/share/hwdata/videoaliases/ which hwdata owns. Or does this get pulled in
from some dependency of xorg-x11-server-Xorg?

4. Doesn't build on ppc32. I get:
 gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/xorg -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -MT vermilion.lo -MD -MP -MF .deps/vermilion.Tpo -c vermilion.c  -fPIC
-DPIC -o .libs/vermilion.o
vermilion.c: In function 'VERMILIONKernelOpen':
vermilion.c:380: warning: ignoring return value of 'fgets', declared with
attribute warn_unused_result
vermilion.c:916:2: error: #error VERMILIONReadMemory and VERMILIONWriteMemory
only work on little endian
make[2]: *** [vermilion.lo] Error 1
make[2]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.8790 (%build)

Does this hardware exist on ppc? Perhaps ppc/ppc64 should also be excluded?

5. I wasn't yet able to verify the md5 on the sources.
Will do that later today once freedesktop is back up.


Comment 4 Adam Jackson 2007-05-22 20:18:30 UTC
(In reply to comment #3)

> 2. Non blocker. rpmlint says:
> W: xorg-x11-drv-vermilion mixed-use-of-spaces-and-tabs (spaces: line 3, tab:
> line 12)
> Fix if you get a chance.

Fixed.

> 3. Shouldn't this package "Require: hwdata" ? It puts a file under
> /usr/share/hwdata/videoaliases/ which hwdata owns. Or does this get pulled in
> from some dependency of xorg-x11-server-Xorg?

% rpm -q --whatrequires hwdata       
pciutils-2.2.3-4
usbutils-0.71-2.1
kudzu-1.2.64-2
isdn4k-utils-3.2-54.fc7
rhpxl-0.43-1.fc7
system-config-display-1.0.51-1.fc7

None of those are in the dep chain for Xorg, afaik.  None of the other drivers
are requiring on hwdata yet, but they're all languishing in mergereview so
that's not really an excuse.

Fixed.

> 4. Doesn't build on ppc32. I get:
>  gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/xorg -O2 -g -pipe -Wall
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
> -m32 -MT vermilion.lo -MD -MP -MF .deps/vermilion.Tpo -c vermilion.c  -fPIC
> -DPIC -o .libs/vermilion.o
> vermilion.c: In function 'VERMILIONKernelOpen':
> vermilion.c:380: warning: ignoring return value of 'fgets', declared with
> attribute warn_unused_result
> vermilion.c:916:2: error: #error VERMILIONReadMemory and VERMILIONWriteMemory
> only work on little endian
> make[2]: *** [vermilion.lo] Error 1
> make[2]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0/src'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0'
> make: *** [all] Error 2
> error: Bad exit status from /var/tmp/rpm-tmp.8790 (%build)
> 
> Does this hardware exist on ppc? Perhaps ppc/ppc64 should also be excluded?

I'm not even close to sure.  I think it's x86 only for now.

Changed to ExclusiveArch: %{ix86}, might add x86_64 if the need presents itself.
 I suspect this is in-chipset hardware so would never need to be added to
anything but intel chipsets.

Updated:
http://people.redhat.com/ajackson/vermilion/xorg-x11-drv-vermilion.spec
http://people.redhat.com/ajackson/vermilion/xorg-x11-drv-vermilion-1.0.0-2.src.rpm

Comment 5 Kevin Fenzi 2007-05-23 03:17:26 UTC
2. ok. looks good. 
3. ok. looks good. 
4. ok. Note that it did build just fine on x86_64 here, but I don't know if the
hardware appears there or not. 
5. I was able to get thru to freedesktop.org now, and verify the md5sum (which
checks out fine). 
3a45adbfbcae487cf04dbcd089533c3d  xf86-video-vermilion-1.0.0.tar.bz2
3a45adbfbcae487cf04dbcd089533c3d  xf86-video-vermilion-1.0.0.tar.bz2.1

I see no further blockers, so this package is APPROVED. 

Don't forget to close this once it's been imported and built. 

Comment 6 Adam Jackson 2007-05-24 17:44:58 UTC
New Package CVS Request
=======================
Package Name: xorg-x11-drv-vermilion
Short Description: Xorg driver for Intel Vermilion chipset
Owners: ajackson
Branches: F-7
InitialCC:

Comment 7 Tom "spot" Callaway 2007-05-24 18:19:43 UTC
cvs done

Comment 8 Adam Jackson 2007-05-26 17:54:53 UTC
Imported, closing.  Thanks all!


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