Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that libraries link to the shared version of NTL by default #11635

Closed
kcrisman opened this issue Jul 29, 2011 · 79 comments
Closed

Ensure that libraries link to the shared version of NTL by default #11635

kcrisman opened this issue Jul 29, 2011 · 79 comments

Comments

@kcrisman
Copy link
Member

In #11547, we added copying of several needed dlls on Cygwin. However, in the meantime NTL had a fairly major update (#5731).

From version 5.5, NTL provides a new build system using libtool (although libtool itself is not provided!) to generate shared libraries.
On Cygwin, libtool properly install the shared library as cygntl-?.dll in the bin directory and the import file as libntl.dll.a in the lib directory, together with a libtool file libntl.la and a static version of the library as libntl.a.

In particular, this ensures that the shared version is linked with by default because ld/gcc/g++ find the .dll.a file in the lib directory (which points to the dll file in the bin directory) before the .a file.

Moreover, only moving the previously built libntl.dll to libntl.dll.a was not a solution.
Indeed, the built file hardcoded its own build-time name into itself (as a DT_SONAME on linux), so that libraries linked to it (before or after renaming, does not matter) would in fact search for a file with the build time name... and fail.

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/ntl-5.5.2.p0.spkg

This spkg vastly simplifies the old spkg-install, uses the (slightly patched) upstream build system and includes its own version of libtool (or rather the tools needed to generate it on the fly).

CC: @mwhansen @dimpase @jpflori @jdemeyer

Component: porting: Cygwin

Keywords: cygwin ntl libtool spkg

Author: Jean-Pierre Flori

Reviewer: Karl-Dieter Crisman

Merged: sage-5.6.beta3

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

@kcrisman kcrisman added this to the sage-5.6 milestone Jul 29, 2011
@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

comment:1

New spkg at http://sage.math.washington.edu/home/kcrisman/ntl-5.5.2.p0.spkg. No guarantees on whether this works, or even is necessary (!) but I strongly suspect both. You'll know it works if you do everything at #6743, including this, and then Sage has a segfault on startup.

