Opened 7 years ago

Closed 7 years ago

#13325 closed defect (fixed)

eclib does not build on Cygwin

Reported by: jpflori Owned by: tbd
Priority: major Milestone: sage-5.6
Component: porting: Cygwin Keywords: eclib spkg cygwin
Cc: kcrisman, dimpase, cremona, vbraun Merged in: sage-5.6.beta0
Authors: Jean-Pierre Flori Reviewers: John Cremona
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: Commit:
Dependencies: #13333 Stopgaps:

Description (last modified by jdemeyer)

There are two problems with the current spkg on Cygwin:

  • -lgmp should come after -lntl and -lpari, I fixed this in configure.ac
  • the executables names in tests/Makefile.am should finish with $(EXEEXT), fixed there as well.
  • the -no-undefined flag should also be passed to libtool so that a shared library gets built on Cygwin and for sage.libs.mwrank to be functional.

All of this is fixed in the 2012-08-30 upstream release.

Apply:

  1. trac_13325.patch
  2. the spkg http://www.infres.enst.fr/~flori/sage/eclib-20120830.spkg

Attachments (7)

13325.patch (1.0 MB) - added by jpflori 7 years ago.
Spkg diff, for review only.
eclib.patch (1.1 MB) - added by jpflori 7 years ago.
Spkg diff, for review only. Not committed in spkg.
spkg.diff (13.9 KB) - added by jpflori 7 years ago.
Spkg diff, for review only. Committed in spkg.
eclib-am.patch (6.3 KB) - added by jpflori 7 years ago.
Patch to am files.
eclib-20120824.patch (2.4 KB) - added by jpflori 7 years ago.
Spkg diff, for review only. Committed in spkg.
trac_13325.patch (3.6 KB) - added by jpflori 7 years ago.
Patch to Sage library.
eclib-20120830.patch (2.3 KB) - added by jpflori 7 years ago.
Spkg diff, for review only. Committed in spkg.

Change History (128)

Changed 7 years ago by jpflori

Spkg diff, for review only.

comment:1 Changed 7 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • Cc kcrisman dimpase cremona added
  • Description modified (diff)
  • Status changed from new to needs_info

Updated spkg available at: http://perso.telecom-paristech.fr/~flori/sage/eclib-20120428.p0.spkg

Although I'd prefer package an updated upstream version including such fixes. John, could you update eclib ggoglecode repo? If you're convinced the changes I propose are useful... At least, they seem harmless on Linux and let the spkg build on Cygwin.

comment:2 Changed 7 years ago by jpflori

Hmmm, the 13325.patch file I uploaded is not the one I planned to. That's a random previous version of the patch, please wait for me to upload a file named spkg.diff.

comment:3 Changed 7 years ago by jpflori

The current spkg does not build yet. I get multiple definitions of _roots while building d2.exe: in d2 and in libpari.a(rootpol.o) apparently.

comment:4 Changed 7 years ago by cremona

It is certainly preferable for the eclib source (build system) to be changed so we do not need to patch it. I have some other suggested changes to the build system from a debian developer -- it would be very helpful if someone else could look at those too. I am travelling at the moment and will not be able to think about this properly for the next 2+ weeks.

comment:5 Changed 7 years ago by jpflori

That's because a static version of libpari is used. Using the shared one (by copying libpari.dll to libpari.dll.a) solves the problem.

comment:6 Changed 7 years ago by jpflori

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Workaround found; Bug reported upstream.
  • Work issues set to wait for official update of the build system; fix pari problem (new spkg?)

comment:7 Changed 7 years ago by jpflori

Another question to John: For the PARI stuff, would you consider renaming the function to something else? That would allow to link the programs statically (not that I really care, I think it's more important to let Cygwin find the shared lib of PARI before the static one anyway).

comment:8 Changed 7 years ago by jpflori

Apparently pari creates a libpari.dll.a file but I guess Sage does not correctly copy it. I also see strange if test "libpari-gmp.dll" != "libpari-gmp.dll"

Have to have a look to pari spkg install script...

comment:9 Changed 7 years ago by dimpase

The spkg does not work for me; I get

/bin/sh ../libtool --tag=CXX    --mode=link g++ -I/home/Dima/sage-5.2.rc1/local/include  -I/home/Dima/sage-5.2.rc1/local/include  -I/home/Dima/sage-5.2.rc1/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3  -ljc -L../libsrc -L/home/Dima/sage-5.2.rc1/local/lib -lntl -lgmp -L/home/Dima/sage-5.2.rc1/local/lib -lpari -lgmp   -o d2.exe d2.o
libtool: link: g++ -I/home/Dima/sage-5.2.rc1/local/include -I/home/Dima/sage-5.2.rc1/local/include -I/home/Dima/sage-5.2.rc1/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3 -o .libs/d2.exe d2.o  /home/Dima/sage-5.2.rc1/spkg/build/eclib-20120428.p0/src/libsrc/.libs/libjc.a /usr/lib/gcc/i686-pc-cygwin/4.5.3/libstdc++.dll.a -L../libsrc -L/home/Dima/sage-5.2.rc1/local/lib -lntl -lpari /home/Dima/sage-5.2.rc1/local/lib/libgmp.dll.a -L/usr/lib/gcc/i686-pc-cygwin/4.5.3 -L/home/Dima/sage-5.2.rc1/local/lib
/home/Dima/sage-5.2.rc1/local/lib/libpari.a(rootpol.o): In function `roots':
/home/Dima/sage-5.2.rc1/spkg/build/pari-2.5.1.p3/src/Ocygwin-i686/../src/basemath/rootpol.c:2057: multiple definition of `_roots'
d2.o:/home/Dima/sage-5.2.rc1/spkg/build/eclib-20120428.p0/src/tests/d2.cc:149: first defined here
collect2: ld returned 1 exit status
Makefile:679: recipe for target `d2.exe' failed
make[3]: *** [d2.exe] Error 1

By the way, did I have to install a different PARI spkg, too?

comment:10 follow-up: Changed 7 years ago by jpflori

Yes you have to somehow tweak PARI to let d2 build.

The quickest hack is to copy libpari.dll to libpari.dll.a.

However I'd prefer to copy the proper import file dll.a ggenerated by PARI when it's build.

And I'd prefer such a fix to be included in PARI itself rather than in a Sage spkg. I'll contact the PARI team today. Hopefully this could make it into 2.5.2 for which Jeroen is currently preparing a spkg as well.

comment:11 in reply to: ↑ 10 Changed 7 years ago by dimpase

Replying to jpflori:

Yes you have to somehow tweak PARI to let d2 build.

The quickest hack is to copy libpari.dll to libpari.dll.a.

there are 2 files named libpari.dll in SAGELOCAL. One in bin/, and another in lib/... And both of them aren't archive files, but "real" dlls. Tried your hack for the one in bin/, it didn't help.

And, by the way, how would it help the problem of multiple definition of `_roots' This looks like overlinking rather than underlinking to me.

comment:12 Changed 7 years ago by jpflori

I never said they were archive files...

I said that when linking the linker finds libpari.a BEFORE libpari.dll. But as there is already a function named _roots in an object file included in libpari.a it fails. That's the problem.

From now on, lets suppose we live in the lib directory and forget about the bin one which is not of interest for our purposes.

So a hack consists in making sure that the linker find libpari.dll BEFORE libpari.a. To do that, you can copy libpari.dll to libpari.dll.a. You could just rename it if you want to have some fun. You could also delete libpari.a or rename it to jdlksjlkdjls.

Or we could explicitely tell the linker to use libpari.dll, rather than just passing -lpari, but that needs more work. And anyway, PARI generates a file ending by .dll.a. AFAIK it's not a copy od libpari.dll, not a dll file, but some import file. The point is that the linker will find it BEFORE libpari.a if its present and by looking at it it will automatically use the libpari.dll file.

comment:13 Changed 7 years ago by jpflori

The point is that if you link to the dll rather than the archive file, ld won't complain, which is quite understandable because the _roots function from libpari is not needed nor called directly from eclib (and how could it be anyway as theres a function called so in eclib itself). The two namespaces/worlds are kept as separate as possible with ld just doing its magic when needed.

If you link archive files together, ar will try to put everything from pari and from eclib in one file, in particular it will have some trouble putting to functions with the same name in that one file. The two namespaces/worlds are colliding.

comment:14 Changed 7 years ago by cremona

I'm very sorry this is calling you trouble, as I hardly use libpari at all (in eclib), literally only for integer factorization. I am planning an upgrade to eclib which will make FLINT a dependency, in which case I might use FLINT for integer factorization instead. But not very soon.

comment:15 Changed 7 years ago by dimpase

  • Description modified (diff)

comment:16 Changed 7 years ago by jpflori

  • Description modified (diff)

comment:17 Changed 7 years ago by jpflori

  • Description modified (diff)

The -no-undefined flag should also be passed to libtool so that a shared library gets built on Cygwin.

Changed 7 years ago by jpflori

Spkg diff, for review only. Not committed in spkg.

comment:18 Changed 7 years ago by jpflori

  • Description modified (diff)

Updated spkg at the same url adding the -no-undefined flag. This has the nice consequence that eclib now builds a shared library and that sage.libs.mwrank is functional.

comment:19 Changed 7 years ago by jpflori

  • Status changed from needs_info to needs_review
  • Work issues wait for official update of the build system; fix pari problem (new spkg?) deleted

As a temporary solution I've uploaded an spkg reautoconfed using #13357, so that there is no spkg size explosion. Not retested on Cygwin yet, but it should be as functional as before. Nothing is definitely committed or tagged yet, I'll do it after review.

comment:20 Changed 7 years ago by jpflori

  • Dependencies set to #13333

comment:21 Changed 7 years ago by cremona

I'm going to need some help & advice here. Since the last release of eclib with autotools there are three sets of changes: (1) the ones by you people for Sage (triggered by cygwin issues); (2) some changes sent to me by email by Bernhard Link of debian; (3) changes made by me to make FLINT 3 a prerequisite, since by using FLINT in a few key places I obtain a vast speedup of some programs (nto ones used by Sage currently). Clearly I can absorb the first two sets easily, but I am not sure about the last one, since until Sage starts to use FLINT 3 I would have to get autoconf to determine whether or not to use FLINT by setting some compiler switches or similar. And I am very capable of getting very confused in merging 3 sets of changes to anything...

comment:22 follow-up: Changed 7 years ago by jpflori

FLINT 3? Is that a super secret project :) If you mean FLINT 2.3, I personally think that it should quickly go into Sage, see #12173, which seems (almost?) functional to me.

