[Buildroot] [PATCH][resend] new package : p910nd print server
David Purdy
david.c.purdy at gmail.com
Fri Jun 15 21:43:33 UTC 2012
Arnout, bedankt ...
Your comments were very insightful and helpful. I addressed each item
you mentioned... [do I need to put a version history (listing changes)
with my V2 patch?]
On Wed, Jun 6, 2012 at 2:43 AM, Arnout Vandecappelle <arnout at mind.be> wrote:
> On 06/03/12 21:52, David Purdy wrote:
>>
> Please include a Signed-off-by line for yourself.
Yes, I normally do... it was actually a "resend" of the original git
send-email-generated patch, I guess no one saw the original one... ?
>
>
> The rest of my comments are a bit of nitpicking.
**Nitpicking?** Certainly just good suggestions - I highly appreciate
them. Hartelijk bedankt!
> [snip]
>
>> diff --git a/package/p910nd/Config.in b/package/p910nd/Config.in
>> new file mode 100644
>> index 0000000..490dace
>> --- /dev/null
>> +++ b/package/p910nd/Config.in
>> @@ -0,0 +1,8 @@
>> +config BR2_PACKAGE_P910ND
>> + bool "p910nd"
>> + help
>> + p910nd is a small printer daemon intended for diskless
>> + workstations. Using ports 9100-9102, it accepts
>> + print jobs and passes them directly to a USB printer.
>> +
>> + http://p910nd.sourceforge.net/
>
> Doesn't this package rely on any external libary at all?
> Not even libusb?
Nope, none at all.
>> diff --git a/package/p910nd/S55p910nd b/package/p910nd/S55p910nd
>> new file mode 100644
>> index 0000000..8778a62
>> --- /dev/null
>> +++ b/package/p910nd/S55p910nd
>> @@ -0,0 +1,48 @@
>> +#!/bin/sh
>> +
>> +DEFAULT=/etc/default/p910nd
>
>
> We don't normally have a /etc/default directory. Even so, it
> normally contains shell fragments rather than configuration files
> (at least it does on Debian+derivatives).
>
> Instead I would name the config file /etc/p910nd.conf
Done, you are right, it makes better sense.
>
>> +RUN_D=/var/run
>> +
>> +_start() {
>> + mkdir -p $RUN_D
>
>
> We usually use tabs to indent shell scripts.
Fixed.
>
>> + [ -f $DEFAULT ]&& (
>
>
> Space between ] and &&
>
> The sub-shell isn't needed, { ... } would be sufficient.
> Or it could even be left out since there's only one command
> (while) inside.
>
>
>> + while read port options; do
>> + case "$port" in
>> + ""|\#*)
>> + continue;
>> + esac
>> + p910nd $options $port
>
>
> We usually use start-stop-daemon to control daemons.
Will also change & clean up the entire initscript...
>
>> + if [ $? -ne 0 ]; then
>> + exit 1
>> + fi
>> + done
>> + )< $DEFAULT
>> + exit 0
>> +}
>> +
>> +_stop() {
>> + [ -f $DEFAULT ]&& (
>> + while read port options; do
>> + case "$port" in
>> + ""|\#*)
>> + continue;
>> + esac
>> + PID_F=$RUN_D/p910${port}d.pid
>> + [ -f $PID_F ]&& kill $(cat $PID_F)
>
>
> This has the annoying effect of not killing things properly
> if the configuration file has changed. What about:
>
> for PID_F in $RUN_D/p910?d.pid; do
> start-stop-daemon -K -p $PID_F -x /usr/sbin/p910nd
> done
Thanks for this - I'll certainly try to incorporate it.
>> + rm $PID_F
>> + done
>> + )< $DEFAULT
>> +}
>> +
>> +case $1 in
>> + start)
>> + _start
>> + ;;
>> + stop)
>> + _stop
>> + ;;
>> + *)
>> + echo "usage: $0 (start|stop)"
>
>
> Please add a restart as well.
Yup, that makes sense as well.
>> + exit 1
>> +esac
>> +exit $?
>> diff --git
>> a/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch
>>
>> b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch
>> new file mode 100644
>> index 0000000..867b8cf
>> --- /dev/null
>> +++
>> b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch
>> @@ -0,0 +1,13 @@
>> +Index: p910nd/p910nd.c
>
>
> Patches should start with a comment explaining what the patch does and why
> it is needed, just like a commit message. It should also contain a
> Signed-off-by line.
With your suggestions about P910ND_MAKE_OPTS, the patch is unnecessary
(yes, simpler this way) - thanks for the idea.
>> +===================================================================
>> +--- p910nd.orig/p910nd.c 2011-11-14 22:47:41.986401420 +0100
>> ++++ p910nd/p910nd.c 2011-11-14 22:49:27.274923524 +0100
>> +@@ -122,7 +122,7 @@
>> + #ifdef LOCKFILE_DIR
>> + #define LOCKFILE LOCKFILE_DIR "/p910%cd"
>
>
> Wouldn't it be possible to simply add -DLOCKFILE_DIR='"/var/lock"' to
> CFLAGS
> (if the Makefile allows extending CFLAGS with something extra).
>
>
>> + #else
>> +-#define LOCKFILE "/var/lock/subsys/p910%cd"
>> ++#define LOCKFILE "/var/lock/p910%cd"
>> + #endif
>> + #ifndef PRINTERFILE
>> + #define PRINTERFILE "/dev/lp%c"
>> diff --git a/package/p910nd/p910nd.default b/package/p910nd/p910nd.default
>> new file mode 100644
>> index 0000000..77317cf
>> --- /dev/null
>> +++ b/package/p910nd/p910nd.default
>> @@ -0,0 +1,9 @@
>> +# printing port list, in the form "number [options]"
>> +# where:
>> +# - number is the port number in the range [0-9]
>> +# the p910nd daemon will listen on tcp port 9100+number
>> +# - options can be :
>> +# -b to turn on bidirectional copying.
>> +# -f to specify a different printer device.
>> +#
>> +0 -b -f /dev/usb/lp0
>> diff --git a/package/p910nd/p910nd.mk b/package/p910nd/p910nd.mk
>> new file mode 100644
>> index 0000000..c229b32
>> --- /dev/null
>> +++ b/package/p910nd/p910nd.mk
>> @@ -0,0 +1,23 @@
>> +#############################################################
>> +#
>> +# p910nd
>> +#
>> +#############################################################
>> +
>> +P910ND_VERSION = 0.95
>> +P910ND_SITE =
>> http://voxel.dl.sourceforge.net/project/p910nd/p910nd/$(P910ND_VERSION)
>
>
> Use $(BR2_SOURCEFORGE_MIRROR) instead of a fixed mirror (voxel).
Also fixed to be standardized as all the other packages/*.mk files
that utilize SourceForge...
> Also, it looks like your patch is line-wrapped, so it won't apply cleanly.
> The easiest way to avoid this is to use git send-email (see the man page
> on how to use it with gmail).
Addressed above...
>> +P910ND_SOURCE = p910nd-$(P910ND_VERSION).tar.bz2
>> +
>> +define P910ND_BUILD_CMDS
>> + $(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D)
>
>
> Nowadays, we usually use
>
> $(MAKE) $(TARGET_CONFIGURE_OPTS) $(P910ND_MAKE_OPTS) -C $(@D)
>
> where P910ND_MAKE_OPTS are the extra make flags, if any (in this
> case none).
Yes, addressed as well. Thanks.
>> +endef
>> +
>> +define P910ND_INSTALL_TARGET_CMDS
>> + $(INSTALL) -D -m 0755 $(@D)/p910nd $(TARGET_DIR)/usr/sbin/p910nd
>> + $(INSTALL) -d -m 0755 $(TARGET_DIR)/etc/default
>
>
> Creating the directory is done automatically by the -D option used below.
Unneeded, since p910nd.conf will live in /etc
>> + $(INSTALL) -m 0755 -D package/p910nd/p910nd.default
>> $(TARGET_DIR)/etc/default/p910nd
>
>
> This one shouldn't be executable (mode 0644 instead of 0755).
Fixed.
>> + $(INSTALL) -m 0755 -D package/p910nd/S55p910nd
>> $(TARGET_DIR)/etc/init.d/S55p910nd
>> +
>> +endef
>> +
>> +$(eval $(call GENTARGETS,package,p910nd))
>
>
> The extra arguments for GENTARGETS are unnecessary.
Yes, you're right.
> Regards,
> Arnout
>
> --
> Arnout Vandecappelle arnout at mind be
> Senior Embedded Software Architect +32-16-286540
> Essensium/Mind http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
Overall, this actually shortened and cleaned it up a lot, both for the
package files, and when installed, as well.
Will send a [PATCH V2] within a few days.
regards, thanks,
Dave Purdy
More information about the buildroot
mailing list