Re: [PATCH] Add gctp

[Leo Famulari <leo@famulari.name>]

On Fri, Jun 17, 2016 at 02:40:17PM +0200, Thomas Danckaert wrote:
> this is a patch to add the GCTP library. It seems the library is no longer
> maintained separately (“official” sources are gone), so this package
> downloads the HDF-EOS5 source, which contains GCTP, and patches the build to
> only build and install GCTP. (I'm packaging HDF-EOS5 later on)

Should we package GCTP separately in that case? Is it used by anything
besides HDF-EOS5? Or, should we just package HDF-EOS5?

We usually don't accept bundled code, but it sounds like GCTP no longer
exists as an independent project. Is that right?

Otherwise, it looks like we are forking GCTP and creating a new
distribution.

> It's not very elegant... An alternative solution would be to get the source
> package from, e.g. Debian, and build that (might involve less patching).

We do fetch a few things from Debian.

Here are some comments:

> Subject: [PATCH] gnu: Add gctp

We prefer to use complete sentences, with punctuation, even when they
are only 2 words long ;)

> * gnu/packages/maths.cm (gctp): New variable
> 
> * guix/packages.scm (decompression-type): Modify by adding ".Z" suffix
>   for gzipped files.

I believe this was addressed in commit 5257ab6de. Can you check?

> * gnu/packages/patches/gct-build-shared.patch: patch build system for
>   shared builds.

We also capitalize the beginning of our sentences :)

> * gnu/packages/patches/gctp-remove-hdf-eos5.patch: don't build the
>   hdf-eos5 library contained in the same archive.
> 
> * gnu/packages/patches/gctp-fix-soname.patch: use a lower case soname.
> ---
>  gnu/packages/maths.scm                          |  39 ++
>  gnu/packages/patches/gctp-build-shared.patch    |  84 +++++
>  gnu/packages/patches/gctp-fix-soname.patch      |  31 ++
>  gnu/packages/patches/gctp-remove-hdf-eos5.patch | 482 ++++++++++++++++++++++++
>  4 files changed, 636 insertions(+)
>  create mode 100644 gnu/packages/patches/gctp-build-shared.patch
>  create mode 100644 gnu/packages/patches/gctp-fix-soname.patch
>  create mode 100644 gnu/packages/patches/gctp-remove-hdf-eos5.patch
> 

> diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
> index 3b860a9..9cd0554 100644
> --- a/gnu/packages/maths.scm
> +++ b/gnu/packages/maths.scm
> @@ -42,6 +42,8 @@
>    #:use-module (guix build-system gnu)
>    #:use-module (guix build-system r)
>    #:use-module (gnu packages algebra)
> +  #:use-module (gnu packages autotools)
> +  #:use-module (gnu packages base)
>    #:use-module (gnu packages bison)
>    #:use-module (gnu packages boost)
>    #:use-module (gnu packages check)
> @@ -408,6 +410,43 @@ plotting engine by third-party applications like Octave.")
>      (license (license:fsf-free
>                "http://gnuplot.cvs.sourceforge.net/gnuplot/gnuplot/Copyright"))))
>  
> +(define-public gctp
> +  (package
> +    (name "gctp")
> +    (version "1.0")

Is this the upstream version or is it arbitrary? I see that the HDF-EOS5
tarball is at version 1.15.

> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "ftp://edhs1.gsfc.nasa.gov/edhs/hdfeos5/latest_release/HDF-EOS5.1.15.tar.Z"))
> +              (sha256
> +               (base32
> +                "1p83333nzzy8rn5chxlm0hrkjjnhh2w1ji8ac0f9q4xzg838i58i"))
> +              (patches (search-patches "gctp-build-shared.patch" "gctp-remove-hdf-eos5.patch" "gctp-fix-soname.patch"))
> +              ))

Some of these lines are too long. We like to keep them shorter than 80
characters whenever possible.

