Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#13026 closed enhancement (fixed)

Upgrade and clean up cddlib

Reported by: Keshav Kini Owned by: tbd
Priority: major Milestone: sage-6.2
Component: packages: standard Keywords: cddlib sd40.5
Cc: R. Andrew Ohana, Jean-Pierre Flori, Julien Puydt Merged in:
Authors: R. Andrew Ohana Reviewers: Karl-Dieter Crisman, Jeroen Demeyer, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: 322ec47 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jean-Pierre Flori)

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:

Attachments (2)

cddlib-094g.p0.diff (1.5 MB) - added by Jeroen Demeyer 10 years ago.
Diff for the cddlib spkg. For reference / review only.
cddlib-094g.p0-nodelete.diff (19.8 KB) - added by Jeroen Demeyer 10 years ago.
Diff for the cddlib spkg without deletes. For reference / review only.

Download all attachments as: .zip

Change History (69)

comment:1 Changed 11 years ago by R. Andrew Ohana

Authors: R. Andrew Ohana
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 11 years ago by R. Andrew Ohana

Keywords: sd40.5 added

comment:3 Changed 11 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_work
Work issues: make sure hard links used

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.

comment:4 Changed 11 years ago by R. Andrew Ohana

Status: needs_workpositive_review

Ok, hard links should be setup now.

comment:5 Changed 11 years ago by R. Andrew Ohana

Work issues: make sure hard links used

comment:6 Changed 11 years ago by Keshav Kini

Description: modified (diff)

comment:7 Changed 11 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

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

comment:8 Changed 11 years ago by Karl-Dieter Crisman

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.

comment:9 Changed 11 years ago by Jeroen Demeyer

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.

comment:10 Changed 11 years ago by François Bissey

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.

comment:11 Changed 11 years ago by Volker Braun

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.

comment:12 Changed 11 years ago by François Bissey

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

comment:13 Changed 10 years ago by R. Andrew Ohana

Status: needs_workneeds_review

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

Changed 10 years ago by Jeroen Demeyer

Attachment: cddlib-094g.p0.diff added

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

Changed 10 years ago by Jeroen Demeyer

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

comment:14 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

comment:15 in reply to:  14 Changed 10 years ago by R. Andrew Ohana

Status: needs_workneeds_review

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
Last edited 10 years ago by R. Andrew Ohana (previous) (diff)

comment:16 Changed 10 years ago by Jeroen Demeyer

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)

comment:17 Changed 10 years ago by Jeroen Demeyer

Why did you remove the configure flag

--libdir="$SAGE_LOCAL/lib"

comment:18 Changed 10 years ago by Jeroen Demeyer

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"

comment:19 Changed 10 years ago by Jeroen Demeyer

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

comment:20 Changed 10 years ago by Jeroen Demeyer

Reviewers: Karl-Dieter CrismanKarl-Dieter Crisman, Jeroen Demeyer
Status: needs_reviewneeds_work

comment:21 in reply to:  16 Changed 10 years ago by Karl-Dieter Crisman

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.

comment:22 in reply to:  17 ; Changed 10 years ago by R. Andrew Ohana

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.

comment:23 in reply to:  16 Changed 10 years ago by R. Andrew Ohana

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 :).

comment:24 in reply to:  22 Changed 10 years ago by François Bissey

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.

comment:25 Changed 10 years ago by R. Andrew Ohana

Status: needs_workneeds_review

Ok, mentioned issues have been fixed.

comment:26 Changed 10 years ago by Jeroen Demeyer

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.

comment:27 Changed 10 years ago by Jeroen Demeyer

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.

comment:28 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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).

Last edited 10 years ago by Jeroen Demeyer (previous) (diff)

comment:29 Changed 10 years ago by Volker Braun

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.

comment:30 in reply to:  29 ; Changed 10 years ago by Jeroen Demeyer

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.

comment:31 Changed 10 years ago by Volker Braun

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

comment:32 in reply to:  30 Changed 10 years ago by Volker Braun

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.

comment:33 Changed 10 years ago by Volker Braun

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

