Hi Thomas,
Thomas Danckaert <thomas.dancka...@gmail.com> skribis:
> From f1a3bc9dcfbe9091110aacf353d8224a88788292 Mon Sep 17 00:00:00 2001
> From: Thomas Danckaert <thomas.dancka...@gmail.com>
> 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’.
