eclib does not build on Cygwin

[jpflori]

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. attachment: trac_13325.patch
2. the spkg http://www.infres.enst.fr/~flori/sage/eclib-20120830.spkg

Depends on #13333

Upstream: Fixed upstream, in a later stable release.

CC: @kcrisman @dimpase @JohnCremona @vbraun

Component: porting: Cygwin

Keywords: eclib spkg cygwin

Author: Jean-Pierre Flori

Reviewer: John Cremona

Merged: sage-5.6.beta0

Issue created by migration from https://trac.sagemath.org/ticket/13325

[jpflori]

Attachment: 13325.patch.gz

Spkg diff, for review only.

[jpflori]

Author: Jean-Pierre Flori

[jpflori]

comment:1

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.

[jpflori]

comment:2

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.

[jpflori]

comment:3

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.

[JohnCremona]

comment:4

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.

[jpflori]

comment:5

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.

[jpflori]

Work Issues: wait for official update of the build system; fix pari problem (new spkg?)

[jpflori]

Changed upstream from Not yet reported upstream; Will do shortly. to Workaround found; Bug reported upstream.

[jpflori]

comment:7

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

[jpflori]

comment:8

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

[dimpase]

comment:9

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?

[jpflori]

comment:10

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.

[dimpase]

comment:11

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.

[jpflori]

comment:12

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.

[jpflori]

comment:13

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.

[JohnCremona]

comment:14

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.

[jpflori]

comment:17

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

[jpflori]

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

[jpflori]

comment:18

Attachment: eclib.patch.gz

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.

[jpflori]

comment:105

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.

[jpflori]

comment:106

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.

[jpflori]

comment:107

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.

[JohnCremona]

comment:108

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.

[jpflori]

Changed work issues from Repackage 2012-08-30 version to none

[jpflori]

comment:109

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

[jpflori]

Spkg diff, for review only. Committed in spkg.

[jpflori]

comment:110

Attachment: eclib-20120830.patch.gz

Now with updated SPKG.txt

[kcrisman]

comment:112

This works on Cygwin on XP!

[kcrisman]

comment:113

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.

[JohnCremona]

comment:114

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?

[kcrisman]

comment:115

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

[jpflori]

comment:116

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.

[JohnCremona]

comment:117

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.

[jpflori]

comment:118

Replying to @JohnCremona:

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

[JohnCremona]

Reviewer: John Cremona

[JohnCremona]

comment:119

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.

[jdemeyer]

Merged: sage-5.6.beta0