Re: [PATCH] Add gctp

[Thomas Danckaert <thomas.danckaert@gmail.com>]

From: Leo Famulari <leo@famulari.name>
Subject: Re: [PATCH] Add gctp
Date: Fri, 17 Jun 2016 21:39:26 -0400

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

The only other use I'm aware of is in HDF-EOS2, which is a separate 
library from HDF-EOS5, built on HDF4 instead of HDF5, and which also 
bundles gctp. I intend to package HDF-EOS2 as well, once HDF4 is 
included.

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

That is my impression, too (broken urls and undeliverable e-mails). 
The package is quite small anyway, so perhaps bundling with the 2 
HDF-EOS libraries is acceptable?

> Here are some comments:

[ Style issues noted, I will take care of that. ]

>> +(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.

The archive does not contain an explicit version number or changelog 
(it just says it's the “new C version of the GCTP” -- before that, it 
seems there were some Fortran routines).  I've also found a gctpc2.0 
archive, which *does* have a changelog, and on closer inspection 
(comparing the source of this package with comments from the 
changelog from 2.0), it seems that this code corresponds to version 
1.3... (though e.g. Debian also calls it 1.0).  It's quite messy 
actually. I'll see if HDF-EOS5 builds against gctp-2.0 (for which a 
I've found a cleaner archive), and maybe package that instead...

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

That's the reason indeed. I wasn't aware of the chmod procedure, I'll 
adapt my package definition .

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

I'll resubmit an improved patch.

Before I do that, though, I'll await your opinion on whether an 
independent gctp package is actually needed or not.

Some other explanations in-line:

>> +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?
>

About this and the other “TESTDRIVERS”-related stuff:
When we run automake after patching the build, it needs the 
“testdrivers” directory.  This is a set of additional 
tests/demonstration programs (HDF-EOS5 already contains tests in the 
src/samples directory), which are distributed in a separate tarball 
(the configure script will test if the testdrivers are there or not, 
so they are optional in a “standard” build scenario). In order to 
allow automake to complete, I removed all references to the 
testdrivers in my patch.  The alternative solution would be to 
download 2 tarballs (source + testdrivers) and extract them in the 
build directory. Removing the testdrivers was the easiest solution.

>> + 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?
>

Actually this only affects the HDF-EOS5 package, and I now realize it 
can be removed from this patchset.  I was using the same patch for 
shared library compilation in my HDF5 and GCTP packages... lazyness.

(The HDF5 API changed in version 1.8. Code written against previous 
version (such as HDF-EOS5) needs this #define to link with newer 
versions of HDF5.)

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

Otherwise the library is installed as “libGctp.so.0”, which seems 
like an unconventional capitalization (?). Like this, users switching 
from Debian & Co. will have a seamless experience ;-)

Thomas