In case there is any doubt that this is Cygwin-only:

    elif [ $UNAME = "CYGWIN" ]; then
        # Cygwin
        if [ "$CYGPKG" = "yes" ]; then
            if [ -f /lib/libgmp.dll.a ]; then
                ln -s /lib/libgmp.dll.a libgmp.a
            else
                # Added by William Stein on 2006-02-06 so that 
                # can build even if gmp is not built locally
                # i.e., even if using a system-wide gmp.
                # (though it has to be in /usr/lib, which is
                # where it is for cygwin).
                ln  -s /usr/lib/*gmp* .
                ln  -s libgmp.dll.a libgmp.a
            fi 
        else
   	    ln -s "$SAGE_LOCAL/lib/libgmp.a" .
        fi
	$MAKE libntl.dll
        if [ ! -f libntl.dll ]; then
            echo "Error creating ntl shared library."
            exit 1
        fi
	cp libntl.dll "$SAGE_LOCAL/lib/libntl.dll.a"
	cp libntl.dll "$SAGE_LOCAL/lib/libntl.dll"
	cp libntl.dll "$SAGE_LOCAL/bin/libntl.dll.a"
	cp libntl.dll "$SAGE_LOCAL/bin/libntl.dll"
        if [ ! -f "$SAGE_LOCAL/bin/libntl.dll.a" ]; then
            exit 1   # CRUCIAL that we have the dynamic link library
        fi

@kcrisman
Copy link
Member Author

Author: Karl-Dieter Crisman

@kcrisman
Copy link
Member Author

kcrisman commented Aug 1, 2011

comment:3

Interestingly, the most recent alpha I tested all the Cygwin stuff on a few weeks ago already had NTL 5.5.2!

$ ls spkg/installed
<snip>
ntl-5.5.2
<snip>

and it seemed to work fine (in the sense that it got to the Pari segfault when initializing).

That doesn't mean that later in the process things wouldn't happen, but at least for now this ticket can be lower priority, if it's needed at all, since we have to figure out how to get past that error first.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 2, 2011

comment:5

This appears to be the same issue as #12104, which should probably be closed as a duplicate then.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 2, 2011

comment:6

Since the 4.8.alpha3 spkg still is 5.5.2 with no patches, this spkg should still be valid to fix this.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 3, 2011

comment:7

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

@dimpase
Copy link
Member

dimpase commented Dec 3, 2011

comment:8

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

@kcrisman
Copy link
Member Author

kcrisman commented Dec 3, 2011

comment:9

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

No idea! I just copied models from other spkgs. Feel free to only copy and/or make links where actually appropriate. I have never claimed to be anything close to an expert; I've only focused on getting it to work by following other spkgs I've seen.

I would love to have this "correct" so that we never have to worry about it again.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 7, 2011

comment:10

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

No idea! I just copied models from other spkgs. Feel free to only copy and/or make links where actually appropriate. I have never claimed to be anything close to an expert; I've only focused on getting it to work by following other spkgs I've seen.

I would love to have this "correct" so that we never have to worry about it again.

This might even be part of the problem at #9167.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 8, 2011

comment:11

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

Just for posterity (addressing the comments at #12104, perhaps?), here is the rationale for naming it dll.a, from #9050.

This is so that Cygwin will find the shared library before it finds the static library; otherwise, there are lots of segfaults in Sage under Cygwin

@dimpase
Copy link
Member

dimpase commented Dec 8, 2011

comment:12

Replying to @kcrisman:

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

Just for posterity (addressing the comments at #12104, perhaps?), here is the rationale for naming it dll.a, from #9050.

This is so that Cygwin will find the shared library before it finds the static library; otherwise, there are lots of segfaults in Sage under Cygwin

that means that only in the cases when both static and dynamic libraries are built, this is needed.
IMHO there are not so many such cases among Sage spkg's.

@dimpase
Copy link
Member

dimpase commented Dec 9, 2011

comment:13

Replying to @kcrisman:

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

Just for posterity (addressing the comments at #12104, perhaps?), here is the rationale for naming it dll.a, from #9050.

This is so that Cygwin will find the shared library before it finds the static library; otherwise, there are lots of segfaults in Sage under Cygwin

IMHO, this is, at best, a bad hack. In principle, *.dll.a files are import libraries for *.dll libaries,
used by the linker to like against *.dll (i.e. dynamic libraries).

See Buiding/using DLLs

@kcrisman
Copy link
Member Author

kcrisman commented Dec 9, 2011

comment:14

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

Just for posterity (addressing the comments at #12104, perhaps?), here is the rationale for naming it dll.a, from #9050.

This is so that Cygwin will find the shared library before it finds the static library; otherwise, there are lots of segfaults in Sage under Cygwin

IMHO, this is, at best, a bad hack. In principle, *.dll.a files are import libraries for *.dll libaries,
used by the linker to like against *.dll (i.e. dynamic libraries).

Well, as I said in comment:9, I would more than happy to have someone who actually knows what they are talking about to fix this and post a different spkg :) especially if it ended up fixing #9167. Go for it!

@dimpase
Copy link
Member

dimpase commented Dec 14, 2011

comment:15

Replying to @kcrisman:

Replying to @kcrisman:

Oh, and no longer 'needs info', because #12104 shows that it's needed to start Sage.

Why does one need copies in local/bin ?
And why does one need to copy rather than to create a symbolic link?

Just for posterity (addressing the comments at #12104, perhaps?), here is the rationale for naming it dll.a, from #9050.

This is so that Cygwin will find the shared library before it finds the static library; otherwise, there are lots of segfaults in Sage under Cygwin

It's probably OK to have "real" (i.e. dynamic libs, not misnamed dll.a or other files) .dlls in local/bin/, and bad to have them in local/lib/. At least that's what a Cygwin-aware libtool will be doing.

@kcrisman
Copy link
Member Author

comment:16

It's probably OK to have "real" (i.e. dynamic libs, not misnamed dll.a or other files) .dlls in local/bin/, and bad to have them in local/lib/. At least that's what a Cygwin-aware libtool will be doing.

Can you be more specific (if only so I can make a good spkg)? Please check which of the following should be copied, which should be linked (with ln -s, or with other options?), and which are just horrible.

  • lib/libntl.dll
  • lib/libntl.dll.a
  • bin/libntl.dll
  • bin/libntl.dll.a
    Recall that currently we also have
  • lib/libntl.a

@kcrisman
Copy link
Member Author

comment:17

Here's something else potentially relevant.

User 1@GC02635 /home/SageUser/sage-4.7.2
$ ./sage -t -verbose devel/sage/sage/calculus/functional.py
init.sage does not exist ... creating
sage -t -verbose "devel/sage/sage/calculus/functional.py"
Setting permissions of DOT_SAGE directory so only you can read and write it.
<snip LOTS of pretty much identical things>
1564601 [main] python 3544 fork: child 3776 - died waiting for dll loading, errno 11
1639139 [main] python 160 C:\cygwin\home\SageUser\sage-4.7.2\local\bin\python.exe: *** fatal error - unable to remap C:\cygwin\home\SageUser\sage-4.7.2\local\lib\libntl.dll to same address as parent: 0x19A40000 != 0x626C0000
Stack trace:
Frame     Function  Args
002093B8  6102796B  (002093B8, 00000000, 00000000, 00000000)
002096A8  6102796B  (6117EC60, 00008000, 00000000, 61180977)
0020A6D8  61004F1B  (611A7FAC, 6124A1B4, 19A40000, 626C0000)
End of stack trace
1642317 [main] python 3544 fork: child 160 - died waiting for dll loading, errno 11
Traceback (most recent call last):
  File "../.sage/tmp/functional_3772.py", line 6, in <module>
    from sage.all_cmdline import *;
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/all_cmdline.py", line 14, in <module>
    from sage.all import *
<snip>
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py", line 235, in <module>
    from sage.interfaces.all import (singular as singular_default,
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/all.py", line 8, in <module>
    from gap import gap, gap_reset_workspace, gap_console, gap_version, is_GapElement, Gap
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 1188, in <module>
    gap_reset_workspace(verbose=False)
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 1179, in gap_reset_workspace
    g.save_workspace()
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 983, in save_workspace
    self.eval('SaveWorkspace("%s");'%WORKSPACE, allow_use_file=False)
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 374, in eval
    result = Expect.eval(self, input_line, **kwds)
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/expect.py", line 1039, in eval
    for L in code.split('\n') if L != ''])
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 476, in _eval_line
    self._start()
  File "/home/SageUser/sage-4.7.2/local/lib/python/site-packages/sage/interfaces/gap.py", line 913, in _start
    raise RuntimeError, msg
RuntimeError: Unable to start gap because the command 'gap -r -b -p -T -o 3900m /home/SageUser/sage-4.7.2/data//extcode/gap/sage.g' failed.

         [11.4 s]

----------------------------------------------------------------------
The following tests failed:


        sage -t -verbose "devel/sage/sage/calculus/functional.py"
Total time for all tests: 11.7 seconds

@kcrisman
Copy link
Member Author

kcrisman commented Dec 6, 2012

comment:58

I can't review that piece, sadly. I guess it would also be helpful to make sure this actually solves the Cygwin problem. I still have to get Sage to start, forking problems, sigh...

@kcrisman
Copy link
Member Author

kcrisman commented Dec 6, 2012

comment:59

Actually, now it does, magically. Somehow installing cvxopt and sage-location resetting paths did the job.

As to the rest: I like the removed files (Debian and non-patches) and most of the patch file changes, though I can't speak for mfile.patch. The changes in spkg-check seem right - why were those flags set anyway? Things should have already been compiled by that point. I would need some help in making sure that all the old stuff in spkg-install with respect to different soft links and so forth was no longer needed or handled by the ntl_install() function, but I assume it is.

@jpflori
Copy link

jpflori commented Dec 6, 2012

comment:60

There is no need for soft links and/or copying files and/or renaming them.

Using libtool, the upstream build system install the following files on Cygwin:

  • lib/libntl.la (libtool magical file)
  • lib/libntl.dll.a (import file for the dynamic library)
  • lib/libntl.a (static archive)
  • bin/cygntl.dll (dynamic library)
    (some - are surely missing here)

When using -lntl, ld will find first libntl.dll.a and in the end link to the dynamic library cygntl.dll.
The only difference with linking directly to cygntl.dll (let's say by specifying its path rather than just -lntl) is that the linking will be a slower, but that's not really an issue, everything is already so slow on Cygwin, I don't think the difference is noticeable.

On other systems the difference in file installed are minimal and will surely go unnoticed and the shared library will still gets picked up first.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 6, 2012

comment:61

This seems to be fine on Mac - relevant tests pass, if anything more quickly.

$ grep -r cygwin .
<a whole bunch of libtool files and a more or less irrelevant reference in src/>

I mean, if the Sage library passes tests with this, I'm inclined to say let's get this in. I still don't understand why upstream needs libtool to do its installing, but if that's a fairly standard solution in this kind of package then that should be okay too.[[Image()]]

@kcrisman
Copy link
Member Author

comment:62

Okay, I can't give a 100% positive review because I know nothing about libtool, but using the upstream solution makes a heck of a lot more sense if we don't have to add any other dependencies and just a few files, and the pieces I do understand of it make sense.

I built 5.5.rc0 with this on sage.math from scratch, and used SAGE_UPGRADING=yes on my Mac with this spkg in 5.4.rc2, and no doctest failures except on sage/tests/startup.py on sage.math, where it consistently seems to take 2.1 seconds, not less than 2 - but this was also true of a 5.4.rc1 build which didn't have this spkg, so it seems like there is just a heavy load there or something. Coupled with its success on Cygwin, I am strongly inclined to give this positive review, modulo understanding libtool stuff.

Jeroen, do you get what JP is trying to do and can you give this positive review based on that part of the changes in the spkg?

@kcrisman
Copy link
Member Author

Reviewer: Karl-Dieter Crisman

@kcrisman
Copy link
Member Author

kcrisman commented Jan 3, 2013

comment:63

Jeroen, do you get what JP is trying to do and can you give this positive review based on that part of the changes in the spkg?

Ping. I almost think it would be better to just put this in and let the test farm find any problems; using upstream's tools seems logical.

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2013

comment:64

Replying to @kcrisman:

Jeroen, do you get what JP is trying to do and can you give this positive review based on that part of the changes in the spkg?

Ping. I almost think it would be better to just put this in and let the test farm find any problems

You mean the Sage buildbot, right? In that case, just put the ticket to positive review if you are convinced that testing is the only thing remaining to be done.

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2013

comment:65

Minor comment: you should quote $CUR in spkg-install:

cd "$CUR/src"

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2013

comment:66

And libtool should not be part of the Mercurial history, since it's just upstream files (similar to src).

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2013

comment:67

There is also a strange file src/src/#mfile# in the sources.

I am preparing a new spkg.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2013

Spkg diff, for review only.

@jpflori
Copy link

jpflori commented Jan 4, 2013

comment:69

Attachment: ntl-5.5.2.p0.diff.gz

Thanks for improving the spkg.
The #mfile# is just some crap emacs must have produced.

@kcrisman
Copy link
Member Author

kcrisman commented Jan 4, 2013

comment:71

Sweet, I'll try this out on XP again right now. Thanks

@jdemeyer
Copy link

jdemeyer commented Jan 7, 2013

Merged: sage-5.6.beta3

@kcrisman
Copy link
Member Author

Changed author from Karl-Dieter Crisman, Jean-Pierre Flori to Jean-Pierre Flori

@kcrisman
Copy link
Member Author

comment:73

I think that my role as author was fairly limited in the final version, and presumably someone needs to have reviewed me in any case ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants