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@xxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review