[Buildroot] [PATCH] netkitftp: added package with Makefile.autotools.in support
Hugues
hugues at hiegel.fr
Mon Oct 26 21:15:37 UTC 2009
Hello,
Thanks for the comments,
I've got some questions ;
Ce cher Peter Korsgaard <jacmet at uclibc.org> a dit :
>>>>>> "Hugues" == Hugues Hiegel <hugues at hiegel.fr> writes:
>
> Hi,
>
> Thanks for the patch - A few comments:
>
> Hugues> diff --git a/package/netkitftp/Config.in b/package/netkitftp/Config.in
> Hugues> new file mode 100644
> Hugues> index 0000000..9b37d6e
> Hugues> --- /dev/null
> Hugues> +++ b/package/netkitftp/Config.in
> Hugues> @@ -0,0 +1,9 @@
> Hugues> +config BR2_PACKAGE_NETKITFTP
> Hugues> + bool "netkitftp"
> Hugues> + default n
>
> 'n' is default, so no need to explictly list that.
Ok.
> Hugues> + help
> Hugues> + ftp client.
> Hugues> + ftp client version 0.17-16 with debian patch.
> Hugues> + ftp://ftp.debian.org/debian/pool/main/netkit-ftp/
>
> That's not a good help text. We generally don't list version numbers in
> the help text as they tend to get out of sync with the .mk files fast.
Yup, I just translated the french text that was there just before, I
didn't thought about this way :)
I'll correct this.
> Hugues> +
> Hugues> + ftp://ftp.uk.linux.org/pub/linux/Networking/netkit/
> Hugues> diff --git a/package/netkitftp/netkitftp-configure-noexit.patch b/package/netkitftp/netkitftp-configure-noexit.patch
> Hugues> new file mode 100644
> Hugues> index 0000000..f588a46
> Hugues> --- /dev/null
> Hugues> +++ b/package/netkitftp/netkitftp-configure-noexit.patch
> Hugues> @@ -0,0 +1,13 @@
> Hugues> +diff --git a/configure b/configure
> Hugues> +index 0440527..54a5278 100755
> Hugues> +--- a/configure
> Hugues> ++++ b/configure
> Hugues> +@@ -39,7 +39,7 @@ EOF
> Hugues> + --manmode=*) MANMODE=`echo $1 | sed 's/^[^=]*=//'` ;;
> Hugues> + --with-c-compiler=*) CC=`echo $1 | sed 's/^[^=]*=//'` ;;
> Hugues> + --without-readline|--disable-readline) WITHOUT_READLINE=1;;
> Hugues> +- *) echo "Unrecognized option: $1"; exit 1;;
> Hugues> ++ *) echo "Unrecognized option: $1";;
>
> Please document patches. Why is this needed, and is this something that
> should get submitted upstream?
Good remark. I'll correct this asap.
> Hugues> diff --git a/package/netkitftp/netkitftp-configure.patch b/package/netkitftp/netkitftp-configure.patch
>
> That one as well.
Well... This one was already packaged with the package I've got, I don't
have any idea why it is needed. I'll check this and then verify if it is
really needed (hope it is not :-) )
> Hugues> +++ b/package/netkitftp/netkitftp.mk
> Hugues> @@ -0,0 +1,23 @@
> Hugues> +#############################################################
> Hugues> +#
> Hugues> +# netkitftp
> Hugues> +#
> Hugues> +#############################################################
> Hugues> +NETKITFTP_VERSION:=0.17
> Hugues> +NETKITFTP_SOURCE:=netkit-ftp_$(NETKITFTP_VERSION).orig.tar.gz
> Hugues> +NETKITFTP_PATCH:=netkit-ftp_0.17-16.diff.gz
> Hugues> +NETKITFTP_SITE:=ftp://ftp.debian.org/debian/pool/main/n/netkit-ftp/
>
> Is that a stable URL? Would something on snapshot.debian.net be better?
> We have BR2_DEBIAN_MIRROR, which should be used.
Oh, that's right !
I'll correct this.
> Hugues> +
> Hugues> +NETKITFTP_CONF_OPT = --installroot=$(TARGET_DIR) \
>
> Something regarding TARGET_DIR at configure time? Doesn't that leak
> paths on the build host to the installed binaries?
Well, I didn't investigated further, maybe is this not needed. I'll try
without and throw then this out, if not.
Or comment it, if needed :o)
> Hugues> + --without-readline
> Hugues> +NETKITFTP_MAKE_ENV = SUB=ftp
>
> A comment to why would be good.
Ok.
> Hugues> +NETKITFTP_INSTALL_TARGET_OPT = INSTALL_PREFIX=$(TARGET_DIR) install
> Hugues> +
> Hugues> +$(eval $(call AUTOTARGETS,package,netkitftp))
> Hugues> +
> Hugues> +$(NETKITFTP_HOOK_POST_BUILD):
> Hugues> + [ ! -d $(TARGET_DIR)/usr/man/man1 ] && install -d $(TARGET_DIR)/usr/man/man1 || true
> Hugues> + [ ! -d $(TARGET_DIR)/usr/man/man5 ] && install -d $(TARGET_DIR)/usr/man/man5 || true
>
> You have _true at the man5 line.
How, sh*t. These funky french keyboards on early testing Debian put
this awkward unbreakable space while pressing AltGt+Space... I didn't
saw this earlier. (Seems to have been corrected since, at least on my
personal desktop at home, which is more fresh than the one I've got at
work, it is.)
> Simply doing mkdir -p
> $(TARGET_DIR)/usr/man/man1 $(TARGET_DIR)/usr/man/man5 would be simpler
> to read.
Great remark.
Anyway, I don't know why I did this way.... O_o
;)
> Also don't forget to touch $@ so the target isn't always considered out
> of date.
Ok. I didn't thought it was necessary at this state, but if you suggest
it.. ;-)
> A comment to why the above is needed in the first place would also be
> good.
Ok
> Hugues> +
> Hugues> +$(NETKITFTP_HOOK_POST_INSTALL):
> Hugues> + rm -vfr $(TARGET_DIR)/usr/man
>
> Why verbose?
Mhmm.. because with the "fr", it makes "vfr", which is a motorbike.. ;o)
I'll throw it ;)
> That uninstall would also remove all other man pages, so
> don't do that.
Mhmm... I'm not sure I understood this.. Why do you talk about uninstall ?
The netkitftp's 'install' rule does installs the manpages with the
binary, without ability to call just the install of the binary without
the manpages.
So I did like I see in other packages : just delete the
$(TARGET_DIR)/usr/man folder... Is this a bad idea ?
What do you suggest at this stage ?
Doing nothing, this will be cleaned later ?
With best regards,
--
Hugues Hiegel [http://www.hiegel.fr/~hugues/]
More information about the buildroot
mailing list