Opened 12 years ago

Last modified 8 years ago

#9914 closed defect

Remove libraries from extension modules when they are not needed there at build time — at Version 12

Reported by: leif Owned by: leif
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords: module_list.py PARI ImportError newforms homspace mwrank upgrade update
Cc: jhpalmieri, mhansen, mpatel, kcrisman, jpflori Merged in:
Authors: Leif Leonhardy Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mhansen)

Many extension modules in devel/sage/module_list.py are linked against libraries they do not use, some at least not directly.

This is inefficient, isn't nice or at least confusing and can (actually does) cause trouble.

See e.g. this comment for a discussion why.

This ticket will only address the removal of some unnecessary libraries listed; there are most probably more.

ADDED LATER: See however the later remarks about that claim of being unnecessary ...

Apply trac_9914.patch

Change History (13)

comment:1 follow-up: Changed 12 years ago by leif

  • Authors set to Leif Leonhardy
  • Cc jhpalmieri mpatel added
  • Keywords PARI ImportError newforms homspace mwrank upgrade update added
  • Status changed from new to needs_review

I've attached a first patch that removes PARI from libraries of:

  • sage.libs.cremona.homspace
  • sage.libs.cremona.newforms
  • sage.libs.mwrank.mwrank

Hope this doesn't cause trouble on non-Unices (like e.g. Cygwin), otherwise we would have to add uname_specific(...) (which is defined in module_list.py for such purposes).

(This patch fixes one of the upgrade issues discussed at #9896; these are in general not limited to MacOS X.)

Setting this to "needs review" to perhaps get it merged (early) into Sage 4.6.*, though more changes (to module_list.py) are desirable.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 12 years ago by drkirkby

Replying to leif:

Hope this doesn't cause trouble on non-Unices (like e.g. Cygwin),

I assume you tested this on Linux - that is a non-Unix!

Solaris, recent OS X, AIX 5.3 or later and one of the more recent HP-UX releases are Unix.

Even the {{{df}} command of Linux violates the Unix standard.

Dave

comment:3 in reply to: ↑ 2 Changed 12 years ago by leif

Replying to drkirkby:

Replying to leif:

Hope this doesn't cause trouble on non-Unices (like e.g. Cygwin),

I assume you tested this on Linux

I did.

  • that is a non-Unix!

U[nN][iI][xX] is a trademark, but also a stretchable term. ;-)

Should I say Ux-like OSs?

comment:4 Changed 12 years ago by GeorgSWeber

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Remove unused libraries from extension modules to Remove libraries from extension modules when they are not needed there at build time

Sorry, but this will certainly break on Cygwin!

Just have a look at the Makefile for the "g0nntl" library (in the eclib spkg, see the file /src/g0n/Makefile, line 41):

NTLLFLAGS = -L$(NTLLIBDIR) -lntl -lgmp -lpari

This means that the g0nntl library has the pari library as a dependency. And on Cywgin (Windows in general), not only the "primary" libraries have to be declared as dependencies during compile time (i.e. for building e.g. an extension module) --- but *all* libraries, i.e. also the "secondary", "tertiary", and so on ... (one may consider this a bug, a deficiency, a nuisance, whatever --- we have to live with it)

I've changed the title of the ticket somewhat, to reflect this, and added a line to the description.

If this ticket is meant to fix some OS X specific issue (see the main ticket at trac #9896), I'd propose to use the "uname_specific" feature in such a way, that the change to the module_list.py affects Darwin --- and only Darwin.

Ensuring that the change is visible on only as few system as possible, as few systems as possible are affected of a possible breakage by such a change.

comment:5 follow-up: Changed 12 years ago by leif

Replying to GeorgSWeber:

Sorry, but this will certainly break on Cygwin!

Did you read the comments here or at the other ticket? I explicitly asked if this is needed on Cygwin (twice), cc'ed Mike, and suggested to use uname_specific() in that case.

Just have a look at the Makefile for the "g0nntl" library (in the eclib spkg, see the file /src/g0n/Makefile, line 41):
NTLLFLAGS = -L$(NTLLIBDIR) -lntl -lgmp -lpari

Bad example, since you can safely remove PARI there (too; and also from NTLLFLAGS in all other Makefiles); NTL uses GMP, but not PARI. (Only procs/parifact.* uses PARI, so PARI has to be linked to all executables and shared libraries that contain / use that, e.g. libjcntl.{so,dylib,dll}, which in turn is linked to libg0nntl.*.)

(Note that "pari" is not listed in the libraries of sage.libs.cremona.mat, though "jcntl" is, and "g0nntl", too.)

Even if e.g. libg0nntl.so directly depended on PARI, none of the extension modules whose libraries I've changed does.

There's a difference between dynamically and statically linking btw.

I've changed the title of the ticket somewhat, to reflect this, and added a line to the description.

By "removing ... from the extension modules" I meant removing the in fact (locally) unused libraries from the dynamic (dependencies) section, i.e. for example the NEEDED entries in ELF.

If this ticket is meant to fix some OS X specific issue (see the main ticket at trac #9896), I'd propose to use the "uname_specific" feature in such a way, that the change to the module_list.py affects Darwin --- and only Darwin.

Again, as mentioned in my first comment here, and also at #9896, this is a general problem which is unrelated to Darwin as the OS.

Ensuring that the change is visible on only as few system as possible, as few systems as possible are affected of a possible breakage by such a change.

The opposite is closer to what is needed: if at all, PARI has to be listed in libraries only on Cygwin (or MS Windows) if you're right (but see above); I'm not sure if it would be required on older Darwins as well.

So I'll update the patch to use uname_specific() (if that makes you happy ;-) - I'd really like to give it a try as is on Cygwin), which then should be tested (also) on MacOS X 10.4 (Tiger).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 12 years ago by GeorgSWeber

Replying to leif:

Even if e.g. libg0nntl.so directly depended on PARI, none of the extension modules whose libraries I've changed does.

Even if the -lpari dependency in the build/compile call of the libg0nntl is technically superfluous, and/or none of the extension modules used libpari functionality in the end --- this parameter -lpari is *there* (in the Makefile line mentioned). This might be a bug in this eclib Makefile. Which probably doesn't matter on most systems, but might very well matter on Cygwin (and the presence of lpari in the module_list.py entries just a q'n'd workaround). Put in other words: the module_list.py patch might be incomplete without also updating (correcting?) this Makefile.

So I'll update the patch to use uname_specific() (if that makes you happy ;-) - I'd really like to give it a try as is on Cygwin), which then should be tested (also) on MacOS X 10.4 (Tiger).

So both of us are kind of stuck here --- neither of us seems to have access to some Cygwin system, to do "real life" testing. Personally, I do have access to Mac OS X 10.4 systems (both MacIntel? and MacPPC), but does that suffice to work on the ticket? I mean, wouldn't updating Sage-4.5.3 to Sage-4.6 still break on Cygwin (even if we fixed all other OS's)?

