Bug 249059 - Review Request: wdaemon - hotplug helper for wacom x.org driver
Summary: Review Request: wdaemon - hotplug helper for wacom x.org driver
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jarod Wilson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-07-20 17:13 UTC by Aristeu Rozanski
Modified: 2009-02-26 14:35 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-02-26 14:35:44 UTC
Type: ---
Embargoed:
jarod: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to /etc/udev/rules.d/50-udev.rules to create a /dev/uinput symlink to /dev/input/uinput (464 bytes, patch)
2007-08-01 15:34 UTC, Alfonso Gazo
no flags Details | Diff
Patch to /etc/udev/scripts/is_uinput.sh to let it detect vendor and product IDs correctly (510 bytes, patch)
2007-08-01 15:36 UTC, Alfonso Gazo
no flags Details | Diff

Description Aristeu Rozanski 2007-07-20 17:13:14 UTC
Spec URL: http://people.redhat.com/arozansk/wdaemon/wdaemon.spec
SRPM URL: http://people.redhat.com/arozansk/wdaemon/wdaemon-0.10-1.fc8.src.rpm
Description: wdaemon creates and maintains virtual input devices that represent
real devices that can be hotplugged on the system at any time. This package is
needed while X.org doesn't properly support input devices hotplugging.

Comment 1 Jarod Wilson 2007-07-23 20:44:25 UTC
First pass through the spec...

1) Why the use of ExclusiveArch, then list just about all possible arches? I
think an ExcludeArch for the arch or arches it doesn't build on might be better.
So far as I can see, that list is really just s390/s390x, no?

2) %makeinstall is a big no-no :)
http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002

3) the CFLAGS aren't being honored.

I've actually got a patch in hand that'll make 'make install
DESTDIR=$RPM_BUILD_ROOT' work, as well as the CFLAGS honored:

----8<----
--- wdaemon-0.10/Makefile       2007-07-20 12:07:44.000000000 -0400
+++ wdaemon-0.10.updated/Makefile       2007-07-23 16:30:06.000000000 -0400
@@ -1,4 +1,4 @@
-CFLAGS = -O0 -g -Wall
+CFLAGS ?= -O0 -g -Wall
 OBJS = hotplug.o \
        input.o \
        monitored.o \
@@ -24,10 +24,10 @@ wdaemon: $(OBJS)
        gcc $(CFLAGS) -c -o $@ $<
 
 install:
-       mkdir -p $(bindir)
-       cp wdaemon $(bindir)/
-       mkdir -p $(sysconfdir)/rc.d/init.d/
-       cp wdaemon.initrd $(sysconfdir)/rc.d/init.d/wdaemon
+       mkdir -p $(DESTDIR)$(bindir)
+       cp wdaemon $(DESTDIR)$(bindir)/
+       mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d/
+       cp wdaemon.initrd $(DESTDIR)$(sysconfdir)/rc.d/init.d/wdaemon
 
 clean:
        rm -f *.o wdaemon core
----8<----

4) I'd install is_uinput.sh mode 755 instead of 644, which also eliminates the
need for the %attr stuff on that file in %files.

That's all I've got so far... rpmlint output is fairly clean, just two warnings
about the udev rules files not being marked as config files. Not sure yet if
they should be, or if we just ignore those.

Comment 2 Aristeu Rozanski 2007-07-23 21:45:59 UTC
> 1) Why the use of ExclusiveArch, then list just about all possible arches? I
> think an ExcludeArch for the arch or arches it doesn't build on might be better.
> So far as I can see, that list is really just s390/s390x, no?
well, yes. But I'd prefer to add new supported architectures instead of
allowing everything that could appear. This is really needed?


> 2) %makeinstall is a big no-no :)
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002
fixed.

> 3) the CFLAGS aren't being honored.
fixed.
 
