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.
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.
> 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
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.
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.
> * 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/
Created attachment 160420 [details] Patch to /etc/udev/rules.d/50-udev.rules to create a /dev/uinput symlink to /dev/input/uinput
Created attachment 160421 [details] Patch to /etc/udev/scripts/is_uinput.sh to let it detect vendor and product IDs correctly
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.
(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.
> 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.
New upstream version/package with the fixes applied: http://people.redhat.com/arozansk/wdaemon/
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
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
fixing the 'owners' field and the short description to match the BZ#'s
cvs done.
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.
The package is in. closing the bug.