Bug 532874 - Review Request: xorg-x11-drv-wacom - Xorg X11 wacom input driver
Summary: Review Request: xorg-x11-drv-wacom - Xorg X11 wacom input driver
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 538036
TreeView+ depends on / blocked
 
Reported: 2009-11-04 06:09 UTC by Peter Hutterer
Modified: 2009-11-20 00:45 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-11-19 23:56:15 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Peter Hutterer 2009-11-04 06:09:34 UTC
Spec URL: http://people.freedesktop.org/~whot/wacom.spec
SRPM URL: http://people.freedesktop.org/~whot/xorg-x11-drv-wacom-0.10.0-1.fc12.src.rpm
Description: X.Org X11 wacom input driver for Wacom tablets. 


Note: xorg-x11-drv-wacom is the forked linuxwacom driver now hosted on X.Org infrastructure (and maintained in git). This driver supports X servers 1.7 and newer while the linuxwacom driver goes into maintenance mode for X servers up to including 1.6. We currently maintain a patchset for linuxwacom to build against our X server, in the future linuxwacom will be declared obsolete by this driver.

Comment 1 Peter Lemenkov 2009-11-06 15:19:52 UTC
It seems that Linux Wacom is still active (they released 0.8.4 very recently). Could you, please, add more details about the reasons, which led to the fork?,

Comment 2 Thomas Spura 2009-11-06 15:28:39 UTC
Just a few comments about the spec:

- rpmlint:
  xorg-x11-drv-wacom.src: E: invalid-spec-name
  xorg-x11-drv-wacom.src: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab:
  line 54)
  1 packages and 0 specfiles checked; 1 errors, 1 warnings.

  The spec should be xorg-x11-drv-wacom.spec…

- In the devel package is a *.pc file, so at least the devel package should 
  require pkg-config

- %global over %define, see:
 
https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

- Does the devel package really need to own %{_includedir}/xorg? I'd say, that 
  you should require xorg-x11-server-devel to get this folder and don't own
  this dir.

