Upgrade and clean up cddlib

[kini]

The cddlib SPKG is huge (when disregarding src/) and contains a lot of weird-looking stuff in the patched directory. The point of this ticket is to simplify the packaging of cddlib and also upgrade it to the latest version, 094g.

Part of metaticket #13025.

Use tarball at:

• ​http://boxen.math.washington.edu/home/jpflori/upstream/cddlib-094g.tar.bz2

[kcrisman]

This all makes sense. I've opened #13056 to address that we might eventually want to have the docs available again.

All spkg and geometry/ tests pass on sage.math and my MacBook Pro. As discussed with Andrew, just want to make sure that hard links were used as pointed out in SPKG.txt, which I can't check - otherwise positive review.

[ohanar]

Ok, hard links should be setup now.

[jdemeyer]

This doesn't even compile for me with gcc-4.2.4 on boxen.math:

****************************************************
Host system:
Linux boxen 2.6.24-24-server #1 SMP Fri Sep 18 16:47:05 UTC 2009 x86_64 GNU/Linux
****************************************************
C compiler: gcc
C compiler version:
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared --with-system-zl
ib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --with-gxx-include-dir=/usr/include/c++/4.2 --prog
ram-suffix=-4.2 --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-mpfr --enable-checking=release --build=x86_64-linu
x-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
****************************************************
patching file src/lib-src/cddcore.c
patching file src/lib-src/cddlp.c
patching file src/Makefile.in
patching file src/src-gmp/Makefile.in
patching file src/src/Makefile.in
patching file src/lib-src/Makefile.in
patching file src/lib-src-gmp/Makefile.in
configure: WARNING: unrecognized options: --with-gmp
[...]
libtool: compile:  gcc -DPACKAGE_NAME=\"\" -DPACKAGE_TARNAME=\"\" -DPACKAGE_VERSION=\"\" -DPACKAGE_STRING=\"\" -DPACKAGE_BUGREPORT=\"\" -D
PACKAGE=\"cddlib\" -DVERSION=\"0.94\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE
_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_LIBG
MP=1 -DSTDC_HEADERS=1 -I. -DGMPRATIONAL -g -O2 -MT cddio.lo -MD -MP -MF .deps/cddio.Tpo -c cddio.c  -fPIC -DPIC -o .libs/cddio.o
In file included from cdd.h:18,
                 from cddmp.c:21:
cddmp.h:31:18: error: gmp.h: No such file or directory
[...]

Note the line

configure: WARNING: unrecognized options: --with-gmp

[kcrisman]

Huh. I did test this on sage.math and OS X, but it's true that I didn't try with random other versions of GCC. I'm not sure what to say about that, and I can't review this one further.

[jdemeyer]

I don't think the GCC version has anything to do with it. Probably both machines you tested have a system-wide /usr/include/gmp.h but boxen.math doesn't.

[fbissey]

I can have a look at that. Tomorrow hopefully, but it looks like to me we just need to set CPPFLAGS="-I$SAGE_LOCAL/include" when we run the configuration, LDFLAGS may also need to be set.

[vbraun]

Support for the --with-gmp configure option was one of the things that I patched into cddlib. Yes, upstream does not have that. Yet another reason why IMHO you can't have a minimally invasive patch to make it work, just too broken.

[fbissey]

So it was some of your stuff. On the other hand upstream included a slightly updated version of the libtool patch I sent them probably about 3-4 years ago :)

In any case adding the following line to configure should solve the problem

CPPFLAGS="-I$SAGE_LOCAL/include" LDFLAGS="-L$SAGE_LOCAL/lib"

Alternatively the variables can be exported and it should be safe on any system. Your original spkg works on my mac so I could have gmp headers somewhere, strangely enough it seems to find the correct sage libraries so may be there is something more subtle going on

/bin/sh ../libtool --tag=CC   --mode=link gcc  -g -O2   -o cdd_both_reps_gmp cdd_both_reps.o ../lib-src-gmp/libcddgmp.la -lgmp 
libtool: link: gcc -g -O2 -o .libs/cdd_both_reps_gmp cdd_both_reps.o  ../lib-src-gmp/.libs/libcddgmp.dylib /Users/frb15/Desktop/sage-5.1.beta1/local/lib/libgmp.dylib

[ohanar]

Ok, I've updated the spkg, hopefully the issues are fixed (it now compiles fine for me with boxen's toolchain).

[jdemeyer]

Diff for the cddlib spkg. For reference / review only.

[jdemeyer]

Diff for the cddlib spkg without deletes. For reference / review only.

[jdemeyer]

Your changes could run into timestamp troubles, when the timestamp of the Makefile.am file is more recent than the Makefile.in file. This might cause automake to be run and should be avoided. Since "am" comes alphabetically before "in", you will not actually run into this problem, but it seems fragile to rely on that.

A standard solution that I have used for several packages is to create 1 patch file which patches both the .am and the .in file in that order. patch applies patches in order of the patch file.

[ohanar]

Replying to jdemeyer:

A standard solution that I have used for several packages is to create 1 patch file which patches both the .am and the .in file in that order. patch applies patches in order of the patch file.

Alternatively we can just elect to not patch the .am files since they aren't needed during the actual build process. I just updated the spkg so it will skip those patches.

My only change was to add the following in the patch loop:

if echo "$patch" | grep 'Makefile.am.patch$' > /dev/null; then
    continue
fi

[jdemeyer]

Why did you comment

#if [ "$SAGE_LOCAL" = "" ]; then
#    echo "SAGE_LOCAL undefined ... exiting";
#    echo "Maybe run 'sage -sh'?"
#    exit 1
#fi

(I might agree with you that this redundant, but let's at least be consistent)

[jdemeyer]

Why did you remove the configure flag

--libdir="$SAGE_LOCAL/lib"

[jdemeyer]

The following is wrong:

LDFLAGS="-I$SAGE_LOCAL/lib $LDFLAGS"; export LDFLAGS

I suppose you meant -L. In any case, you can simplify this to

export LDFLAGS="-L$SAGE_LOCAL/lib $LDFLAGS"

[jdemeyer]

Optionally, you can remove

# sage-env sets RM which will break libtool
# See http://trac.sagemath.org/sage_trac/ticket/7818#comment:28
unset RM

[kcrisman]

Replying to jdemeyer:

Why did you comment

#if [ "$SAGE_LOCAL" = "" ]; then
#    echo "SAGE_LOCAL undefined ... exiting";
#    echo "Maybe run 'sage -sh'?"
#    exit 1
#fi

(I might agree with you that this redundant, but let's at least be consistent)

Or even follow the developer guide ​recommended syntax.

[ohanar]

Replying to jdemeyer:

Why did you remove the configure flag

--libdir="$SAGE_LOCAL/lib"

Because it is implicit when you specify --prefix="$SAGE_LOCAL". The option is only needed if you want to install the lib directory in some weird location.

[ohanar]

Replying to jdemeyer:

Why did you comment

#if [ "$SAGE_LOCAL" = "" ]; then
#    echo "SAGE_LOCAL undefined ... exiting";
#    echo "Maybe run 'sage -sh'?"
#    exit 1
#fi

(I might agree with you that this redundant, but let's at least be consistent)

I was debugging, forgot to uncomment this :).

[fbissey]

Replying to ohanar:

Replying to jdemeyer:

Why did you remove the configure flag

--libdir="$SAGE_LOCAL/lib"

Because it is implicit when you specify --prefix="$SAGE_LOCAL". The option is only needed if you want to install the lib directory in some weird location.

Not quite. There is a reason this has been added not so long ago, so stuff wouldn't land in $SAGE_LOCAL/lib64 on some system. We didn't want to have a lib/lib64 thing then. The only exception is for the gcc spkg. That's why this spkg and a few other have this line. Now please put it back in.

[ohanar]

Ok, mentioned issues have been fixed.

[jdemeyer]

Why are you patching at -p2 level instead of -p1 like all other packages?

This isn't such a big deal, but since your goal is better standardization of packages, you should use -p1 patch files.

[jdemeyer]

Also, better document why you're not patching Makefile.am. Something along the lines of

We should not patch Makefile.am files, since that causes automake to be run
and we don't want to require automake to build Sage.

[jdemeyer]

positive_review if you add the comment regarding patching Makefile.am, if you mention this ticket number in SPKG.txt and if you make all files in src world-readable (permission 0644 or 0755).

[vbraun]

Bonus points for patching sage -pkg to check that all files are indeed world readable and all directories listable. On another ticket, of course ;-)

I take it that src/ChangeLog (Autosaved) (outdated version of src/ChangeLog) was already in the upstream sources. Does anybody know which editor has such a tempfile convention? But oh well if upstream ships it then we should include it by default ;-)

I think upstream is also missing some auto-generated files, or has mismatched versions. When I just configure/make the vanillas sources, I get

