[Buildroot] [PATCH 1/1] introduce nwipe package
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Wed Oct 14 21:43:21 UTC 2015
Dear Charles Duffy,
Thanks for your contribution. See some comments below.
On Tue, 13 Oct 2015 23:28:38 -0500, Charles Duffy wrote:
> diff --git a/package/nwipe/Config.in b/package/nwipe/Config.in
> new file mode 100644
> index 0000000..0cecd25
> --- /dev/null
> +++ b/package/nwipe/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_NWIPE
> + bool "nwipe"
> + depends on BR2_USE_MMU # fork()
> + select BR2_PACKAGE_NCURSES
You also need to select parted here if you depend on it, as suggested
by your NWIPE_DEPENDENCIES variable below.
> + help
> + nwipe thoroughly overwrites block devices, forked from a component at
> + the core of the venerable DBAN.
> +
> + https://github.com/martijnvanbrummelen/nwipe
> diff --git a/package/nwipe/nwipe.mk b/package/nwipe/nwipe.mk
> new file mode 100644
> index 0000000..18efb22
> --- /dev/null
> +++ b/package/nwipe/nwipe.mk
> @@ -0,0 +1,27 @@
> +################################################################################
> +#
> +# nwipe
> +#
> +################################################################################
> +
> +NWIPE_VERSION = 0.17
> +NWIPE_SITE = https://github.com/martijnvanbrummelen/nwipe
> +NWIPE_SITE_METHOD = git
Use the github helper function instead. This way, you can remove the
NWIPE_SITE_METHOD variable and simply do:
NWIPE_SITE = $(call github,martijnvanbrummelen,nwipe,$(NWIPE_VERSION))
> +NWIPE_DEPENDENCIES = ncurses parted
> +NWIPE_CONF_OPTS = --bindir=/bin
Why do you need to override --bindir ?
> +NWIPE_LICENSE = GPL
You should specify the GPL version. In the case of this program, it's
GPLv2.
> +NWIPE_LICENSE_FILES = LICENCE
There is no file named 'LICENCE' in the repository. Use COPYING instead.
> +
> +define NWIPE_RUN_AUTOGEN
> + cd $(@D) && { printf '%s\n' '44a' 'AC_CHECK_LIB([intl], [libintl_dgettext]) # needed by static builds of libparted' 'AC_CHECK_LIB([uuid], [uuid_generate]) # needed by static builds of libparted' '.' 'w' | edit configure.ac; }
Ouch, this is not pretty. What are you trying to do here exactly ?
Could you instead do a patch ?
> + cd $(@D) && PATH=$(BR_PATH) ./init.sh
Please use:
NWIPE_AUTORECONF = YES
instead. Your solution doesn't work because you do not have the
guarantee that autoconf/automake/libtool will be available, while
<pkg>_AUTORECONF = YES ensures that all the necessary dependencies
will be available.
> +endef
> +NWIPE_PRE_CONFIGURE_HOOKS += NWIPE_RUN_AUTOGEN
> +
> +# to fix support for MUSL
> +define NWIPE_USE_OFF64T
> + cd $(@D) && find . -type f -name '*.[ch]' -exec sed -i -r -e 's/loff_t/off64_t/g' -- '{}' +
Same, could you do a patch instead, and submit it upstream? github
makes it very easy to submit pull requests to the projects. I see you
already submitted an issue
(https://github.com/martijnvanbrummelen/nwipe/issues/11), but maybe a
pull request with a patch would help?
Regarding https://github.com/martijnvanbrummelen/nwipe/issues/12 (i.e
missing intl and uuid), I think the proper solution is not your patch,
but instead to use pkg-config to detect parted. I.e, replace:
AC_CHECK_LIB([parted], [ped_device_probe_all], ,[AC_MSG_ERROR([parted development library not found])])
by something like:
PKG_CHECK_MODULES([PARTED], [libparted])
Also, I see that the configure.ac already uses PKG_CHECK_MODULES() for
other libraries, so you should add host-pkgconf in NWIPE_DEPENDENCIES.
Finally, please add a nwipe.hash file that contains the sha256 hash of
the downloaded tarball (yes, when you use the github helper function as
suggested above, it's really a tarball that gets downloaded over HTTP,
and not a Git clone that gets done).
Could you submit an updated version of your patch that takes into
account those comments?
Thanks a lot!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list