Opened 2 years ago

Closed 8 months ago

Last modified 6 months ago

#13026 closed enhancement (fixed)

Upgrade and clean up cddlib

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

Description (last modified by jpflori)

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 jdemeyer 2 years ago.
Diff for the cddlib spkg. For reference / review only.
cddlib-094g.p0-nodelete.diff (19.8 KB) - added by jdemeyer 2 years ago.
Diff for the cddlib spkg without deletes. For reference / review only.

Download all attachments as: .zip

Change History (69)

comment:1 Changed 2 years ago by ohanar

  • Authors set to R. Andrew Ohana
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 2 years ago by ohanar

  • Keywords sd40.5 added

comment:3 Changed 2 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work
  • Work issues set to 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 2 years ago by ohanar

  • Status changed from needs_work to positive_review

Ok, hard links should be setup now.

comment:5 Changed 2 years ago by ohanar

  • Work issues make sure hard links used deleted

comment:6 Changed 2 years ago by kini

  • Description modified (diff)

comment:7 Changed 2 years ago by jdemeyer

  • Status changed from positive_review to needs_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 2 years ago by 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.

comment:9 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by 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.

comment:12 Changed 2 years ago by 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

comment:13 Changed 2 years ago by ohanar

  • Status changed from needs_work to needs_review

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

Changed 2 years ago by jdemeyer

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

Changed 2 years ago by jdemeyer

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

comment:14 follow-up: Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_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 2 years ago by ohanar

  • Status changed from needs_work to needs_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 2 years ago by ohanar (previous) (diff)

comment:16 follow-ups: Changed 2 years ago by 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)

comment:17 follow-up: Changed 2 years ago by jdemeyer

Why did you remove the configure flag

--libdir="$SAGE_LOCAL/lib"

comment:18 Changed 2 years ago by 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"

comment:19 Changed 2 years ago by 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

comment:20 Changed 2 years ago by jdemeyer

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:21 in reply to: ↑ 16 Changed 2 years ago by 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.

comment:22 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by 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.

comment:23 in reply to: ↑ 16 Changed 2 years ago by 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 :).

comment:24 in reply to: ↑ 22 Changed 2 years ago by 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.

comment:25 Changed 2 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, mentioned issues have been fixed.

comment:26 Changed 2 years ago by 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.

comment:27 Changed 2 years ago by 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.

comment:28 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_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 2 years ago by jdemeyer (previous) (diff)

comment:29 follow-ups: Changed 2 years ago by 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.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 2 years ago by 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.

comment:31 Changed 2 years ago by 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

comment:32 in reply to: ↑ 30 Changed 2 years ago by 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.

comment:33 Changed 2 years ago by vbraun

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

comment:34 Changed 2 years ago by jpflori

  • Cc jpflori added

comment:35 in reply to: ↑ 29 Changed 2 years ago by 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.

comment:36 Changed 23 months ago by 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?

comment:37 Changed 19 months ago by 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.

comment:38 Changed 19 months ago by Snark

  • Cc Snark added

comment:39 Changed 19 months ago by 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.

comment:40 Changed 19 months ago by 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).

comment:41 Changed 19 months ago by vbraun

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

comment:42 Changed 19 months ago by Snark

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

comment:43 Changed 19 months ago by 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.

comment:44 Changed 15 months ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:45 Changed 9 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:46 Changed 8 months ago by jpflori

  • Branch set to u/jpflori/ticket/13026
  • Commit set to 5961ef13d0f97894aa108ee8dd83924e4ea2bd07
  • Reviewers changed from Karl-Dieter Crisman, Jeroen Demeyer to Karl-Dieter Crisman, Jeroen Demeyer, Jean-Pierre Flori

New commits:

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

comment:47 Changed 8 months ago by jpflori

  • 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 8 months ago by git

  • Commit changed from 5961ef13d0f97894aa108ee8dd83924e4ea2bd07 to 84b27cf7690678571c732773b1d076ad64052c27

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

84b27cfUpdate tests folder for cddlib.

comment:49 Changed 8 months ago by 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:

84b27cfUpdate tests folder for cddlib.

New commits:

84b27cfUpdate tests folder for cddlib.

comment:50 Changed 8 months ago by 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)))

comment:51 follow-up: Changed 8 months ago by git

  • Commit changed from 84b27cf7690678571c732773b1d076ad64052c27 to 269c1e1551285566b8ba7a2b890989e5590e9f11

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

269c1e1Previously failing cddlib related doctest now pass.

comment:52 Changed 8 months ago by jpflori

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

comment:53 Changed 8 months ago by jpflori

  • Status changed from needs_work to positive_review

comment:54 follow-up: Changed 8 months ago by vbraun

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

comment:55 in reply to: ↑ 51 Changed 8 months ago by fbissey

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 8 months ago by 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.

comment:57 Changed 8 months ago by 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.

comment:58 Changed 8 months ago by jpflori

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

comment:59 Changed 8 months ago by jpflori

  • Status changed from positive_review to needs_work

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

comment:60 Changed 8 months ago by git

  • Commit changed from 269c1e1551285566b8ba7a2b890989e5590e9f11 to 322ec473abcf04fbcf610f3a43794f514ee3e62a

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

322ec47Replace mv by cp in cddlib install script.

comment:61 Changed 8 months ago by jpflori

  • Status changed from needs_work to positive_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 follow-up: Changed 8 months ago by jpflori

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 8 months ago by jpflori

  • Description modified (diff)

comment:64 in reply to: ↑ 62 Changed 8 months ago by jpflori

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 8 months ago by vbraun

  • Branch changed from u/jpflori/ticket/13026 to 322ec473abcf04fbcf610f3a43794f514ee3e62a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:66 Changed 6 months ago by Snark

  • Commit 322ec473abcf04fbcf610f3a43794f514ee3e62a deleted

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 6 months ago by vbraun

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.