[vbraun@laptop t]$ make -j 8
cd . && /bin/sh /home/vbraun/Sage/spkg/cddlib-094g.p0/t/missing --run autoconf
aclocal.m4:16: warning: this file was generated for autoconf 2.63.
You have another version of autoconf.  It may work, but is not guaranteed to.
If you have problems, you may need to regenerate the build system entirely.
To do so, use the procedure documented by the package, typically `autoreconf'.
configure.in:10: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2662: _AC_LINK_IFELSE is expanded from...
../../lib/autoconf/general.m4:2679: AC_LINK_IFELSE is expanded from...
aclocal.m4:1037: _LT_SYS_MODULE_PATH_AIX is expanded from...
aclocal.m4:4173: _LT_LINKER_SHLIBS is expanded from...
aclocal.m4:5248: _LT_LANG_C_CONFIG is expanded from...
aclocal.m4:159: _LT_SETUP is expanded from...
aclocal.m4:88: LT_INIT is expanded from...
configure.in:10: the top level
configure.in:10: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in body

While I'm still in favour of just replacing all the autotool'ed files by new ones generated from scratch, I guess its good enough if this builds on all buildbot machines.

[jdemeyer]

Replying to vbraun:

I think upstream is also missing some auto-generated files, or has mismatched versions. When I just configure/make the vanillas sources, I get

[vbraun@laptop t]$ make -j 8
cd . && /bin/sh /home/vbraun/Sage/spkg/cddlib-094g.p0/t/missing --run autoconf

Looking at the log file, I did not get this problem with the spkg I tested in 28. Could it be that you accidentally messed up the timestamps?

I do consider it a serious issue, because it could cause build failures on systems which don't have autotools (or which have the wrong version) installed.

[vbraun]

Since the only changes we care about are some memleak fixes, I've made a new spkg that keeps our rewritten build system just with the new sources:

​http://www.stp.dias.ie/~vbraun/Sage/spkg/cddlib-094g.p1.spkg

[vbraun]

Replying to jdemeyer:

Looking at the log file, I did not get this problem with the spkg I tested in 28. Could it be that you accidentally messed up the timestamps?

Oops, right. Sorry about that.

[vbraun]

I've also disabled static libraries, for the record. Its time we get rid of that.

[jdemeyer]

Replying to vbraun:

Bonus points for patching sage -pkg to check that all files are indeed world readable and all directories listable. On another ticket, of course ;-)

There is a ticket for that somewhere, but there some discussion about whether it should be an error or warning.

[jpflori]

Did someone tried to contact the upstream author to get our patches integrated? At least the one which would make sense outside of Sage?

[Snark]

As far as I can tell this spkg is still patching cddlib pretty heavily (the random.* stuff) and adding two executables which aren't in upstream (cdd_both_reps_gmp and cdd_both_reps).

That means debian's cddlib (which comes from upstream) is incompatible with sage's fork.

[fbissey]

We are shipping the extra executables in Gentoo. I guess a better solution for a distro would be to split them in a separate package. Have to check the random stuff, autotool patching is quite necessary as far as I remember.

[Snark]

Well, yes, if it is sage code, it belongs to sage and not to cddlib (likewise for the other forks -- I know of cliquer #14349 and pari #14346, but I didn't know about cddlib this morning so perhaps there are others lurking).

[vbraun]

The "random" stuff is because otherwise the output is not deterministic. For an algorithm that does not benefit from randomization...

[Snark]

If it makes sense for upstream, it should be forwarded upstream!

[jpflori]

I wanted to contact upstream and query about the bunch of patches we apply and if they were aware of it when I was working on #13354 to be able to build a shared library on cygwin, but then I thought I should first ask here if someone already tried just in case, and cddlib is not really a problem for cygwin as the static lib seems to work ok and I went on to something else and it was maybe 8 or 7 monthes ago...

So yes, I think we should definitely contact upstream first. Most of the teams I've contacted for #13354 have reacted quite quickly. Of course some gave some signs of life and then disappeared. And some never reacted. But who knows, we should contact them first.

Note that in the case of lrcalc, see #13839, upstream even ended up distributing two tarballs:

• one historical, with the build system they developped;
• one based on our patches, with an autotool based build system.

[jpflori]

New commits:

​5961ef1

Update cddlib to 094g and tidy the patches a little bit.

[jpflori]

I suggest to postpone the splitting of non-upstream stuff to a follow up ticket. Let's already update or nothing at all will happen.

Some tweaks are also needed on Cygwin to build a shared lib.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​84b27cf

Update tests folder for cddlib.

[jpflori]

Oops, I forgot to update the tests (which looked the same but are not in fact).

Anyway, I'm personally (moderately but enough) happy with the way the pkg looks now. Let's get this merged.

---

New commits:

​84b27cf

Update tests folder for cddlib.

---

New commits:

​84b27cf

Update tests folder for cddlib.

[jpflori]

(I've not used Volker patches, especially that shared libs won't build on Cygwin (but should fallback to static libs, not sure about this)))

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​269c1e1

Previously failing cddlib related doctest now pass.

[jpflori]

#15871 for further clean up and #15872 for Cygwin shared lib.

[vbraun]

Not building a shared library would be a serious regression IMHO.

[fbissey]

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

​269c1e1

Previously failing cddlib related doctest now pass.

Thank you. I have had that doctest error in sage-on-gentoo for a year and 19 days - when we moved to 0.94g.

[jpflori]

Replying to vbraun:

Not building a shared library would be a serious regression IMHO.

I didn't disable the build of a shared library. I disabled the disabling of the build of a static library.

[jpflori]

Once we fix #15872 we can feed back the --enable-shared --disable-static stuff to configure. If that's a sufficient reason reason for you to review that ticket, I'll include it there with pleasure.

[jpflori]

(For now there's nothing there, I'm aware of that.)

[jpflori]

I was a little too fast. The mv part of spkg-install are wrong with the git version of sage.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​322ec47

Replace mv by cp in cddlib install script.

[jpflori]

Or not actually, as I guess we still copy everything before building.... Anyway, changes are here. I guess we can keep it as positive review then.

[jpflori]

For some mysterious reason the previously failing doctest does not fail either when I revert back to trac/develop, make distclean and rebuild...