#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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)
Change History (69)
comment:1 Changed 11 years ago by
Authors: | → R. Andrew Ohana |
---|---|
Description: | modified (diff) |
Status: | new → needs_review |
comment:2 Changed 11 years ago by
Keywords: | sd40.5 added |
---|
comment:3 Changed 11 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → needs_work |
Work issues: | → make sure hard links used |
comment:4 Changed 11 years ago by
Status: | needs_work → positive_review |
---|
Ok, hard links should be setup now.
comment:5 Changed 11 years ago by
Work issues: | make sure hard links used |
---|
comment:6 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:7 Changed 11 years ago by
Status: | positive_review → 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 11 years ago by
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
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
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
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
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 11 years ago by
Status: | needs_work → needs_review |
---|
Ok, I've updated the spkg, hopefully the issues are fixed (it now compiles fine for me with boxen's toolchain).
Changed 11 years ago by
Attachment: | cddlib-094g.p0.diff added |
---|
Diff for the cddlib spkg. For reference / review only.
Changed 11 years ago by
Attachment: | cddlib-094g.p0-nodelete.diff added |
---|
Diff for the cddlib spkg without deletes. For reference / review only.
comment:14 follow-up: 15 Changed 11 years ago by
Status: | needs_review → 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 Changed 11 years ago by
Status: | needs_work → 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
comment:16 follow-ups: 21 23 Changed 11 years ago by
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: 22 Changed 11 years ago by
Why did you remove the configure flag
--libdir="$SAGE_LOCAL/lib"
comment:18 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
Reviewers: | Karl-Dieter Crisman → Karl-Dieter Crisman, Jeroen Demeyer |
---|---|
Status: | needs_review → needs_work |
comment:21 Changed 11 years ago by
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 follow-up: 24 Changed 11 years ago by
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 Changed 11 years ago by
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 Changed 11 years ago by
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 11 years ago by
Status: | needs_work → needs_review |
---|
Ok, mentioned issues have been fixed.
comment:26 Changed 10 years ago by
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
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
Status: | needs_review → needs_work |
---|
positive_review if you add the comment regarding patching Makefile.am
and if you mention this ticket number in SPKG.txt
.
comment:29 follow-ups: 30 35 Changed 10 years ago by
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 follow-up: 32 Changed 10 years ago by
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
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 Changed 10 years ago by
comment:33 Changed 10 years ago by
I've also disabled static libraries, for the record. Its time we get rid of that.
comment:34 Changed 10 years ago by
Cc: | jpflori added |
---|
comment:35 Changed 10 years ago by
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
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
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
Cc: | Snark added |
---|
comment:39 Changed 10 years ago by
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
comment:41 Changed 10 years ago by
The "random" stuff is because otherwise the output is not deterministic. For an algorithm that does not benefit from randomization...
comment:43 Changed 10 years ago by
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
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:45 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:46 Changed 9 years ago by
Branch: | → u/jpflori/ticket/13026 |
---|---|
Commit: | → 5961ef13d0f97894aa108ee8dd83924e4ea2bd07 |
Reviewers: | Karl-Dieter Crisman, Jeroen Demeyer → Karl-Dieter Crisman, Jeroen Demeyer, Jean-Pierre Flori |
New commits:
5961ef1 | Update cddlib to 094g and tidy the patches a little bit.
|
comment:47 Changed 9 years ago by
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
Commit: | 5961ef13d0f97894aa108ee8dd83924e4ea2bd07 → 84b27cf7690678571c732773b1d076ad64052c27 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
84b27cf | Update tests folder for cddlib.
|
comment:49 Changed 9 years ago by
comment:50 Changed 9 years ago by
(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: 55 Changed 9 years ago by
Commit: | 84b27cf7690678571c732773b1d076ad64052c27 → 269c1e1551285566b8ba7a2b890989e5590e9f11 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
269c1e1 | Previously failing cddlib related doctest now pass.
|
comment:53 Changed 9 years ago by
Status: | needs_work → positive_review |
---|
comment:54 follow-up: 56 Changed 9 years ago by
Not building a shared library would be a serious regression IMHO.
comment:55 Changed 9 years ago by
comment:56 Changed 9 years ago by
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
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:59 Changed 9 years ago by
Status: | positive_review → 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 9 years ago by
Commit: | 269c1e1551285566b8ba7a2b890989e5590e9f11 → 322ec473abcf04fbcf610f3a43794f514ee3e62a |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
322ec47 | Replace mv by cp in cddlib install script.
|
comment:61 Changed 9 years ago by
Status: | needs_work → 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: 64 Changed 9 years ago by
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
Description: | modified (diff) |
---|
comment:64 Changed 9 years ago by
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
Branch: | u/jpflori/ticket/13026 → 322ec473abcf04fbcf610f3a43794f514ee3e62a |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:66 Changed 9 years ago by
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
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.
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.