> I've actually got a patch in hand that'll make 'make install
> DESTDIR=$RPM_BUILD_ROOT' work, as well as the CFLAGS honored:
> 
> ----8<----
> --- wdaemon-0.10/Makefile       2007-07-20 12:07:44.000000000 -0400
> +++ wdaemon-0.10.updated/Makefile       2007-07-23 16:30:06.000000000 -0400
> @@ -1,4 +1,4 @@
> -CFLAGS = -O0 -g -Wall
> +CFLAGS ?= -O0 -g -Wall
>  OBJS = hotplug.o \
>         input.o \
>         monitored.o \
> @@ -24,10 +24,10 @@ wdaemon: $(OBJS)
>         gcc $(CFLAGS) -c -o $@ $<
>  
>  install:
> -       mkdir -p $(bindir)
> -       cp wdaemon $(bindir)/
> -       mkdir -p $(sysconfdir)/rc.d/init.d/
> -       cp wdaemon.initrd $(sysconfdir)/rc.d/init.d/wdaemon
> +       mkdir -p $(DESTDIR)$(bindir)
> +       cp wdaemon $(DESTDIR)$(bindir)/
> +       mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d/
> +       cp wdaemon.initrd $(DESTDIR)$(sysconfdir)/rc.d/init.d/wdaemon
>  
>  clean:
>         rm -f *.o wdaemon core
> ----8<----
> 
> 4) I'd install is_uinput.sh mode 755 instead of 644, which also eliminates the
> need for the %attr stuff on that file in %files.
fixed.

> That's all I've got so far... rpmlint output is fairly clean, just two warnings
> about the udev rules files not being marked as config files. Not sure yet if
> they should be, or if we just ignore those.
I don't think they're configuration files because the user has no reason to
change the rules, only add more and that could be done in a different file.

I've fixed all your comments but #1 and the updated version is on:
http://people.redhat.com/arozansk/

Thanks for the review Jarod


Comment 3 Jarod Wilson 2007-07-24 15:16:47 UTC
Fedora Package Review: wdaemon
------------------------------

MUST Items:

* rpmlint output acceptable (post full output w/waiver notes where needed): 
  $ rpmlint /build/RPMS/x86_64/wdaemon-*0.10-2*
  W: wdaemon non-conffile-in-etc /etc/udev/rules.d/61-uinput-wacom.rules
  W: wdaemon non-conffile-in-etc /etc/udev/rules.d/61-uinput-stddev.rules

  I'm thinking it wouldn't hurt to just mark these %config(noreplace), in the
event a user does go and edit them/append to them. It completely silences
rpmlint if we go that route, and I don't see any real reason not to just do it.

* Meets Package Naming Guidelines: PASS

* spec file name matches %{name}, in the format %{name}.spec (nb: there are a
few exceptions): PASS

* The package must meet the Packaging Guidelines: PASS

* open-source compatible license and meets fedora legal reqs: PASS

* License field in spec matches actual license: PASS 

* If source includes text of license(s) in its own file, that file must be in
%doc: PASS

* spec file legible and in American English: PASS

* sources used match the upstream source, as provided in spec URL. Verify with
md5sum (if no upstream URL, source creation method must be documented and can be
verified using diff): PASS

  $ md5sum wdaemon-0.10.tar.bz2*
  9c90cefbe4ae7d6c79ded408f9a435a7  wdaemon-0.10.tar.bz2
  9c90cefbe4ae7d6c79ded408f9a435a7  wdaemon-0.10.tar.bz2.1

* produces binary rpms on at least one arch: PASS (f7/x86_64)

* If ExcludeArch used, must be documented why (and a bug filed against ExArch
tracker once approved): NEEDS WORK

  Generally, we're to assume the package will build on any architecture, and
only exclude it from building on a certain arch if there's a good reason to do
so. ExclusiveArch is really frowned upon, unless its a package that really only
makes sense on a very limited set or arches.

* BuildRequires are sane: PASS

* locales, if necessary, handled properly with %find_lang: N/A

* if package contains shared libs, calls ldconfig in %post/postun: N/A

* if package is relocatable, must justify: N/A

