| Message ID | 1444796918-22912-1-git-send-email-chaduffy@cisco.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
diff --git a/package/Config.in b/package/Config.in index 8e3c64a..32affcc 100644 --- a/package/Config.in +++ b/package/Config.in @@ -1496,6 +1496,7 @@ endif source "package/ncdu/Config.in" source "package/numactl/Config.in" source "package/nut/Config.in" + source "package/nwipe/Config.in" source "package/openvmtools/Config.in" source "package/polkit/Config.in" source "package/powerpc-utils/Config.in" 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 + 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 +NWIPE_DEPENDENCIES = ncurses parted +NWIPE_CONF_OPTS = --bindir=/bin +NWIPE_LICENSE = GPL +NWIPE_LICENSE_FILES = LICENCE + +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; } + cd $(@D) && PATH=$(BR_PATH) ./init.sh +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' -- '{}' + +endef +NWIPE_PRE_CONFIGURE_HOOKS += NWIPE_USE_OFF64T + +$(eval $(autotools-package))