I still have some issues with Cygwin there, so the spkg I posted is not vanilla #12173, but rather vanilla plus a quite large rewrite of the build system. I'm aware that's a bad idea, because it won't get into FLINT 2.3, but I won't have the time to produce a bunch of small patches in order for only the Cygwin part to get included.

As Cygwin is not required to be supported, we should surely just ship a vanilla FLINT 2.3 when it gets released and wait for Bill to integrate my changes (if he feels like) in a 2.4 or subsequent release.

comment:23 in reply to: ↑ 22 Changed 7 years ago by cremona

Replying to jpflori:

FLINT 3? Is that a super secret project :) If you mean FLINT 2.3, I personally think that it should quickly go into Sage, see #12173, which seems (almost?) functional to me.

Yes, I did mean 2.3! I guess FLINT3 is waiting for Bill to write a new language / compiler...

comment:24 Changed 7 years ago by jpflori

  • Status changed from needs_review to needs_work

Im finally testing the last spkg, and it seems I deleted something in it. I'll fix it tomorrow.

Changed 7 years ago by jpflori

Spkg diff, for review only. Committed in spkg.

comment:25 Changed 7 years ago by jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Spkg is now fixed. The error was caused by the fact that files of the build system were patched and so their timestamp updated. Then autotool magic wanted to regenerate some of them, which would fail because some pieces needed to do that are missing (this would also be true with the previous spkg). So touching some files in the right order after patching solves the issue.

comment:26 Changed 7 years ago by cremona

I am right now working on making the changes you have been making to the upstream distribution. I will report here when that is done. At the same time I am making the changes suggested by Bernhard Link of debian (but nothing to do with FLINT at this point).

comment:27 follow-up: Changed 7 years ago by cremona

I want to make sure that understand the changes that you made so I can make them "upstream" and hence avoid any patching. I realise that this will mean a new version of the spkg: when I am done the eclib version will be bumped up to 2012-08-21 (assuming that I finish today) so the next spkg can have a new name instead of a patch level.

From the Changelog in SPKG.txt:

+ * #13325: let eclib build on Cygwin.
+ * Put -lgmp after -lntl and -lpari in configure.ac.
+ * Add $(EXEEXT) to the targets in tests/Makefile.am.
+ * Add '-no-undefined' flag to libjc.la LDFLAGS so that a shared library
+   is built on Cygwin.
+ * automake and autoconf were run to regenerate the configure script and
+   Makefiles.
  1. will no longer be necessary. These flags are now set i nthe various Makefile.am using the macros LIBADD and LDADD, and I did put gmp last.
  1. $(EXEEXT). I assume that this is set on cygwin (to ".exe" or something) but not otherwise, so (apart from cluttering up the Makefiles) there will be no effect on other systems? You say that this is done in src/tests, but should it also be done in src/progs? The latter builds some binaries which are actually installed (e.g. mwrank).
  1. '-no-indefined' OK I can add that too. By the way, the library name is now libec and not libjc.
  1. OK, so I'm a bit uncertain as to which auto* files to actually include. When I change configure.ac or any Makefile.am files I run autoreconf. I never touch or look at configure or the actual Makefiles.

So I will now deal with points 2 and 3 and hand back to you, if that is OK.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 7 years ago by jpflori

Replying to cremona:

  1. will no longer be necessary. These flags are now set i nthe various Makefile.am using the macros LIBADD and LDADD, and I did put gmp last.

Great.

  1. $(EXEEXT). I assume that this is set on cygwin (to ".exe" or something) but not otherwise, so (apart from cluttering up the Makefiles) there will be no effect on other systems? You say that this is done in src/tests, but should it also be done in src/progs? The latter builds some binaries which are actually installed (e.g. mwrank).

Yes, this is only "really" needed on Cygwin. On most systems, let's say Linuxes, $(EXEEXT) will be void, so putting it or not won't have any visible effect.

If I remember correctly, the executables in the progs directory already magically included the $(EXEEXT) (or at least the file produced ended up in .exe). There must be some difference between the two Makefile.am you used. I can have a look if you want.

  1. '-no-indefined' OK I can add that too. By the way, the library name is now libec and not libjc.

Great (jsut beware of the typo here :)

  1. OK, so I'm a bit uncertain as to which auto* files to actually include. When I change configure.ac or any Makefile.am files I run autoreconf. I never touch or look at configure or the actual Makefiles.

I'm not really sure of what should be included in a distribution tarball, I've begun fighting with autotools just a few weeks ago. What was really painfull here is that it seemed that having a more recent configure than aclocal.m4 was detected by configure itself and let it try to regenerate aclocal.m4 (which failed because of the lack of a proper m4 directory with I don't really know what macros inside). The same was true for Makefile.in.

Don't know what the behavior if you actually omit the configure.ac and Makefile.am files.

So I will now deal with points 2 and 3 and hand back to you, if that is OK.

comment:29 in reply to: ↑ 28 Changed 7 years ago by jpflori

Replying to jpflori:

Replying to cremona:

  1. will no longer be necessary. These flags are now set i nthe various Makefile.am using the macros LIBADD and LDADD, and I did put gmp last.

Great.

  1. $(EXEEXT). I assume that this is set on cygwin (to ".exe" or something) but not otherwise, so (apart from cluttering up the Makefiles) there will be no effect on other systems? You say that this is done in src/tests, but should it also be done in src/progs? The latter builds some binaries which are actually installed (e.g. mwrank).

Yes, this is only "really" needed on Cygwin. On most systems, let's say Linuxes, $(EXEEXT) will be void, so putting it or not won't have any visible effect.

If I remember correctly, the executables in the progs directory already magically included the $(EXEEXT) (or at least the file produced ended up in .exe). There must be some difference between the two Makefile.am you used. I can have a look if you want.

Disregard this last comment, I did not get what you said. The splitting of progs and tests is posterior to the current spkg Sage ships.

But yes, I guess you could add $(EXEEXT) everywhere I guess, I must admit I don't really really remember if this is really necessary on Cygwin... but on Windows executables usually finish with .exe, so it can not hurt. Plus, on recent Cygwin versions, there seems to be some automatic translation from prog to prog.exe (I guess this also made my changes actually work when setting SAGE_CHECK, because I did not add $(EXEEXT) at each invocation of the programs as I just realized).

  1. '-no-indefined' OK I can add that too. By the way, the library name is now libec and not libjc.

Great (jsut beware of the typo here :)

  1. OK, so I'm a bit uncertain as to which auto* files to actually include. When I change configure.ac or any Makefile.am files I run autoreconf. I never touch or look at configure or the actual Makefiles.

I'm not really sure of what should be included in a distribution tarball, I've begun fighting with autotools just a few weeks ago. What was really painfull here is that it seemed that having a more recent configure than aclocal.m4 was detected by configure itself and let it try to regenerate aclocal.m4 (which failed because of the lack of a proper m4 directory with I don't really know what macros inside). The same was true for Makefile.in.

Don't know what the behavior if you actually omit the configure.ac and Makefile.am files.

So I will now deal with points 2 and 3 and hand back to you, if that is OK.

comment:30 Changed 7 years ago by cremona

Regarding $(EXEEXT): I have been readong up on this. Yes, the suffix gets automagically added to bin_PROGRAMS targets but apparently not to everything. See http://www.gnu.org/software/automake/manual/automake.html#index-Executable-extension-620 . I assumed that you (or someone) only added this manually where it was needed, but forgot that the clib version now in Sage was as old as it is.

See #13021 for example (not my ticket and I was not CC'ed onto it).

comment:31 Changed 7 years ago by cremona

On rereading the automake manual reference (previous post) I would assume that the suffix was provided for you *unless* you find that in some cases it is not, rather than adding the suffix manually everywhere.

comment:32 Changed 7 years ago by jpflori

I think I now remember what the problem was:

  • autotools indeed does its magic with $(EXEEXT),
  • but you directly put smattest (and others) in a target, so make tries to build it generically and doesn't use the magic smattest$(EXEEXT) target and its autotools CFLAGS (and the one you provided to it) so it fails
  • except on Linux (and others) where the autotools magic defines a target for smattest and we get lucky...

Here is an excerpt of what failed on Cygwin:

libtool: link: g++ -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3 -o .libs/mwrank.exe mwrank.o  /home/jp/sage-5.2/spkg/build/eclib-20120428.p0/src/libsrc/.libs/libjc.a /usr/lib/gcc/i686-pc-cygwin/4.5.3/libstdc++.dll.a -L../libsrc -L/home/jp/sage-5.2/local/lib -lntl -lpari /home/jp/sage-5.2/local/lib/libgmp.dll.a -L/usr/lib/gcc/i686-pc-cygwin/4.5.3 -L/home/jp/sage-5.2/local/lib
g++ -g -O3     smattest.cc   -o smattest
smattest.cc:30:25: erreur fatale: eclib/arith.h : No such file or directory
compilation terminée.
<builtin>: recipe for target `smattest' failed

After adding $(EXEEXT) I got

/bin/sh ../libtool --tag=CXX    --mode=link g++ -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3  -ljc -L../libsrc -L/home/jp/sage-5.2/local/lib -lntl -lgmp -L/home/jp/sage-5.2/local/lib -lpari -lgmp   -o mwrank.exe mwrank.o  
libtool: link: g++ -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3 -o .libs/mwrank.exe mwrank.o  /home/jp/sage-5.2/spkg/build/eclib-20120428.p0/src/libsrc/.libs/libjc.dll.a -L../libsrc -L/home/jp/sage-5.2/local/lib /home/jp/sage-5.2/local/lib/libntl.dll.a /usr/lib/gcc/i686-pc-cygwin/4.5.3/libstdc++.dll.a -lpari /home/jp/sage-5.2/local/lib/libgmp.dll.a -L/home/jp/sage-5.2/local/lib -L/usr/lib/gcc/i686-pc-cygwin/4.5.3
g++ -DPACKAGE_NAME=\"eclib\" -DPACKAGE_TARNAME=\"eclib\" -DPACKAGE_VERSION=\"2012-04-25\" -DPACKAGE_STRING=\"eclib\ 2012-04-25\" -DPACKAGE_BUGREPORT=\"john.cremona@gmail.com\" -DPACKAGE_URL=\"\" -DPACKAGE=\"eclib\" -DVERSION=\"2012-04-25\" -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_ALLOCA_H=1 -DHAVE_ALLOCA=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_UNISTD_H=1 -DHAVE__BOOL=1 -DHAVE_STDBOOL_H=1 -Drestrict=__restrict -DHAVE_FLOOR=1 -DHAVE_MEMMOVE=1 -DHAVE_MEMSET=1 -DHAVE_POW=1 -DHAVE_SQRT=1 -DHAVE_STRCHR=1 -I.   -I../libsrc -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3  -MT smattest.o -MD -MP -MF .deps/smattest.Tpo -c -o smattest.o smattest.cc
mv -f .deps/smattest.Tpo .deps/smattest.Po
/bin/sh ../libtool --tag=CXX    --mode=link g++ -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include  -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3  -ljc -L../libsrc -L/home/jp/sage-5.2/local/lib -lntl -lgmp -L/home/jp/sage-5.2/local/lib -lpari -lgmp   -o smattest.exe smattest.o  
libtool: link: g++ -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -I/home/jp/sage-5.2/local/include -DNTL_ALL -DUSE_PARI_FACTORING -DMETHOD=2 -DNEW_OP_ORDER -g -O3 -o .libs/smattest.exe smattest.o  /home/jp/sage-5.2/spkg/build/eclib-20120428.p0/src/libsrc/.libs/libjc.dll.a -L../libsrc -L/home/jp/sage-5.2/local/lib /home/jp/sage-5.2/local/lib/libntl.dll.a /usr/lib/gcc/i686-pc-cygwin/4.5.3/libstdc++.dll.a -lpari /home/jp/sage-5.2/local/lib/libgmp.dll.a -L/home/jp/sage-5.2/local/lib -L/usr/lib/gcc/i686-pc-cygwin/4.5.3

comment:33 Changed 7 years ago by cremona

OK -- so I just removed all mention of EXEEXT from the Makefile.am's and redid autoreconf, and in the resulting Makefiles all the targets have got their $(EXEEXT) suffices on correctly. So provided that these Makefiles are in the distribution it should not be necessary to do anything special. Could you try that? There's a new eclib tarball at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/

comment:34 Changed 7 years ago by jpflori

I fear the problem is still present. Have a look at tests/Makefile.in; there at line 571, the PROCS_TESTS variable is defined as a list of executable names which do not include $(EXEEXT). So when make will try to build the procs_tests target, it try to build targets without the $(EXEEXT), which on Cygwin are not defined by the autotools magic because EXEXT is not empty. On Linux, as EXEEXT is empty, there will be no problem.

I'll confirm that Cygwin fails in a few tens of minutes.

comment:35 Changed 7 years ago by jpflori

(In the other places there is indeed $(EXEEXT) thanks to the autotools magic, the problem is precisely that target in proc_tests do not refer to this autogenerated targets!)

comment:36 Changed 7 years ago by jpflori

Mmmm, in fact the build failed while linking libec. Not really sure what the point of LIBADD is, but it does not get used for libec. Although there is a libec_la_LIBADD variable which is empty in Makefile.in. Maybe that's what you meant to add? http://sources.redhat.com/autobook/autobook/autobook_92.html suggests it is at least (no plain LIBADD variable mentionned there, only the LIBADD macro).

comment:37 Changed 7 years ago by jpflori

Apparently a general AM_LIBADD was suggested: http://osdir.com/ml/automake-gnu/2012-06/msg00019.html

comment:38 Changed 7 years ago by jpflori

Or you could use LIBS?

comment:39 Changed 7 years ago by cremona

I'll ask the debian guy about LIBADD etc, since that was one of his.

comment:40 Changed 7 years ago by jpflori

By the way, using libec_la_LIBADD let libec compile and in fact I got no problems with smattest (and the following programs)!

comment:41 Changed 7 years ago by jpflori

The non-failure of the smattest and co might just be luck. Now the Makefile producedfirst includes an all target line with as dependencies all the binaries (so with the EXEEXT extension). And at the end of the Makefile, the all dependencies from your Makefile.am file is also included, so it points through the procs_tests target to the binaries without the EXEEXT extension.

What happens I guess is that make first generates the file with a .exe extension, and when it want to generate the files without .exe, the automagic translation made by Cygwin makes it think that smattest=smattest.exe is already uptodate.

Not sure why this did not happen before. Maybe was the noinst_PROGRAMS line in Makefile.am added recently?

Whatsoever, I'm not convinced that relying on this behavior from Cygwin is a good idea.

comment:42 follow-up: Changed 7 years ago by jpflori

Not sure this is the right place to continue this discussion, but here are some additional poitns:

  • About the EXEEXT stuff: http://www.gnu.org/savannah-checkouts/gnu/automake/manual/html_node/EXEEXT.html#EXEEXT So I guess you unfortunately really have to add it to the PROCS_TESTS and similar variables. This can be automated using some Makefile loops.
  • about the all target in the progs dir, your first dependencies includes all programs, and then you got Automake PROGRAMS stuff defined depending on the allprogs configuration flag, see below for further comments.

Two additional questions:

  • when allprogs is disabled, only the library and mwrank are installed, but everything is built. Is this intended? On solw platform (let's say Cygwin...) it would be nice to only build what is installed (and so remove the noinst_PROGRAMS stuff). I see the noinst was added in commit 93229fe24a35, could you elaborate on that? has it something to do with check targets?
  • all your tests in the check targets are really similar, except for the executable name of course, do you plan on refatoring that?

comment:43 in reply to: ↑ 42 ; follow-up: Changed 7 years ago by cremona

  • Cc vbraun added

Replying to jpflori:

Sorry for the delay, when I got to this today trac was down and then had to wait before I could even reread your post.

Not sure this is the right place to continue this discussion, but here are some additional poitns:

I'm happy to have the discussion here since then it's properly on the record. I should perhaps add to the CC list Volker and Leif since they helped with the autotoolification earlier this year.

OK, I'll do that. It makes sense.

This can be automated using some Makefile loops.

No problem, it only needs doing once.

  • about the all target in the progs dir, your first dependencies includes all programs, and then you got Automake PROGRAMS stuff defined depending on the allprogs configuration flag, see below for further comments.

Two additional questions:

  • when allprogs is disabled, only the library and mwrank are installed, but everything is built. Is this intended? On solw platform (let's say Cygwin...) it would be nice to only build what is installed (and so remove the noinst_PROGRAMS stuff). I see the noinst was added in commit 93229fe24a35, could you elaborate on that? has it something to do with check targets?

This should definitely be possible. I only added the allprogs thing later on, specifically for Sage where the only binary used is mwrank. So , progs that should be the only binary built (when allprogs is disabled).

  • all your tests in the check targets are really similar, except for the executable name of course, do you plan on refatoring that?

Do you mean, by some sort of loop, or by clever wildcard targets in the Makefile? Would you fancy writing something suitable? Otherwise it does not seem very urgent (and they are similar, but not identical...)

I'll work more on this tomorrow.

comment:44 in reply to: ↑ 43 ; follow-ups: Changed 7 years ago by jpflori

Replying to cremona: /EXEEXT.html#EXEEXT

So I guess you unfortunately really have to add it to the PROCS_TESTS and similar variables.

OK, I'll do that. It makes sense.

Replacing

PROCS_PROGS = solve_conic solve_legendre reduce_cubics list_cubics
procs_progs: $(PROCS_PROGS)

by

PROCS_PROGS = solve_conic solve_legendre reduce_cubics list_cubics
procs_progs: $(addsuffix $(EXEEXT), $(PROCS_PROGS))

everywhere needed should do the trick.

is built. Is this intended? On solw platform (let's say Cygwin...) it would be nice to only build what is installed (and so remove the noinst_PROGRAMS stuff). I see the noinst was added in commit 93229fe24a35, could you elaborate on that? has it something to do with check targets?

This should definitely be possible. I only added the allprogs thing later on, specifically for Sage where the only binary used is mwrank. So , progs that should be the only binary built (when allprogs is disabled).

Great! I guess this implies removing noinst_PROGRAMS but also some dependencies for the all target:

progs: procs_progs qcurves_progs qrank_progs g0n_progs

which would become

progs: $(bin_PROGRAMS)

if that works, or something like

if ALLPROGS
PROGS = $(PROCS_PROGS) $(QCURVES_PROGS) $(QRANK_PROGS) $(G0N_PROGS)
else
PROGS = mwrank$(EXEEXT)
endif

progs: $(PROGS)
  • all your tests in the check targets are really similar, except for the executable name of course, do you plan on refatoring that?

Do you mean, by some sort of loop, or by clever wildcard targets in the Makefile? Would you fancy writing something suitable? Otherwise it does not seem very urgent (and they are similar, but not identical...)

For all the programs tested in a similar way, you could replace

check_qrank: qrank_progs
	@echo Checking qrank programs...
	./mwrank < $(test_input_dir)/mwrank.in > mwrank.testout 2>/dev/null && diff mwrank.testout $(test_output_dir)/mwrank.out
	./reduce_quartics < $(test_input_dir)/reduce_quartics.in > reduce_quartics.testout 2>/dev/null && diff reduce_quartics.testout $(test_output_dir)/reduce_quartics.out
	./quartic_points < $(test_input_dir)/quartic_points.in > quartic_points.testout 2>/dev/null && diff quartic_points.testout $(test_output_dir)/quartic_points.out
	rm -f PRIMES 1

by something like

check_run = ./$(prog)$(EXEEXT) < $(test_input_dir)/$(prog).in > $(prog).testout 2>/dev/null && diff $(prog).testout $(test_output_dir)/$(prog).out || exit $$?

check_qrank: qrank_progs
	@echo Checking qrank programs...
	$(foreach prog, $(PROCS_PROGS), $(check_run))
	rm -f PRIMES 1

Or, potentially better cause it can use parallelism, using Makefile dependencies

check_run = ./$<$(EXEEXT) < $(test_input_dir)/$<.in > $<.testout 2>/dev/null && diff $<.testout $(test_output_dir)/$<.out || exit $$?

%_RUN: %$(EXEEXT)
	$(check_run)

qrank_progs_RUN: $(addsuffix _RUN, $(QRANK_PROGS))

check_qrank: qrank_progs qrank_progs_RUN
	rm -f PRIMES 1

provided it works.

And for parts were not every executable is tested in this way, you could create a secondary variable

QRANK_PROGS_TEST = ...

including only the ones to be tested and use it instead of

QRANK_PROGS

directly.

I'll test such ideas tonight.

comment:45 in reply to: ↑ 44 Changed 7 years ago by jpflori

Replying to jpflori:

I guess this implies removing noinst_PROGRAMS but also some dependencies for the all target:

progs: procs_progs qcurves_progs qrank_progs g0n_progs

which would become

progs: $(bin_PROGRAMS)

if that works,

In fact, I think you can just remove the all targets... The program included in bin_PROGRAMS will be built anywat thanks to autotools.

comment:46 in reply to: ↑ 44 Changed 7 years ago by jpflori

Replying to jpflori:

Replacing

PROCS_PROGS = solve_conic solve_legendre reduce_cubics list_cubics
procs_progs: $(PROCS_PROGS)

by

PROCS_PROGS = solve_conic solve_legendre reduce_cubics list_cubics
procs_progs: $(addsuffix $(EXEEXT), $(PROCS_PROGS))

everywhere needed should do the trick.

Hum, Automake does not like GNU make extensions...

comment:47 Changed 7 years ago by jpflori

Some other comments:

  • there seems to be some inconsistencies in the comments about the progs/tests not checked, and in tests h1degphi built but not checked
  • could you make the distinction between progs and tests clearer (to me! sorry, I'm not really used to eclib and mwrank...): are progs really progs for computations and tests only testing which have no real interest by themselves?
  • in the tests directory d2 is put in two targets

Changed 7 years ago by jpflori

Patch to am files.

comment:48 Changed 7 years ago by jpflori

Here is a patch which seems more or less functional in Linux at least (except that autoreconf -i rants about GNU extensions...).

A couple of remarks:

  • the programs in test are only built for make check
  • in progs, when make check is run all programs are built and tested even when -disable-allprogs was passed, thats maybe not the most sensible choice
  • as mentionned above, some comments about files checked should be modified, and some other added
  • I've forgot to remove some file which should not be tested, so it fails at some point

comment:49 follow-up: Changed 7 years ago by cremona

OK, I have the patch and will try it out, expect to update the source tarball accordingly (I will report back).

To answer your (reasonable) questions: yes, the intention is that binaries which are only used for testing are in tests, whle ones which people might actually want to run are in progs. It's not an absolute distinction: one of the "tests" is a perfectly good interactive program which prompts for an elliptic curve and outputs its conductor; but these days I would not expect anyone to use this program for that as they can get conductors from Sage (or Magma or pari), so it has the status of a test that the conductor-computing code (which definitely is used elsewhere in the library) is correct.

Secondly, in progs, the only program used in Sage is mwrank; there are some others here which could easily be used in Sage, though wrapping the relavant library functions would be better. But anyone using eclib outside of Sage (including me, installing it on various machines) needs more than that, and these other prgrams are in progs.

Inconsistencies are no great surprise given that I completely rewrote this back in March/April?, and (as you have observed) changed my mind a bit since then as to what should go where. I'll put in a test for h1degphi (though this is actually testing some functionality which I no longer use so I might decide to remove it instead). I'll also put in a test for d2. I think that the ones listed in extra_progs are the test programs which I build but do not yet have test data for; I will sort that out. [Note that until I started repackaging this code for Sage back in 2007 I did not have any standard test data at all; this is one good habit Sage has taught me!]

comment:50 in reply to: ↑ 49 Changed 7 years ago by jpflori

Replying to cremona:

To answer your (reasonable) questions: yes, the intention is that binaries which are only used for testing are in tests, whle ones which people might actually want to run are in progs. It's not an absolute distinction: one of the "tests" is a perfectly good interactive program which prompts for an elliptic curve and outputs its conductor; but these days I would not expect anyone to use this program for that as they can get conductors from Sage (or Magma or pari), so it has the status of a test that the conductor-computing code (which definitely is used elsewhere in the library) is correct.

I see, thanks for the explanation.

Secondly, in progs, the only program used in Sage is mwrank; there are some others here which could easily be used in Sage, though wrapping the relavant library functions would be better. But anyone using eclib outside of Sage (including me, installing it on various machines) needs more than that, and these other prgrams are in progs.

About the fact that only mwrank is installed in Sage, after thinking a little bit about that, I'm not convinced anymore that providing an option in your build system only to please Sage is really relevant.

I mean that if at some point someone decides to integrate more of them, you'll have to modify your build system once more. Whereas having everything installed by default is not so painful, that's just a few more kB which are insignificant in comparison with Sage's current size.

Inconsistencies are no great surprise given that I completely rewrote this back in March/April?, and (as you have observed) changed my mind a bit since then as to what should go where. I'll put in a test for h1degphi (though this is actually testing some functionality which I no longer use so I might decide to remove it instead). I'll also put in a test for d2. I think that the ones listed in extra_progs are the test programs which I build but do not yet have test data for; I will sort that out. [Note that until I started repackaging this code for Sage back in 2007 I did not have any standard test data at all; this is one good habit Sage has taught me!]

About the tests, it might be a good idea to actually test that the output of diff is empty (with wc or something like that) and error out if its not.

comment:51 follow-up: Changed 7 years ago by cremona

Thanks for the comments. I can avoit the warning about GNU make extesion for the addsuffix command by using (for example)

PROCS_PROGS_SUFFIXED = $(PROCS_PROGS:=$(EXEEXT))

instead. I have not yet found a way to avoid your (very neat) checkrun macro together with foreach. I think I will leave that in and wait for someone to aomplain (there usually is someone, if some non-POSIX thing is used).

I have added test input/output for the remaining programs in progs. In view of your last comments I will not stop that other programs from building wnen allprogs is not set: it would need changing the working of "make check" too, and I think that something useful is being served by building and testing these programs in progs as well as the pure tests. And I will make similar changes to the Makefile in tests as you did in progs, including adding test input/output for the few which do not yet have them.

Good point about checking the diff output; I will try that.

comment:52 follow-up: Changed 7 years ago by cremona

PS regarding diff: I am using the fact that the exit status is 0 iff the files are the same. What could be better?

comment:53 in reply to: ↑ 52 Changed 7 years ago by jpflori

Replying to cremona:

PS regarding diff: I am using the fact that the exit status is 0 iff the files are the same. What could be better?

I think that's perfect, it's just that I was not aware of that.

comment:54 in reply to: ↑ 51 Changed 7 years ago by jpflori

Replying to cremona:

Thanks for the comments. I can avoit the warning about GNU make extesion for the addsuffix command by using (for example)

PROCS_PROGS_SUFFIXED = $(PROCS_PROGS:=$(EXEEXT))

instead. I have not yet found a way to avoid your (very neat) checkrun macro together with foreach. I think I will leave that in and wait for someone to aomplain (there usually is someone, if some non-POSIX thing is used).

I have added test input/output for the remaining programs in progs. In view of your last comments I will not stop that other programs from building wnen allprogs is not set: it would need changing the working of "make check" too, and I think that something useful is being served by building and testing these programs in progs as well as the pure tests. And I will make similar changes to the Makefile in tests as you did in progs, including adding test input/output for the few which do not yet have them.

Oh, I somehow failed to included such changes in the patch I uploaded here. As you noted, the changes should be similar to those for progs.

Just one remark, for the programs to be built in test, you can use check_PROGRAMS which ensures that they are only built for make check, rather that noinst_PROGRAMS.

Good point about checking the diff output; I will try that.

comment:55 Changed 7 years ago by cremona

OK, I will try using check_PROGRAMS, good idea.

One side-effect of using the loop (foreach...) for the tests is that instead of each test being visible on a separate line they are all run together with ";" separating them (which I see you included, as it does not work without). That does not look good. I have a version in which first check_run is changes slightly replacing $(prog) by $${prog}, and then instead of the foreach I have

for prog in $(PROCS_TESTS); do	$(check_run); done

which works --it gives no output at all when the tests pass except echoing the whole input line:

jec@fermat%(cd tests; make check)
Running procs checks...
for prog in smattest comptest rattest bigrattest ptest mptest tbessel mvectest mmattest mspace thilbert tp2points tilll vectest1 vectest2 mattest1 mattest2 space1 space2 svectest1 svectest2 tcon2 tlegcert; do	./${prog} < ./in/${prog}.in > ${prog}.testout 2>/dev/null && diff ${prog}.testout ./out/${prog}.out || exit $?; done

-- but I do not know if this shell loop would work on non-linux (or even non-bash) systems. Do you know?

comment:56 Changed 7 years ago by jpflori

I think (and hope) that for loops are part of POSIX spec for shells, just as if/then/fi, or a POSIX compliant shell would not offer much scripting capabilities.

If you're on a Debian (or derivated) system, dash which is quite minimal should be installed by default. To be sure it is used by make (for testing purpose), I think you can define SHELL or sthg like that in the Makefile.

comment:57 Changed 7 years ago by jpflori

If the executed line is not printed anymore, it would be nice to modify the "
exit" part to print the culprit name.

Or prefixing echo Testing $${prog} to check_run.

comment:58 Changed 7 years ago by cremona

This has taken a long time but I think I have fixed all the issues raised so far. I moved some programs between tests and progs, unified all the tests so they can be run with a loop, including adding tests for the programs which did not. (One of these was not trivial since the program prompts for the filename of a file of input data and it was not easy to get that to work even with "make distcheck" -- but it does!).

There are no more uses of addsuffix() or foreach() but I do rely on loops in Makefile rules,so do not know whether these will run properly on other systems.

There's a new tarball at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/

comment:59 follow-ups: Changed 7 years ago by jpflori

Looks really nice.

In tests dir, you forgot to replace LDADD by LIBS and the dependencies of check targets by binaries names with EXEEXT appended (it should work as is, even on Cygwin, but for a bad reason as mentioned before).

And unfortunately, I don't think the echo -e command is portable. For example it will behave differently on bash and dash. (On of them will just print -e and will not interpret the \c).

Finally, as I'm a little confused by your answer about what programs in progs are built/check, I did not suggest not to check every program when allprogs is not set and make check is run.

In fact I rather suggested to always build and install (and check) every program, i.e. remove the allprogs option. Hence, in the future, if Sage uses more programs from your package it wont need further modification (and it will only take a little more time, and a tiny amount of memory, when built for Sage).

If you want to keep the option, i suggest you add the programs different from mwrank in check_PROGRAMS when the option is disabled (I'd say you could even include mwrank for simplicity, to use the already defined variables). That way, automake will generate its magic targets to build the executables (Ive not tested yet, but Id say the generated Makefile won't include anything specific currently and they will be built generically... and surely fail).

comment:60 in reply to: ↑ 59 Changed 7 years ago by jpflori

Replying to jpflori:

If you want to keep the option, i suggest you add the programs different from mwrank in check_PROGRAMS when the option is disabled (I'd say you could even include mwrank for simplicity, to use the already defined variables). That way, automake will generate its magic targets to build the executables (Ive not tested yet, but Id say the generated Makefile won't include anything specific currently and they will be built generically... and surely fail).

In fact automake always generates its magic for all file because their all in one of the bin_PROGRAMS, its just the one actually built which change. Anyway, using check_PROGRAMS cannot hurt.

comment:61 in reply to: ↑ 59 Changed 7 years ago by cremona

Replying to jpflori:

Looks really nice.

Thanks.

In tests dir, you forgot to replace LDADD by LIBS and the dependencies of check targets by binaries names with EXEEXT appended (it should work as is, even on Cygwin, but for a bad reason as mentioned before).

Fixed.

And unfortunately, I don't think the echo -e command is portable. For example it will behave differently on bash and dash. (On of them will just print -e and will not interpret the \c).

Fixed. The testing just outputs the test name (one per line) as it goes, with no "done" at the end since that would go on a separate line which did not look good. So, no news is good news for test output.

Finally, as I'm a little confused by your answer about what programs in progs are built/check, I did not suggest not to check every program when allprogs is not set and make check is run.

Fixed (see below)

In fact I rather suggested to always build and install (and check) every program, i.e. remove the allprogs option. Hence, in the future, if Sage uses more programs from your package it wont need further modification (and it will only take a little more time, and a tiny amount of memory, when built for Sage).

If you want to keep the option, i suggest you add the programs different from mwrank in check_PROGRAMS when the option is disabled (I'd say you could even include mwrank for simplicity, to use the already defined variables). That way, automake will generate its magic targets to build the executables (Ive not tested yet, but Id say the generated Makefile won't include anything specific currently and they will be built generically... and surely fail).

I have kept the allprogs option (set to true by default). When false: "make" builds the library and mwrank only (nothing in tests, nor the other prgrams in progs) while "make check" builds and tests everything. When true: "make" builds the library and all programs in progs (nothing in tests) while "make check" builds and tests everything.

There's anew tarball at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-2012-08-24.tar.gz and I am about to push these changes to https://code.google.com/p/eclib/ .

comment:62 follow-up: Changed 7 years ago by jpflori

Great. Could you tag a release on the repo? Then I'll repackage it in a spkg.

comment:63 in reply to: ↑ 62 Changed 7 years ago by cremona

Replying to jpflori:

Great. Could you tag a release on the repo?

Done (2012-08-24)

Then I'll repackage it in a spkg.

Thanks. Of course I'll help test, especially if there are some changes needed in Sage.

comment:64 Changed 7 years ago by jpflori

The spkg at http://perso.telecom-paristech.fr/~flori/sage/eclib-20120824.spkg built and passed its tests on Linux. It also built on Cygwin but got stuck while testing smattest at the end of the test during the rank computation. I don't think the later failure should be a blocker, the situation is already much better on Cygwin, and the problem could have another origin (some dependency being broken or I don't know what). And Cygwin is not supported officialy anyway.

comment:65 follow-up: Changed 7 years ago by jpflori

  • Description modified (diff)
  • Report Upstream changed from Workaround found; Bug reported upstream. to Fixed upstream, in a later stable release.

comment:66 in reply to: ↑ 65 Changed 7 years ago by cremona

Replying to jpflori:

The spkg installs OK for me but devel/sage/module_list.py needs editing (jc --> ec).

comment:67 Changed 7 years ago by jpflori

  • Status changed from needs_review to needs_work
  • Work issues set to take name chang into account

Oh yes, of course, I did not try to rebuild the Sage library itself yet, but that's definitely needed!

comment:68 follow-up: Changed 7 years ago by jpflori

About the spkg changes:

  • I've removed the Padus thing, I could not make it point to anything useful with Google. I guess it used to be a maintainer of the spkg, is that correct?
  • I think the autotools build system should have removed all GNU make specificities (especially thanks to John not using foreach etc. :), this could be confirmed on Solaris or such systems I guess, I don't have access to such systems though.

About the module_list.py modification:

  • I'd like to check the need for -lpari on Cygwin mentioned in comments by leif in lines concerning mwrank et al, before we commit it changes to the file.

comment:69 in reply to: ↑ 68 Changed 7 years ago by cremona

Replying to jpflori:

About the spkg changes:

  • I've removed the Padus thing, I could not make it point to anything useful with Google. I guess it used to be a maintainer of the spkg, is that correct?

I don't know who that was but I'm sure it is best removed. It was put there years ago, certainly not by me.

  • I think the autotools build system should have removed all GNU make specificities (especially thanks to John not using foreach etc. :), this could be confirmed on Solaris or such systems I guess, I don't have access to such systems though.

Nor me. We'll see.

About the module_list.py modification:

  • I'd like to check the need for -lpari on Cygwin mentioned in comments by leif in lines concerning mwrank et al, before we commit it changes to the file.

I changed 'jc' to 'ec' in 3 places in that file but even after 'sage -ba' it didn't work properly. I'll try again today.

Changed 7 years ago by jpflori

Spkg diff, for review only. Committed in spkg.

comment:70 Changed 7 years ago by jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues take name chang into account deleted

I've updated module_list.py to take the name change into account. I've also removed Leif comment about pari and leave it in the libraries dependencies. Indeed, this is needed if we try to link the concerned modules against a static version of eclib. This triggers the need of a static version of pari and the corresponding dependency, and that not only on Cygwin.

In a perfect world, this will never be needed because everything will properly build as shared libraries, but we never know what will happen. Until now eclib was only build static on Cygwin, so the problem surfaced, now it is also shared, but things may change and I don't think the inclusion of pari hurts enough to be removed right now.

And I think this is now needs_review

comment:71 follow-up: Changed 7 years ago by cremona

I don't think I can be the one to review this being responsible for most of the upstream changes, and in any case I don't have a cygwin system to test on. But in any case I have built the new spkg successfully (on top of 5.3.beta1 on 64-bit ubuntu linux), applied the patch and am now doing a full test which I'll report back on.

comment:72 in reply to: ↑ 71 Changed 7 years ago by cremona

Replying to cremona:

I don't think I can be the one to review this being responsible for most of the upstream changes, and in any case I don't have a cygwin system to test on. But in any case I have built the new spkg successfully (on top of 5.3.beta1 on 64-bit ubuntu linux), applied the patch and am now doing a full test which I'll report back on.

There were several strange failures all complaining that -ljc could not be found, even after applying the patch and remembering sage -b. I am tring again after sage -ba. Is there anywhere else these library names are referred to?

comment:73 follow-up: Changed 7 years ago by jpflori

That's strange. Can you make sure the patch to the Sage library is correctly applied (stupid question, but...) by looking at module_list.py and searching for "jc"? On my Linux system with 5.3.rc0 plus this ticket only, everything looks fine.

comment:74 in reply to: ↑ 73 Changed 7 years ago by cremona

Replying to jpflori:

That's strange. Can you make sure the patch to the Sage library is correctly applied (stupid question, but...) by looking at module_list.py and searching for "jc"?

It's fine, there is no jc anywahere and grep -w ec module_list.py gives the correct 4 matches,

On my Linux system with 5.3.rc0 plus this ticket only, everything looks fine.

Here is a sample failure:

sage -t -long devel/sage-main/sage/structure/misc.pyx
**********************************************************************
File "/home/jec/sage-5.3.beta1/devel/sage-main/sage/structure/misc.pyx", line 244:
    sage: cython("cdef class A:\n    cdef public int a")
Exception raised:
    Traceback (most recent call last):
      File "/home/jec/sage-5.3.beta1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/home/jec/sage-5.3.beta1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/home/jec/sage-5.3.beta1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_5[7]>", line 1, in <module>
        cython("cdef class A:\n    cdef public int a")###line 244:
    sage: cython("cdef class A:\n    cdef public int a")
      File "cython_c.pyx", line 61, in sage.misc.cython_c.cython (sage/misc/cython_c.c:779)
      File "/home/jec/sage-5.3.beta1/local/lib/python/site-packages/sage/server/support.py", line 473, in cython_import_all
        create_local_c_file=create_local_c_file)
      File "/home/jec/sage-5.3.beta1/local/lib/python/site-packages/sage/server/support.py", line 450, in cython_import
        **kwds)
      File "/home/jec/sage-5.3.beta1/local/lib/python/site-packages/sage/misc/cython.py", line 539, in cython
        raise RuntimeError, "Error compiling %s:\n%s\n%s"%(filename, log, err)
    RuntimeError: Error compiling /home/jec/.sage//temp/fermat/6134//tmp_0.spyx:
    running build
    running build_ext
    building '_home_jec__sage_temp_fermat_6134_tmp_0_spyx_0' extension
    creating build
    creating build/temp.linux-x86_64-2.7
    gcc -fno-strict-aliasing -fwrapv -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/home/jec/sage-5.3.beta1/local/include/csage/ -I/home/jec/sage-5.3.beta1/local/include/ -I/home/jec/sage-5.3.beta1/local/include/python2.7/ -I/home/jec/sage-5.3.beta1/local/lib/python2.7/site-packages/numpy/core/include -I/home/jec/sage-5.3.beta1/devel/sage/sage/ext/ -I/home/jec/sage-5.3.beta1/devel/sage/ -I/home/jec/sage-5.3.beta1/devel/sage/sage/gsl/ -I/home/jec/.sage//temp/fermat/6134 -I/home/jec/sage-5.3.beta1/local/include/python2.7 -c _home_jec__sage_temp_fermat_6134_tmp_0_spyx_0.c -o build/temp.linux-x86_64-2.7/_home_jec__sage_temp_fermat_6134_tmp_0_spyx_0.o -w -O2
    creating build/lib.linux-x86_64-2.7
    gcc -pthread -shared -L/home/jec/sage-5.3.beta1/local/lib build/temp.linux-x86_64-2.7/_home_jec__sage_temp_fermat_6134_tmp_0_spyx_0.o -L/home/jec/sage-5.3.beta1/local//lib/ -L/home/jec/sage-5.3.beta1/local/lib -lmpfr -lgmp -lgmpxx -lstdc++ -lpari -lm -ljc -lgsl -lgslcblas -latlas -lntl -lcsage -lpython2.7 -o build/lib.linux-x86_64-2.7/_home_jec__sage_temp_fermat_6134_tmp_0_spyx_0.so -L/home/jec/sage-5.3.beta1/local//lib

    /usr/bin/ld: cannot find -ljc
    collect2: ld returned 1 exit status
    error: command 'gcc' failed with exit status 1

showing that it's the linker complaining.

I do not know why during a test any compilation like this needs to be done: recall that I had previously done sage -ba.

comment:75 follow-up: Changed 7 years ago by jpflori

My bad, there are indeed failing doctests. I'll correct that ASAP.

comment:76 Changed 7 years ago by jpflori

  • Status changed from needs_review to needs_work
  • Work issues set to fix failing doctests

comment:77 in reply to: ↑ 75 Changed 7 years ago by cremona

Replying to jpflori:

My bad, there are indeed failing doctests. I'll correct that ASAP.

If that means that you understand what is causing this, very good! I was about to put out a query to sage-devel.

comment:78 Changed 7 years ago by jpflori

I think its only the cython magic command in sage which by default use a bunch of libraries including jc (which now should be ec). Recompiling right now to check I'm correct.

comment:79 Changed 7 years ago by jpflori

About the smattest issue on Cygwin, it seems to get stuck in smat_elim.cc in the step3 function. Not sure where exactly yet (nor why!). And to be clear, I don't think this should be a blocker for the current ticket (unless its solved in a couple of days...), but I'm reporting here because I mentioned the issue here earlier.

comment:80 Changed 7 years ago by jpflori

Here comes an updated patch for the remaining jc issue.

Changed 7 years ago by jpflori

Patch to Sage library.

comment:81 Changed 7 years ago by jpflori

  • Status changed from needs_work to needs_review
  • Work issues fix failing doctests deleted

comment:82 Changed 7 years ago by cremona

Well done! I am retesting now.

I have no ideas about the smat_elim issue, but that function does have a while loop which I know must terminate....without a way to debug things in cygwin it is hard to know how to proceed on that. Good job it is not a blocker.

comment:83 follow-up: Changed 7 years ago by jpflori

I think I got the smattest issue. This must be a type length issue (and several warning about overflow in xmod.h during compilation should have made me realize it earlier).

In smat_elim.cc, if I use the "works fine" code in xmm0 rather than the David Harvey's one, the test passes.

I think the problem is with INV_BIGPRIME being defined as long, which is 32 bits here on Cygwin I guess and does not fit (1<<33)+140!) rather than int64_t, whence the warnings during compilation. I'm recompiling right now and will retest. If that's indeed the problem, that should not be restricted to Cygwin but anywhere where long is only 32 bits.

comment:84 in reply to: ↑ 83 Changed 7 years ago by cremona

Replying to jpflori:

I think I got the smattest issue. This must be a type length issue (and several warning about overflow in xmod.h during compilation should have made me realize it earlier).

In smat_elim.cc, if I use the "works fine" code in xmm0 rather than the David Harvey's one, the test passes.

I think the problem is with INV_BIGPRIME being defined as long, which is 32 bits here on Cygwin I guess and does not fit (1<<33)+140!) rather than int64_t, whence the warnings during compilation. I'm recompiling right now and will retest. If that's indeed the problem, that should not be restricted to Cygwin but anywhere where long is only 32 bits.

That makes a lot of sense. It was a recent change, and I clearly have not built & tested on my 32-bit laptop recently. (I will do in future, I promise!). I take it that the solution is to redefine that variable as int64_t?

comment:85 follow-up: Changed 7 years ago by jpflori

You should indeed set INV_BIGPRIME to be an int64_t, but also ab in the xmm0 function in smat_elim.cc. This seems to do the trick (only modifying INV_BIGPRIME was not enough).

I'm rebuilding and testing everything now.

comment:86 in reply to: ↑ 85 Changed 7 years ago by cremona

Replying to jpflori:

You should indeed set INV_BIGPRIME to be an int64_t, but also ab in the xmm0 function in smat_elim.cc. This seems to do the trick (only modifying INV_BIGPRIME was not enough).

I'm rebuilding and testing everything now.

Done and all long tests pass. As far as I am concerned this deserves a positive review (for the new spkg to replace the old one + the patch for two files just to take into account the change of the library name from jc to ec).

comment:87 Changed 7 years ago by jpflori

And I'm only getting numerical noise for tbessel in the tests directory:

Enter x: x = 1.23     K0(x)=....634[2]     K1(x) = ...062

the ending "2" only appearing in one of the outputs.

comment:88 Changed 7 years ago by jpflori

To be more precise, it appears in the expected output, but not in the one spit out on Cygwin.

comment:89 Changed 7 years ago by jpflori

In the progs directory, nfhpmcurve seems broken. In particular, it returns 3 [0,...,0] instead of 3 [1,-1,1,17,19] and then loops.

comment:90 Changed 7 years ago by jpflori

It seems the construction of the elliptic curve from c_4, c_6 fails.

comment:91 Changed 7 years ago by jpflori

The problem is fact much earlier. First hint, it tries to compute 4 curves instead of 3, having found 4 newforrms at level 2002.

comment:92 follow-up: Changed 7 years ago by jpflori

There is a problem with the dimension of subspaces corresponding to eigenvalues.

comment:93 in reply to: ↑ 92 Changed 7 years ago by cremona

Replying to jpflori:

There is a problem with the dimension of subspaces corresponding to eigenvalues.

Very sorry for the hassle you are having here. I am currently building eclib on my 32-bit laptop (ubuntu 12.04) in case there are any other 32/64-bit issues. In fact I am already seeing a lot of warnings "overflow in implicit constant conversion [-Woverflow]" from the included file xmod.h (in line 114, i.e. where INV_BIGPRIME is defined, as you found) which certainly needs to be fixed. Wait for me before spending more time (though I only have another half hour today as I'll be going out shortly -- to see Shakespeare's Comedy of Errors, so very appropriate!).

All the things you report could be explained by some overflow in low-level arithmetic, it will not be as serious as it looks.

comment:94 Changed 7 years ago by jpflori

I think so. The fix I proposed is not sufficient, setting back once again the old "this works" code in xmm0 function from smat_elim solves the problem.

comment:95 Changed 7 years ago by jpflori

Setting r to be int64_t rather than long as well make the tests pass!

comment:96 Changed 7 years ago by jpflori

Apart from that, I get a little bit of what seems to be numerical noise in h1bsd.

comment:97 Changed 7 years ago by jpflori

So I'll leave you fix the int64_t and floating point issues, tag a release and update the spkg here if that's OK with you (it took less than a couple of days :).

comment:98 Changed 7 years ago by cremona

I agree, I had just written:

For me, defining INV_BIGPRIME to be int64_t and also in smat_elim.cc's xmm0() putting back the old "this works" code resulted in everything passing except numerical noise in tbessel and h1bsd (both of which are new tests added last week). I am now trying your suggestion to make the variable r in xmm0() also into int64_t.

I'll fix the remaining issues tomorrow.

comment:99 Changed 7 years ago by jpflori

Don't forget to let "ab" be int64_t as well. (And with the "old" code, it seems INV_BIGPRIME is not used at all, but for making the new code work, this is definitely needed).

comment:100 Changed 7 years ago by cremona

I'm having a problem here. I could not bring my laptop in to work so have instead been working on my 32-bit desktop (a machine I do not usually do development work on, running 32-bit Suse Enterprise). I built recent pari and NTL on the machine but could not at first compile eclib, on account of that constant INV_BIGPRIME on line 114 of xmod.h. It (GCC 4.3.4) tells me that it is too big to convert to a long, with no difference if I decalre it as a long or int64_t or long long. The solution is to append LL to the constant.

Sorry for the delay in updating the distribution... I still have to sort out the two numerical fuzz issues.

comment:101 Changed 7 years ago by cremona

Updated sources at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/progs/ should fix the 32-bit issues.

comment:102 Changed 7 years ago by jpflori

Great!

I'll test it in a 32 bits virtual machine this afternoon, and on Cygwin tonight.

comment:103 follow-up: Changed 7 years ago by jpflori

  • Status changed from needs_review to needs_work
  • Work issues set to Repackage 2012-08-30 version

I'm finally testing the spkg. By the way, it seems you tagged the wrong commit on your google code repo.

comment:104 in reply to: ↑ 103 Changed 7 years ago by cremona

Replying to jpflori:

I'm finally testing the spkg. By the way, it seems you tagged the wrong commit on your google code repo.

There is something about tagging which I do not understand, including whether to tag before or after doing something. Anyway, the last version on googlecode is the right one, and is also available as a tarball, so ignore any tags.

comment:105 Changed 7 years ago by jpflori

Don't worry, your hg history is not included in the dist tarball anyway, so it does not hurt here at all.

In fact you should tag a release after committing your changes: it will more or less create a new special commit telling that the previous one (you can also give an explicit commit number IIRC) corresponds to such release.

comment:106 Changed 7 years ago by jpflori

Hopefully last remark on the new build system. It tries to rebuild the test executables after the tests were run (and does nothing because everything is uptodate of course). No time to look at it right now.

comment:107 follow-up: Changed 7 years ago by jpflori

Ok, I guess that's just because the Automake magic check build of the test programs (which is before in the Makefile) is treated by make after the one you defined which builds the programs (which is fortunate) and then tests them. Not sure if anything can be done, but that's not really a problem anyway.

comment:108 in reply to: ↑ 107 Changed 7 years ago by cremona

Replying to jpflori:

Ok, I guess that's just because the Automake magic check build of the test programs (which is before in the Makefile) is treated by make after the one you defined which builds the programs (which is fortunate) and then tests them. Not sure if anything can be done, but that's not really a problem anyway.

I am glad that is your conclusion, as I thought this was harmless and it's not too obvious how to avoid it. As you say, the created Makefile does things which have already been done by my manual targets. Let's leave it.

comment:109 Changed 7 years ago by jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues Repackage 2012-08-30 version deleted

I can confirm the updated eclib version passes all its tests in a 32 bits Ubuntu 12.04 virtual machine (inside a 64 bits Ubuntu 12.04).

Updated spkg at http://perso.telecom-paristech.fr/~flori/sage/eclib-20120830.spkg

Changed 7 years ago by jpflori

Spkg diff, for review only. Committed in spkg.

comment:110 Changed 7 years ago by jpflori

Now with updated SPKG.txt

comment:111 Changed 7 years ago by jpflori

  • Description modified (diff)

comment:112 Changed 7 years ago by kcrisman

This works on Cygwin on XP!

comment:113 Changed 7 years ago by kcrisman

Please let's get this in. I can't currently test it on Win 7, but maybe if I quick do a thing on Mac later, that would be ok. Definitely needed on Cygwin.

comment:114 Changed 7 years ago by cremona

What needs to be done before there's a positive review? More checking on Cygwin (which I cannot help with) or what? Do we need more people to test jpflori's spkg on other architectures?

comment:115 follow-up: Changed 7 years ago by kcrisman

This works on Mac. John, could you check on some kind of Linux? I think that's enough for positive review. Then the buildbot should do the rest once Jeroen merges this...

Last edited 7 years ago by kcrisman (previous) (diff)

comment:116 Changed 7 years ago by jpflori

It is ok on Windows 7. I think the main point is to someone to look at the spkg diff. Anyway, most of the work has been done by John upstream (or let's say integrated for the few things I suggested if there are any), so the spkg diff is quite minimal (if there is any, I don't really remember). So this should really be trivial.

I don't think "testing" on "exotic" (let's say non linux) architecture is still needed.

comment:117 in reply to: ↑ 115 ; follow-up: Changed 7 years ago by cremona

Replying to kcrisman:

This works on Mac. John, could you check on some kind of Linux? I think that's enough for positive review. Then the buildbot should do the rest once Jeroen merges this...

Testing the spkg on 5.4.1 on linux (64-bit ubuntu), will report back.

comment:118 in reply to: ↑ 117 Changed 7 years ago by jpflori

  • Description modified (diff)

Replying to cremona:

Replying to kcrisman:

This works on Mac. John, could you check on some kind of Linux? I think that's enough for positive review. Then the buildbot should do the rest once Jeroen merges this...

Testing the spkg on 5.4.1 on linux (64-bit ubuntu), will report back.

Nor testing on Linux, but that won't hurt :)

What I think really matters is for someone to look at trac_13325.patch and say "ok, this makes sense, let's merge this".

I think that even John could put himself as reviewer here as he worked on the "upstream" side of this ticket (and I agree with all the great work he did if he needs some approbation on his side).

comment:119 Changed 7 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

I have looked at trac_13325.patch and I say "ok, this makes sense, let's merge this". I have also applied (built) the new spkg and tested the whole library, with no problems. This was on 64-bit ubuntu linux.

Given this, ans all the commnets above, I am taking the liberty of marking this as "positive review" and hope that buildbots will agree. I hope they are capable of both building the new spkgs and applying the patch, since doing just one will definitely not work.

comment:120 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:121 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.