* package owns all directories it creates: PASS

* no duplicates in %files: PASS

* Permissions on %files sane: PASS -- though I might suggest some further
updates to the Makefile to use 'install -mXXXX' to install the binaries and init
script, rather than having to use %attr in the %files section.

* %clean includes rm -rf %{buildroot}/$RPM_BUILD_ROOT: PASS

* macros used consistently: PASS

* package contains code, or permissable content: PASS

* Large/lots of docs, if present, should go in a -doc subpackage: N/A

* files in %doc aren't required for package to work: PASS

* Header files in -devel package: N/A

* Static libs in -static package: N/A

* package Reqs: pkgconfig if pkgconfig(.pc) files present: N/A

* if package has versioned libs, unversioned ones go in -devel package: N/A

* if present, -devel packages must require the base package NVR (w/some rare
exceptions):  N/A

* no libtool archives (w/some rare exceptions): PASS

* if GUI app, include a %{name}.desktop file, installed with
desktop-file-install in the %install section (or justify why not): N/A

* don't own files or folders other package own (or justify why you must): PASS

* %install starts with rm -rf %{buildroot}/$RPM_BUILD_ROOT: PASS

* filenames in packages must be valid UTF-8: PASS


SHOULD Items (not absolutely mandatory, but highly encouraged)

* If source does not include license text(s), ask upstream to include it: N/A
(already included)

* description and summary sections in spec should contain translations for
supported Non-English languages, if available: N/A

* package should build in mock: PASS (f7/x86_64)

* package should build on all supported architectures: not tested

* package should function as expected: don't have hardware to test myself

* any scriptlets must be sane: N/A

* subpackages other than -devel require the base package using a fully versioned
dependency: N/A

* pkgconfig files go in -devel pkg, unless package is a devel tool itself: N/A

* If package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or
/usr/sbin consider requiring the package which provides the file instead of the
file itself: N/A

------------------------------

So for the short version, I'd add %config(noreplace) for the two udev rules
files, since it looks like it only helps and use ExcludeArch: if you really need
to have this not build on a specific arch. Although I suppose that could start
to get equally messy (mips, arm, s390, s390x...). Okay, if you do want to stick
with ExcludeArch, I suppose that's fine. One note though: do you really want to
exclude ppc64? Only other minor spec cleanup I'd suggest is to remove the
extraneous slashes between $RPM_BUILD_ROOT and %{_sysconfdir} on lines 36 and 37.

Package APPROVED, none of the above are blockers to me.

Comment 4 Jarod Wilson 2007-07-24 15:25:21 UTC
Excerpt from http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :

- MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed
in bugzilla, describing the reason that the package does not compile/build/work
on that architecture. The bug number should then be placed in a comment, next to
the corresponding ExcludeArch line. New packages will not have bugzilla entries
during the review process, so they should put this description in the comment
until the package is approved, then file the bugzilla entry, and replace the
long explanation with the bug number. (Extras Only) The bug should be marked as
blocking one (or more) of the following bugs to simplify tracking such issues:
FE-ExcludeArch-x86, FE-ExcludeArch-x64, FE-ExcludeArch-ppc, FE-ExcludeArch-ppc64

I believe something similar should be done if ExclusiveArch is used instead of
ExcludeArch, particularly if the ppc64 exclusion was intentional. If ppc64 is
added to the ExclusiveArch for this package, then I think there probably isn't
anything extra to be done here -- not building this for s390 just seems like
common sense, no need to file a bug.

Comment 5 Aristeu Rozanski 2007-07-24 16:18:52 UTC
> * rpmlint output acceptable (post full output w/waiver notes where needed): 
>   $ rpmlint /build/RPMS/x86_64/wdaemon-*0.10-2*
>   W: wdaemon non-conffile-in-etc /etc/udev/rules.d/61-uinput-wacom.rules
>   W: wdaemon non-conffile-in-etc /etc/udev/rules.d/61-uinput-stddev.rules
> 
>   I'm thinking it wouldn't hurt to just mark these %config(noreplace), in the
> event a user does go and edit them/append to them. It completely silences
> rpmlint if we go that route, and I don't see any real reason not to just do it.
fixed