> +    (native-inputs
> +     `(("autoconf" ,autoconf)
> +       ("automake" ,automake)
> +       ("libtool" ,libtool)
> +       ("coreutils" ,coreutils)))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:configure-flags '("--enable-install-include")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before
> +             'configure 'autogen
> +           (lambda _
> +             (and (zero? (system* "chmod" "-R" "u+w" ".")) ; archive is read-only...

We have a Scheme procedure for chmod. There are examples in
'gnu/packages'. Is this what required coreutils as a native-input?

> diff --git a/gnu/packages/patches/gctp-build-shared.patch b/gnu/packages/patches/gctp-build-shared.patch

In general, I think these patches need some more comments and
explanation of the various changes.

> new file mode 100644
> index 0000000..1139fbb
> --- /dev/null
> +++ b/gnu/packages/patches/gctp-build-shared.patch
> @@ -0,0 +1,84 @@
> +Allow shared build.
> +
> +---
> + 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 is the effect of this?

> + 
> + if INSTALL_INCLUDE_CONDITIONAL
> + INCLUDE=include

> +diff --git a/configure.ac b/configure.ac
> +index 5d48b43..cfa9d4e 100644
> +--- a/configure.ac
> ++++ b/configure.ac
> +@@ -13,9 +13,6 @@ AC_CONFIG_AUX_DIR([config])
> + AM_INIT_AUTOMAKE([foreign])
> + AM_MAINTAINER_MODE
> + 
> +-# Disable shared libraries (for now)
> +-AC_DISABLE_SHARED

Okay.

> +-
> + AM_PROG_LIBTOOL
> + 
> + dnl ----------------------------------------------------------------------
> +@@ -597,13 +594,4 @@ AC_CONFIG_FILES([Makefile
> +                  gctp/src/Makefile
> +                  samples/Makefile])
> + 
> +-if test "X$TESTDRIVERS_DIR" = "Xyes"; then
> +-  AC_CONFIG_FILES([ testdrivers/Makefile
> +-                 testdrivers/grid/Makefile
> +-                 testdrivers/point/Makefile
> +-                 testdrivers/swath/Makefile
> +-                 testdrivers/threads/Makefile
> +-                 testdrivers/za/Makefile])
> +-fi
> +-

It's related to an earlier diff, but can you explain?

> + AC_OUTPUT

> +diff --git a/include/HE5_HdfEosDef.h b/include/HE5_HdfEosDef.h
> +index 9ed7881..abf0a90 100755
> +--- a/include/HE5_HdfEosDef.h
> ++++ b/include/HE5_HdfEosDef.h
> +@@ -24,6 +24,7 @@
> + #ifndef HE5_HDFEOSDEF_H_
> + #define HE5_HDFEOSDEF_H_
> + 
> ++#define H5_USE_16_API 1

What is the significance of this definition?

> + #include <hdf5.h>
> + 
> + #ifdef H5_USE_16_API

> +diff --git a/src/Makefile.am b/src/Makefile.am
> +index 76b1d4c..daf7ad8 100644
> +--- a/src/Makefile.am
> ++++ b/src/Makefile.am
> +@@ -10,7 +10,8 @@ INCLUDES=-I$(top_srcdir)/include/
> + 
> + # Set LDFLAGS to allow the HDF-EOS library to use extern variables from
> + # HDF5
> +-LDFLAGS=-Wl,-single_module
> ++LDFLAGS+= -shrext .so
> ++LIBS+= -lhdf5_hl -lhdf5 -lm

Okay.

> + 
> + # Build HDF-EOS5
> + lib_LTLIBRARIES=libhe5_hdfeos.la
> +-- 
> +2.5.0
> +

> diff --git a/gnu/packages/patches/gctp-fix-soname.patch b/gnu/packages/patches/gctp-fix-soname.patch
> new file mode 100644
> index 0000000..5a32970
> --- /dev/null
> +++ b/gnu/packages/patches/gctp-fix-soname.patch
> @@ -0,0 +1,31 @@
> +Make library name all-lowercase.

Is this a stylistic change or does it have some other effect?

> +
> +---
> + gctp/src/Makefile.am | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +

> +diff --git a/gctp/src/Makefile.am b/gctp/src/Makefile.am
> +index 76ecf1c..97c2438 100755
> +--- a/gctp/src/Makefile.am
> ++++ b/gctp/src/Makefile.am
> +@@ -4,7 +4,7 @@
> + include $(top_srcdir)/config/include.am
> + 
> + # Library to build
> +-lib_LTLIBRARIES = libGctp.la
> ++lib_LTLIBRARIES = libgctp.la
> + 
> + ## Normally DEFAULT_INCLUDES is supplied by Automake, but one of the
> + ## directories included by default is $(top_builddir)/include, which
> +@@ -17,7 +17,7 @@ DEFAULT_INCLUDES = -I. -I$(srcdir)
> + INCLUDES=-I$(srcdir)/../include/
> + 
> + # Library source files
> +-libGctp_la_SOURCES = gctp.c alberfor.c alberinv.c alconfor.c alconinv.c      \
> ++libgctp_la_SOURCES = gctp.c alberfor.c alberinv.c alconfor.c alconinv.c      \
> +           azimfor.c aziminv.c bceafor.c bceainv.c br_gctp.c ceafor.c         \
> +           ceainv.c cproj.c eqconfor.c eqconinv.c equifor.c equiinv.c         \
> +           for_init.c gnomfor.c gnominv.c goodfor.c goodinv.c gvnspfor.c      \
> +-- 
> +2.5.0
> +

> diff --git a/gnu/packages/patches/gctp-remove-hdf-eos5.patch b/gnu/packages/patches/gctp-remove-hdf-eos5.patch
> new file mode 100644
> index 0000000..4601c5f
> --- /dev/null
> +++ b/gnu/packages/patches/gctp-remove-hdf-eos5.patch
> @@ -0,0 +1,482 @@
> +From c206a99fc4692ef9ea48a11bbb9de8a7dd44a7f8 Mon Sep 17 00:00:00 2001
> +From: Thomas Danckaert <thomas.danckaert@gmail.com>
> +Date: Fri, 17 Jun 2016 07:55:58 +0200
> +Subject: [PATCH 1/2] Build only GCTP.
> +
> +---
> + Makefile.am  |   2 +-
> + configure.ac | 430 +----------------------------------------------------------
> + 2 files changed, 2 insertions(+), 430 deletions(-)

430 deletions: If it works, then okay :)