In general, such update problems are not new. The Sage build/update mechanisms have shortcomings that are well known. In the past, one had to (and did) give hints to them. I can't put my finger to a specific release or sage-devel thread right now, but the idea is as simple as powerful, I'll explain it at the example at hand.

Solution:

Just artificially bump up the version number of the eclib spkg. (No real changes are done to its contents --- and often in the past, even the update of the SPKG.txt was forgotten, but that's another matter, one should do that always).

As a result, during an update, the eclib spkg *will* be built anew, and as a consequence, also during "sage -b" the mentioned extensions are rebuilt. Et voila.

Leif, I couldn't follow each and every of the recent threads on sage-devel and sage-release, let alone trac. Has this "old way" been discussed, or even considered (for Sage-4.6)? If not, should a message thread on sage-release be opened?

I think there's a very good chance to get the update from Sage-4.5.3 to Sage-4.6 working, just by artificially bumping up the version numbers of a handful spkg's (in the sense I described above).

Cheers,

Georg

comment:7 in reply to: ↑ 6 Changed 12 years ago by leif

  • Status changed from needs_work to needs_info

Replying to GeorgSWeber:

Solution:

Just artificially bump up the version number of the eclib spkg. (No real changes are done to its contents --- and often in the past, even the update of the SPKG.txt was forgotten, but that's another matter, one should do that always).

As a result, during an update, the eclib spkg *will* be built anew, and as a consequence, also during "sage -b" the mentioned extensions are rebuilt. Et voila.

This doesn't work either, see http://trac.sagemath.org/sage_trac/ticket/9896#comment:16. (Bumping the patch level is equivalent to doing ./sage -f ....)

I think there's a very good chance to get the update from Sage-4.5.3 to Sage-4.6 working, just by artificially bumping up the version numbers of a handful spkg's (in the sense I described above).

Nevertheless, imagine upgrading MPIR (#8664). Do you want to bump the version numbers of dozens of packages? Changing sage-spkg to sage-spkg -f in spkg/standard/deps avoids this, but doesn't solve all problems we have with (re)building the Sage library.

Robert B. wanted to make Cython smarter such that more of what's manually provided in module_list.py gets deduced automatically from the Cython source files (through pragmas), but the new Cython 0.13 doesn't yet support these enhancements.

So unfortunately we currently have to keep hacking module_list.py and setup.py to get around subtle dependency issues...

We might as an alternative add depends = [ ... ] to these extension modules to get them updated when ECLIB gets (I think in general not a bad idea), but this doesn't really address the PARI issue, i.e. that the library should IMHO not be listed there.

There are or have been btw. other weird things like libcsage and libstdc++ being linked to each and every module, regardless of the language or if the module referenced symbols from there at all.

I started cleaning up module_list.py month ago (which is quite tedious), but then Robert said we'd have a better solution in the near future...

comment:8 Changed 12 years ago by mpatel

  • Cc mhansen added

Mike, do you think the changes above will cause problems on Cygwin?

comment:9 Changed 12 years ago by leif

Incidentally there came up a related discussion in an otherwise unrelated sage-devel thread.

comment:10 Changed 12 years ago by mhansen

These (three) changes don't cause any problem on Cygwin with Sage 4.6.

comment:11 Changed 12 years ago by GeorgSWeber

  • Status changed from needs_info to needs_review

Good to know,

sorry that I was wrong, Leif: please accept my apologies!

Cheers, Georg

Changed 10 years ago by mhansen

comment:12 Changed 10 years ago by mhansen

  • Description modified (diff)

I've added a rebased patch.

Apply trac_9914.patch

Assuming tests pass, I'll give this a positive review.

Note: See TracTickets for help on using tickets.