Message ID | 1385661764-14355-1-git-send-email-daniel.nystrom@timeterminal.se |
---|---|
State | Accepted |
Headers | show |
Daniel, All, On 2013-11-28 19:02 +0100, Daniel Nyström spake thusly: > E2tools is a simple set of GPL'ed utilities to read, write, and > manipulate files in an ext2/ext3 filesystem. These utilities access a > filesystem directly using the ext2fs library. > > Signed-off-by: Daniel Nyström <daniel.nystrom@timeterminal.se> > --- > package/Config.in | 1 + > package/e2tools/Config.in | 10 ++++++++++ > package/e2tools/e2tools.mk | 16 ++++++++++++++++ > 3 files changed, 27 insertions(+) > create mode 100644 package/e2tools/Config.in > create mode 100644 package/e2tools/e2tools.mk > > diff --git a/package/Config.in b/package/Config.in > index f9e72ad..96e729a 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -227,6 +227,7 @@ source "package/cramfs/Config.in" > source "package/curlftpfs/Config.in" > source "package/dosfstools/Config.in" > source "package/e2fsprogs/Config.in" > +source "package/e2tools/Config.in" > source "package/ecryptfs-utils/Config.in" > source "package/exfat/Config.in" > source "package/exfat-utils/Config.in" > diff --git a/package/e2tools/Config.in b/package/e2tools/Config.in > new file mode 100644 > index 0000000..14f25ab > --- /dev/null > +++ b/package/e2tools/Config.in > @@ -0,0 +1,10 @@ > +config BR2_PACKAGE_E2TOOLS > + bool "e2tools" Since it needs threads, you should add: depends on BR2_TOOLCHAIN_HAS_THREADS > + select BR2_PACKAGE_E2FSPROGS > + help > + E2tools is a simple set of GPL'ed utilities to read, write, > + and manipulate files in an ext2/ext3 filesystem. These > + utilities access a filesystem directly using the ext2fs > + library. > + > + https://github.com/ndim/e2tools [...] and here add a comment: comment "e2tools needs a toolchain w/ threads" depends on !BR2_TOOLCHAIN_HAS_THREADS > diff --git a/package/e2tools/e2tools.mk b/package/e2tools/e2tools.mk > new file mode 100644 > index 0000000..56797bb > --- /dev/null > +++ b/package/e2tools/e2tools.mk > @@ -0,0 +1,16 @@ > +################################################################################ > +# > +# e2tools > +# > +################################################################################ > + > +E2TOOLS_VERSION = 3158ef18 Please use the full commit hash, not just the shortened hash. > +E2TOOLS_SITE = http://github.com/ndim/e2tools/tarball/$(E2TOOLS_VERSION) > +E2TOOLS_AUTORECONF = YES Explain why it needs to be autoreconfed (eg. ./configure is not provided). > +E2TOOLS_LICENSE = GPLv2 > +E2TOOLS_LICENSE_FILES = COPYING > +E2TOOLS_DEPENDENCIES = e2fsprogs > +E2TOOLS_CONF_ENV = LIBS="-lpthread" > +E2TOOLS_INSTALL_TARGET_OPT = DESTDIR=$(TARGET_DIR) install-exec > + > +$(eval $(autotools-package)) As discussed on IRC, please also make it a host package: $(eval $(host-autotools-package)) And add it to package/Config.in.host (look at package/dosfstools for how it is done there). Regards, Yann E. MORIN.
Dear Daniel Nyström, Thanks for this contribution! On Thu, 28 Nov 2013 19:02:44 +0100, Daniel Nyström wrote: > diff --git a/package/e2tools/Config.in b/package/e2tools/Config.in > new file mode 100644 > index 0000000..14f25ab > --- /dev/null > +++ b/package/e2tools/Config.in > @@ -0,0 +1,10 @@ > +config BR2_PACKAGE_E2TOOLS > + bool "e2tools" > + select BR2_PACKAGE_E2FSPROGS You need to propagate the e2fsprogs dependency here. > + help > + E2tools is a simple set of GPL'ed utilities to read, write, > + and manipulate files in an ext2/ext3 filesystem. These > + utilities access a filesystem directly using the ext2fs > + library. > + > + https://github.com/ndim/e2tools and add a comment here. > diff --git a/package/e2tools/e2tools.mk b/package/e2tools/e2tools.mk > new file mode 100644 > index 0000000..56797bb > --- /dev/null > +++ b/package/e2tools/e2tools.mk > @@ -0,0 +1,16 @@ > +################################################################################ > +# > +# e2tools > +# > +################################################################################ > + > +E2TOOLS_VERSION = 3158ef18 > +E2TOOLS_SITE = http://github.com/ndim/e2tools/tarball/$(E2TOOLS_VERSION) > +E2TOOLS_AUTORECONF = YES > +E2TOOLS_LICENSE = GPLv2 The code isn't really clear as to whether it is GPLv2 or GPLv2+. The COPYING file is the text of GPLv2, but the comment headers in the code don't explicit whether it's GPLv2 only, or GPLv2+. I guess in a case like this, we should assume it's GPLv2, as you did. > +E2TOOLS_LICENSE_FILES = COPYING > +E2TOOLS_DEPENDENCIES = e2fsprogs > +E2TOOLS_CONF_ENV = LIBS="-lpthread" So you need a dependency on BR2_TOOLCHAIN_HAS_THREADS as well. > +E2TOOLS_INSTALL_TARGET_OPT = DESTDIR=$(TARGET_DIR) install-exec I know (from IRC discussion) you used this instead of the default "make install" to not install man pages. But that's not needed: provided BR2_HAVE_DOCUMENTATION is disabled (which is almost always the case since this option is deprecated), then Buildroot automatically removes man pages. So unless using "make install" causes a problem, we'd prefer to not use a specific <pkg>_INSTALL_TARGET_OPT, and let Buildroot remove the man pages. Best regards, Thomas
Dear Daniel Nyström, On Thu, 28 Nov 2013 19:02:44 +0100, Daniel Nyström wrote: > E2tools is a simple set of GPL'ed utilities to read, write, and > manipulate files in an ext2/ext3 filesystem. These utilities access a > filesystem directly using the ext2fs library. > > Signed-off-by: Daniel Nyström <daniel.nystrom@timeterminal.se> > --- > package/Config.in | 1 + > package/e2tools/Config.in | 10 ++++++++++ > package/e2tools/e2tools.mk | 16 ++++++++++++++++ > 3 files changed, 27 insertions(+) > create mode 100644 package/e2tools/Config.in > create mode 100644 package/e2tools/e2tools.mk Thanks, I've applied your patch, after taking into account the comments made during the review: propagation of toolchain dependencies, usage of a full Git hash, usage of the github helper, addition of the host variant. Thanks! Thomas
diff --git a/package/Config.in b/package/Config.in index f9e72ad..96e729a 100644 --- a/package/Config.in +++ b/package/Config.in @@ -227,6 +227,7 @@ source "package/cramfs/Config.in" source "package/curlftpfs/Config.in" source "package/dosfstools/Config.in" source "package/e2fsprogs/Config.in" +source "package/e2tools/Config.in" source "package/ecryptfs-utils/Config.in" source "package/exfat/Config.in" source "package/exfat-utils/Config.in" diff --git a/package/e2tools/Config.in b/package/e2tools/Config.in new file mode 100644 index 0000000..14f25ab --- /dev/null +++ b/package/e2tools/Config.in @@ -0,0 +1,10 @@ +config BR2_PACKAGE_E2TOOLS + bool "e2tools" + select BR2_PACKAGE_E2FSPROGS + help + E2tools is a simple set of GPL'ed utilities to read, write, + and manipulate files in an ext2/ext3 filesystem. These + utilities access a filesystem directly using the ext2fs + library. + + https://github.com/ndim/e2tools diff --git a/package/e2tools/e2tools.mk b/package/e2tools/e2tools.mk new file mode 100644 index 0000000..56797bb --- /dev/null +++ b/package/e2tools/e2tools.mk @@ -0,0 +1,16 @@ +################################################################################ +# +# e2tools +# +################################################################################ + +E2TOOLS_VERSION = 3158ef18 +E2TOOLS_SITE = http://github.com/ndim/e2tools/tarball/$(E2TOOLS_VERSION) +E2TOOLS_AUTORECONF = YES +E2TOOLS_LICENSE = GPLv2 +E2TOOLS_LICENSE_FILES = COPYING +E2TOOLS_DEPENDENCIES = e2fsprogs +E2TOOLS_CONF_ENV = LIBS="-lpthread" +E2TOOLS_INSTALL_TARGET_OPT = DESTDIR=$(TARGET_DIR) install-exec + +$(eval $(autotools-package))
E2tools is a simple set of GPL'ed utilities to read, write, and manipulate files in an ext2/ext3 filesystem. These utilities access a filesystem directly using the ext2fs library. Signed-off-by: Daniel Nyström <daniel.nystrom@timeterminal.se> --- package/Config.in | 1 + package/e2tools/Config.in | 10 ++++++++++ package/e2tools/e2tools.mk | 16 ++++++++++++++++ 3 files changed, 27 insertions(+) create mode 100644 package/e2tools/Config.in create mode 100644 package/e2tools/e2tools.mk