comment:34 Changed 10 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:35 in reply to:  29 Changed 10 years ago by Jeroen Demeyer

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.

comment:36 Changed 10 years ago by Jean-Pierre Flori

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

comment:37 Changed 10 years ago by Julien Puydt

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.

comment:38 Changed 10 years ago by Julien Puydt

Cc: Julien Puydt added

comment:39 Changed 10 years ago by François Bissey

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.

comment:40 Changed 10 years ago by Julien Puydt

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).

comment:41 Changed 10 years ago by Volker Braun

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

comment:42 Changed 10 years ago by Julien Puydt

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

comment:43 Changed 10 years ago by Jean-Pierre Flori

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.

comment:44 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:45 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:46 Changed 9 years ago by Jean-Pierre Flori

Branch: u/jpflori/ticket/13026
Commit: 5961ef13d0f97894aa108ee8dd83924e4ea2bd07
Reviewers: Karl-Dieter Crisman, Jeroen DemeyerKarl-Dieter Crisman, Jeroen Demeyer, Jean-Pierre Flori

New commits:

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

comment:47 Changed 9 years ago by Jean-Pierre Flori

Description: modified (diff)

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.

comment:48 Changed 9 years ago by git

Commit: 5961ef13d0f97894aa108ee8dd83924e4ea2bd0784b27cf7690678571c732773b1d076ad64052c27

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

84b27cfUpdate tests folder for cddlib.

comment:49 Changed 9 years ago by Jean-Pierre Flori

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:

84b27cfUpdate tests folder for cddlib.

New commits:

84b27cfUpdate tests folder for cddlib.

comment:50 Changed 9 years ago by Jean-Pierre Flori

(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)))

comment:51 Changed 9 years ago by git

Commit: 84b27cf7690678571c732773b1d076ad64052c27269c1e1551285566b8ba7a2b890989e5590e9f11

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

269c1e1Previously failing cddlib related doctest now pass.

comment:52 Changed 9 years ago by Jean-Pierre Flori

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

comment:53 Changed 9 years ago by Jean-Pierre Flori

Status: needs_workpositive_review

comment:54 Changed 9 years ago by Volker Braun

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

comment:55 in reply to:  51 Changed 9 years ago by François Bissey

Replying to git:

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

269c1e1Previously 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.

comment:56 in reply to:  54 Changed 9 years ago by Jean-Pierre Flori

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.

comment:57 Changed 9 years ago by Jean-Pierre Flori

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.

comment:58 Changed 9 years ago by Jean-Pierre Flori

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

comment:59 Changed 9 years ago by Jean-Pierre Flori

Status: positive_reviewneeds_work

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

comment:60 Changed 9 years ago by git

Commit: 269c1e1551285566b8ba7a2b890989e5590e9f11322ec473abcf04fbcf610f3a43794f514ee3e62a

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

322ec47Replace mv by cp in cddlib install script.

comment:61 Changed 9 years ago by Jean-Pierre Flori

Status: needs_workpositive_review

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.

comment:62 Changed 9 years ago by Jean-Pierre Flori

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

comment:63 Changed 9 years ago by Jean-Pierre Flori

Description: modified (diff)

comment:64 in reply to:  62 Changed 9 years ago by Jean-Pierre Flori

Replying to jpflori:

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

No such problem on another machine.

comment:65 Changed 9 years ago by Volker Braun

Branch: u/jpflori/ticket/13026322ec473abcf04fbcf610f3a43794f514ee3e62a
Resolution: fixed
Status: positive_reviewclosed

comment:66 Changed 9 years ago by Julien Puydt

Commit: 322ec473abcf04fbcf610f3a43794f514ee3e62a

Sorry to come so late to the party, but looking at sage's git, I don't really see why this bug was closed: the cddlib spkg still has patches that mean sage isn't based on cddlib, but on a fork...

comment:67 Changed 9 years ago by Volker Braun

The bug was closed because the branch was merged. It might not be perfect but it was an improvement. Feel free to work on this.

Note: See TracTickets for help on using tickets.