[Bug 249059] Review Request: wdaemon - hotplug helper for wacom x.org driver

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: wdaemon - hotplug helper for wacom x.org driver


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=249059





------- Additional Comments From arozansk at redhat.com  2007-07-23 17:45 EST -------
> 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


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.