Comment 3 Peter Hutterer 2009-11-09 01:03:11 UTC
(In reply to comment #1)
> It seems that Linux Wacom is still active (they released 0.8.4 very recently).
> Could you, please, add more details about the reasons, which led to the fork?,  

Ping (upstream linuxwacom maintainer) and I agreed that a fork is best for getting a cleaner and more efficient X driver.
linuxwacom has become a hard-to maintain piece of code, with the X driver trying to support everything from XFree86 3 up to X.Org Server 1.6. I have the experience with X drivers, so the first thing we agreed on was to ditch pre X server 1.6 support and go with 1.7 and later.

in terms of future development, linuxwacom and xf86-input-wacom will be developed in parallel when it comes to new hardware support and general bug fixes. linuxwacom will not see the updates to server 1.7, so any attempt of supporting this in F12 will be a messy patch set (like it is now already).

note that Ping is a kernel developer responsible for new hardware support. The X driver should not have any special hardware dependencies - that's what the kernel is for. the forked driver is a major step towards that.

New specfile coming up in a minute.

Comment 4 Peter Hutterer 2009-11-09 01:27:20 UTC
New spec file and srpm:

http://people.freedesktop.org/~whot/xorg-x11-drv-wacom.spec
http://people.freedesktop.org/~whot/xorg-x11-drv-wacom-0.10.0-1.fc12.src.rpm

Changes:
- spec file renamed to xorg-x11-drv-wacom.spec 
- leftover tab replaced with spaces
- s/define/global/
- devel package requires pkgconfig and xorg-x11-server-devel

Comment 5 Thomas Spura 2009-11-14 16:26:46 UTC
Peter, I have the hp touchsmart tx2 and on some webpages, there is mentioned, that this is a wacom tablet pc, so it should be working with this driver...

But I can't get it working with F-12. Is there anything I can help with testing?
Maybe I'm doing something wrong, but shouldn't the display be autodetected anyway, when this driver is installed?

Comment 6 Peter Hutterer 2009-11-17 00:06:18 UTC
Thomas - please file a separate bugreport for this issue, let's not use the review request for actual bugs on this driver. Please attach the output of lshal and your Xorg.log to the new bug (if you're using this driver, temporarily file under linuxwacom and note that you're using xf86-input-wacom instead). Make sure you assign the bug to me to avoid confusion. Thanks.

Comment 7 Thomas Spura 2009-11-17 19:49:27 UTC
REVIEW:

Good:
- spec legible
- %global is used
- correct spec name
- compiler optflags are in %configure
- no static libs
- %clean section exists
- removing .la files
- BR/R are ok
- buildroot ok
- builds in koji:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1812867
- License GPLv2+ ok, but no license text, see should items.
- source match upstream, both md5 91af5fab1637c6ce52e3f9418d53e22b
- Groups ok
- %files section is ok, except missing docs see below


Needswork:
- permissions not ok, see rpmlint
- rpmlint: $ rpmlint xorg-x11-drv-wacom.spec xorg-x11-drv-wacom-0.10.0-1.fc12.src.rpm x86_64/xorg-x11-drv-wacom-*
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/xf86WacomDefs.h
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmCommon.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmCompat.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmConfig.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/xf86Wacom.h
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/xf86Wacom.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmUSB.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmISDV4.c
xorg-x11-drv-wacom-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/xf86-input-wacom-0.10.0/src/wcmFilter.c
xorg-x11-drv-wacom-devel.x86_64: W: summary-ended-with-dot Xorg X11 wacom input driver development package.
xorg-x11-drv-wacom-devel.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 11 warnings.

  * for the spurious permission:
    e.g. run in %prep: find -type f -name '*.c' | xargs chmod -x
  * no-documentation  in devel is ignorable

- no documentation in main package:
  There is AUTHORS, ChangeLog, README.

_____________________

Should:
- xorg-x11-drivers requires linuxwacom. When this package is build, the drivers package should require this one instead.
- Add a the license text upstream and add it to %doc, see https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
- devel package just contains text files -> should be noarch

You should bump the release and change the changelog, when you make changes to the spec.

Comment 8 Peter Hutterer 2009-11-18 01:50:47 UTC
Thanks for the review, items are addressed:

http://people.freedesktop.org/~whot/xorg-x11-drv-wacom.spec
http://people.freedesktop.org/~whot/xorg-x11-drv-wacom-0.10.0-2.fc12.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=1813431

Changes (this time with changelog in specfile):
- Obsolete linuxwacom, don't Conflict with it.
- Remove trailing dot from summary (rpmlint warning).
- Remove spurious executable bits from source files (rpmlint warning).
- Add AUTHORS, ChangeLog, README to doc

For the reasons outlined in Comment #3, I think it's better to obsolete linuxwacom instead of just conflicting with it. This should also solve the issue with xorg-x11-drivers (though I will update that to require xorg-x11-drv-wacom).

> - rpmlint: $ rpmlint xorg-x11-drv-wacom.spec
This didn't give me any of the warnings when I ran it here, is there a specific flag I have to provide?

> - Add a the license text upstream and add it to %doc, see
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

done, file was already upstream, just missed out on being included in the tarball. Will be in the next version.

> - devel package just contains text files -> should be noarch

noarch requires installing the .pc file into _datadir to avoid 64-bit conflicts. This again seems to require upstream changes and is a bit iffy anyway in light of all the other xorg-x11-drivers installing into _libdir.
This should be cleaned up in one go across all drivers to maintain consistency.

Comment 9 Thomas Spura 2009-11-18 19:39:17 UTC
> Obsolete linuxwacom, don't Conflict with it.

I think, this can't work, because in bug #538036, you say, I should copy the fdi file from linuxwacom and that this file will be included in this package too. If you don't rename this file, when adding here, this would probably cause problems, when both are installed.
If you rename the file -> Obsolete,
no rename -> Conflict.
Maybe name it 10-wacom.fdi...

> > - rpmlint: $ rpmlint xorg-x11-drv-wacom.spec
> This didn't give me any of the warnings when I ran it here, is there a specific
> flag I have to provide?

No, this was a full copy from my terminal. I ran rpmlint on the spec, the src.rpm and all generated rpms in one command. See one line below:
- rpmlint: $ rpmlint xorg-x11-drv-wacom.spec
xorg-x11-drv-wacom-0.10.0-1.fc12.src.rpm x86_64/xorg-x11-drv-wacom-*


One last thing:
- run make %{?_smp_mflags} and not just make to enable building targets in parallel, see https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

____________________


All other issues, except the 'non-working' at least for me, are fixed:

-> APPROVED

Comment 10 Peter Hutterer 2009-11-18 22:27:21 UTC
(In reply to comment #9)
> > Obsolete linuxwacom, don't Conflict with it.
> 
> I think, this can't work, because in bug #538036, you say, I should copy the
> fdi file from linuxwacom and that this file will be included in this package
> too. If you don't rename this file, when adding here, this would probably cause
> problems, when both are installed.
> If you rename the file -> Obsolete,
> no rename -> Conflict.
> Maybe name it 10-wacom.fdi...

ok, I'll make sure to do that and check the conflicts. note that 0.10.0 is a developer release, hence the less-than optimal fdi file installation.
http://lists.freedesktop.org/archives/xorg/2009-November/047927.html
hotplugging is fixed upstream, i'll get a new release out soon to avoid this.

> One last thing:
> - run make %{?_smp_mflags} and not just make to enable building targets in
> parallel, see https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

done, will be in the import.
thanks for the review.

> All other issues, except the 'non-working' at least for me, are fixed:
> 
> -> APPROVED

New Package CVS Request
=======================
Package Name: xorg-x11-drv-wacom
Short Description: Xorg X11 wacom input driver
Owners: whot
Branches: F-12
InitialCC: xgl-maint

Comment 11 Jason Tibbitts 2009-11-19 18:59:30 UTC
I've done CVS but the new pkgdb is having issues accepting the CC of xgl-maint for some reason, so I'll leave this in the CVS queue to remind me to get back to it when that's been fixed up.

Comment 12 Peter Hutterer 2009-11-19 23:56:15 UTC
Imported and built for devel. F12 packages coming soon. Closing.

Comment 13 Jason Tibbitts 2009-11-20 00:45:54 UTC
pkgdb is fixed now so I've added the requested CC entry.


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