> * If ExcludeArch used, must be documented why (and a bug filed against ExArch
> tracker once approved): NEEDS WORK
ok, removed excludearch/exclusivearch: the package will compile and work on
all architectures. It may be not useful if the machine doesn't has USB ports
to use a tablet.

> * Permissions on %files sane: PASS -- though I might suggest some further
> updates to the Makefile to use 'install -mXXXX' to install the binaries and init
> script, rather than having to use %attr in the %files section.
fixed.
All the fixes included as patches are queued for next upstream release

http://people.redhat.com/arozansk/wdaemon/


Comment 6 Alfonso Gazo 2007-08-01 15:34:53 UTC
Created attachment 160420 [details]
Patch to /etc/udev/rules.d/50-udev.rules to create a /dev/uinput symlink to /dev/input/uinput

Comment 7 Alfonso Gazo 2007-08-01 15:36:11 UTC
Created attachment 160421 [details]
Patch to /etc/udev/scripts/is_uinput.sh to let it detect vendor and product IDs correctly

Comment 8 Alfonso Gazo 2007-08-01 15:41:53 UTC
I attached two patches to let wdaemon work out-of-the box in fc7. I found two
issues with wdaemon:

First, wdaemon expect to find /dev/uinput device node, but I only have found
/dev/input/uinput. I really think this could be changed in the wdaemon source
code, but I temporally created a patch over 50udev.rules to create a symlink
from /dev/input/uinput to /dev/uinput so wdaemon is happy.

Second, the /etc/udev/scripts/is_uinput.sh detection logic for Vendor & Product
IDs does not seem to work right, and /dev/input/uinput-devices/xxx symlinks are
not created. I attached a patch to solve this.

Comment 9 Aristeu Rozanski 2007-08-01 15:43:23 UTC
(In reply to comment #6)
> Created an attachment (id=160420) [edit]
> Patch to /etc/udev/rules.d/50-udev.rules to create a /dev/uinput symlink to
> /dev/input/uinput
Hi Alfonso, this one should be fixed during the build and I'll fix that in wdaemon
package instead of creating an udev rule. /dev/uinput is used in RHEL-4 and
wdaemon was developed to solve a problem in RHEL-4, that's why it defaults to
/dev/uinput instead of /dev/input/uinput.


Comment 10 Aristeu Rozanski 2007-08-01 15:44:30 UTC
> Second, the /etc/udev/scripts/is_uinput.sh detection logic for Vendor & Product
> IDs does not seem to work right, and /dev/input/uinput-devices/xxx symlinks are
> not created. I attached a patch to solve this.
Thanks, I'll apply this fix upstream and in the spec.


Comment 11 Aristeu Rozanski 2007-08-01 17:13:17 UTC
New upstream version/package with the fixes applied:
http://people.redhat.com/arozansk/wdaemon/


Comment 12 Aristeu Rozanski 2007-08-29 15:42:00 UTC
New Package CVS Request
=======================
Package Name: wdaemon
Short Description: x.org hotplug helper for Wacom tablets
Owners: arozansk
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits: yes


Comment 13 Aristeu Rozanski 2007-08-29 16:03:33 UTC
New Package CVS Request
=======================
Package Name: wdaemon
Short Description: hotplug helper for wacom x.org driver
Owners: arozansk
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits: yes


Comment 14 Aristeu Rozanski 2007-08-29 16:04:35 UTC
fixing the 'owners' field and the short description to match the BZ#'s


Comment 15 Kevin Fenzi 2007-08-29 19:59:36 UTC
cvs done.

Comment 16 Fedora Update System 2007-09-12 16:41:57 UTC
wdaemon-0.11-1.fc7 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Aristeu Rozanski 2009-02-26 14:35:44 UTC
The package is in. closing the bug.


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