Patchwork e2tools: new package

login
register
mail settings
Submitter Daniel Nyström
Date Nov. 28, 2013, 6:02 p.m.
Message ID <1385661764-14355-1-git-send-email-daniel.nystrom@timeterminal.se>
Download mbox | patch
Permalink /patch/294990/
State Accepted
Headers show

Comments

Daniel Nyström - Nov. 28, 2013, 6:02 p.m.
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
Yann E. MORIN - Nov. 28, 2013, 6:25 p.m.
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.
Thomas Petazzoni - Nov. 28, 2013, 6:28 p.m.
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
Thomas Petazzoni - April 20, 2014, 9:49 p.m.
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

Patch

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))