Re: [PATCH] gnu: Add hdf-eos5.

[Ludovic Courtès]

Hi Thomas,

Thomas Danckaert <address@hidden> skribis:

> From f1a3bc9dcfbe9091110aacf353d8224a88788292 Mon Sep 17 00:00:00 2001
> From: Thomas Danckaert <address@hidden>
> Date: Fri, 17 Jun 2016 10:51:38 +0200
> Subject: [PATCH] gnu: Add hdf-eos5.
>
> * gnu/packages/maths.scm (hdf-eos5): New variable.
> * gnu/packages/patches/hdf-eos5-build-shared.patch: New file.
> * gnu/packages/patches/hdf-eos5-remove-gctp.patch: New file.
> * gnu/packages/patches/hdf-eos5-fix-szip.patch: New file.

Thanks for this patch!  Overall it looks pretty good to me; some
comments/suggestions follow.

First, please make sure to list the patches in gnu/local.mk.

Also, for each patch, could you add a word stating what the upstream
status is, such as the URL of the upstream commit or discussion, when it
exists?

> --- a/gnu/packages/maths.scm
> +++ b/gnu/packages/maths.scm

Please add a copyright line for you.

> +    (native-inputs
> +     `(("autoconf" ,autoconf)
> +       ("automake" ,automake)
> +       ("libtool" ,libtool)))
> +    (build-system gnu-build-system)
> +    (inputs
> +     `(("hdf5" ,hdf5)
> +       ("zlib" ,zlib)
> +       ("gctp" ,gctp)))
> +    (arguments
> +     `(#:configure-flags '("--enable-install-include")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before
> +             'configure 'autogen
> +           (lambda _  ; configure.ac was patched
> +             (and (chmod "./configure" #o755)
> +                  (zero? (system* "autoreconf" "--install"))))))

I wonder if we could avoid running autoreconf, which would mean patching
configure/Makefile.in instead of configure.ac/Makefile.am.

> +    (synopsis
> +     "HDF-EOS5: HDF5-based data format for NASA's Earth Observing System")

Remove “HDF-EOS5:” here.

> +++ b/gnu/packages/patches/hdf-eos5-build-shared.patch
> @@ -0,0 +1,88 @@
> +Allow shared build.
> +
> +We remove all references to the 'testdrivers' directory.  This directory is
> +not included in the distributed source archive, and its absence trips up
> +automake.
> +
> +---
> + Makefile.am             |  6 ------
> + configure.ac            | 12 ------------
> + include/HE5_HdfEosDef.h |  1 +
> + src/Makefile.am         |  3 ++-
> + 4 files changed, 3 insertions(+), 19 deletions(-)
> +
> +diff --git a/Makefile.am b/Makefile.am
> +index 363bcfb..01ed024 100644
> +--- a/Makefile.am
> ++++ b/Makefile.am
> +@@ -3,13 +3,7 @@
> + # Include boilerplate
> + include $(top_srcdir)/config/include.am
> + 
> +-# List of subdirectories.
> +-# Only build the testdrivers directory if configure detected that it's 
> present.
> +-if TESTDRIVERS_CONDITIONAL
> +-TESTDRIVERS=testdrivers
> +-else
> + TESTDRIVERS=
> +-endif

What about patching ‘configure’ such that the shell variable
‘TESTDRIVERS_DIR’ is set to “no”, and such that the
‘TESTDRIVERS_CONDITIONAL’ Automake condition is false?

That should be doable and would avoid this relatively length patch.

> +-# Disable shared libraries (for now)
> +-AC_DISABLE_SHARED

This macro only changes the default behavior.  Passing “--enable-shared”
to #:configure-flags should be enough to build shared libraries.

Could you check if that works?

> --- /dev/null
> +++ b/gnu/packages/patches/hdf-eos5-headers.patch
> @@ -0,0 +1,14 @@
> +Do not install unnecessary headers.
> +
> +diff --git a/include/Makefile.am b/include/Makefile.am
> +index 3de93db..52246af 100755
> +--- a/include/Makefile.am
> ++++ b/include/Makefile.am
> +@@ -4,6 +4,5 @@
> + include $(top_srcdir)/config/include.am
> + 
> + # Headers to install
> +-include_HEADERS = HE5_GctpFunc.h HE5_HdfEosDef.h HE5_config.h cproj.h 
> ease.h \
> +-                  isin.h proj.h tutils.h cfortHdf.h
> ++include_HEADERS = HE5_GctpFunc.h HE5_HdfEosDef.h ease.h isin.h cfortHdf.h

What about removing the extra files in a separate phase?  That would
avoid the need to modify Makefile.am.

Also please add a comment on why you think it’s unnecessary.  :-)

> diff --git a/gnu/packages/patches/hdf-eos5-remove-gctp.patch 
> b/gnu/packages/patches/hdf-eos5-remove-gctp.patch
> new file mode 100644
> index 0000000..359d486
> --- /dev/null
> +++ b/gnu/packages/patches/hdf-eos5-remove-gctp.patch
> @@ -0,0 +1,61 @@
> +Do not build/install the bundled GCTP and link shared -lgctp.
> +
> +---
> + Makefile.am         | 2 +-
> + config/include.am   | 1 -
> + configure.ac        | 3 ---
> + include/Makefile.am | 3 +--
> + src/Makefile.am     | 2 +-
> + 5 files changed, 3 insertions(+), 8 deletions(-)
> +
> +diff --git a/Makefile.am b/Makefile.am
> +index 01ed024..ef325ac 100644
> +--- a/Makefile.am
> ++++ b/Makefile.am
> +@@ -11,5 +11,5 @@ else
> + INCLUDE=
> + endif
> + 
> +-SUBDIRS=gctp src $(INCLUDE) samples $(TESTDRIVERS)
> ++SUBDIRS=src $(INCLUDE) samples $(TESTDRIVERS)

I think this can be achieved by removing it from Makefile.in with
‘substitute*’.  The LDFLAGS below can be passed like this:

  #:configure-flags (list … "LDFLAGS=-Wl,--as-needed -lgctp")

Could you send an updated patch?

Thank you!

Ludo’.