Opened 7 years ago

Closed 7 years ago

#14038 closed defect (fixed)

Let libgap build a shared library on Cygwin

Reported by: jpflori Owned by: tbd
Priority: major Milestone: sage-5.8
Component: packages: standard Keywords: cygwin spkg libgap
Cc: kcrisman, dimpase, vbraun Merged in: sage-5.8.beta0
Authors: Volker Braun, Jean-Pierre Flori Reviewers: Jean-Pierre Flori, Dmitrii Pasechnik
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

We need to pass -no-undefined to libtool, the proper place is in libgap_la_LDFLAGS, potentially only on Cygwin and other Windows systems (have a look in GMP/MPIR/MPFR for examples), but should not hurt anyway elsewhere if added unconditionally.

(By the way the spkg-install script is terrible, does not use $MAKE, use [ ] and ?, sets CXXFLAGS but does not export it...)

http://www.stp.dias.ie/~vbraun/Sage/spkg/libgap-4.5.7.p1.spkg

Change History (36)

comment:1 Changed 7 years ago by jpflori

And now MPIR defines GNU_MP_RELEASE so the GMP_MP_RELEASE hack is not needed anymore here or in gap spkg.

comment:2 Changed 7 years ago by vbraun

I made #14039 to keep track of what to change for the next GAP update.

comment:3 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • Description modified (diff)
  • Status changed from new to needs_review

I've made a new spkg that includes the --no-undefined. Please let me know if it fixes things.

comment:4 Changed 7 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to needs_work

I've had a quick look at the spkg and it seems fine except there should be only one dash in front of no-undefined (not sure though putting two will break things).

The spkg-install cript looks cleaner. Would you mind replacing the last double brackets by simple ones? The use of double brackets does not seem necerssary for the simple tests involved.

A line for the version bump in SPKG.txt would be nice as well.

It should fix things on Cygwin (I crafted my own spkg yesterday and everything went fine, see CygwinPort). The question should then rather be: Does it break things on other systems? (it should not.)

comment:5 follow-up: Changed 7 years ago by vbraun

Long-style is --no-undefined (two dashes), short is -z. ld is being nice and also accepts the mongrel -no-undefined but we shouldn't rely on that imho.

I made the other cosmetic changes.

It doesn't break anything on Linux. After you review the spkg, the buildbot will catch anything that breaks on exotic platforms.

comment:6 Changed 7 years ago by vbraun

  • Status changed from needs_work to needs_review

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

  • Status changed from needs_review to needs_work

Replying to vbraun:

Long-style is --no-undefined (two dashes), short is -z. ld is being nice and also accepts the mongrel -no-undefined but we shouldn't rely on that imho.

Its not an ld flag its a libtool one and its definitely one dash. The fact that libtool does not follow usual conventions is another problem.

I made the other cosmetic changes.

It doesn't break anything on Linux. After you review the spkg, the buildbot will catch anything that breaks on exotic platforms.

comment:8 Changed 7 years ago by jpflori

(And I seem to remember there should be a LIBTOOLFLAGS var or stg like that, but I don't think anyone uses is, not even sure libtool uses it itself...)

comment:9 follow-up: Changed 7 years ago by dimpase

Why do we even need CXXFLAGS? There is no C++ in GAP, isn't it?

comment:11 in reply to: ↑ 9 Changed 7 years ago by jpflori

Replying to dimpase:

Why do we even need CXXFLAGS? There is no C++ in GAP, isn't it?

Don't know but I feel you're right. Id say its an artifact from the time where spkg-install files' headers were all formatted the same.

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

Replying to dimpase:

Please see https://bitbucket.org/vbraun/libgap/pull-requests/1/cf-sage-trac-14038/diff for updates on this

so we need a Python patch for this to work on OSX. http://hg.python.org/cpython/rev/864b9836dae6 I've opened #14057 for this.

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

comment:13 Changed 7 years ago by dimpase

  • Dependencies set to #14057

comment:14 follow-up: Changed 7 years ago by dimpase

Even after applying #14057 the modified libgap does not build on OSX 10.6.8. Does it need more libtool magic? I get:

libtool: link: gcc -dynamiclib  -o .libs/libgap.0.dylib  .libs/libgap_la-ariths.o .libs/libgap_la-c_random.o .libs/libgap_la-gmpints.o .libs/libgap_la-objccoll.o .libs/libgap_la-rational.o .libs/libgap_la-system.o .libs/libgap_la-blister.o .libs/libgap_la-c_type1.o .libs/libgap_la-gvars.o .libs/libgap_la-objcftl.o .libs/libgap_la-read.o .libs/libgap_la-tietze.o .libs/libgap_la-bool.o .libs/libgap_la-cyclotom.o .libs/libgap_la-integer.o .libs/libgap_la-objects.o .libs/libgap_la-records.o .libs/libgap_la-vars.o .libs/libgap_la-calls.o .libs/libgap_la-dt.o .libs/libgap_la-intfuncs.o .libs/libgap_la-objfgelm.o .libs/libgap_la-saveload.o .libs/libgap_la-vec8bit.o .libs/libgap_la-c_filt1.o .libs/libgap_la-dteval.o .libs/libgap_la-intrprtr.o .libs/libgap_la-objpcgel.o .libs/libgap_la-scanner.o .libs/libgap_la-vecffe.o .libs/libgap_la-c_meths1.o .libs/libgap_la-exprs.o .libs/libgap_la-iostream.o .libs/libgap_la-objscoll.o .libs/libgap_la-sctable.o .libs/libgap_la-vecgf2.o .libs/libgap_la-code.o .libs/libgap_la-finfield.o .libs/libgap_la-libgap.o .libs/libgap_la-opers.o .libs/libgap_la-set.o .libs/libgap_la-vector.o .libs/libgap_la-compiler.o .libs/libgap_la-funcs.o .libs/libgap_la-listfunc.o .libs/libgap_la-permutat.o .libs/libgap_la-stats.o .libs/libgap_la-weakptr.o .libs/libgap_la-compstat.o .libs/libgap_la-gap.o .libs/libgap_la-listoper.o .libs/libgap_la-plist.o .libs/libgap_la-streams.o .libs/libgap_la-c_oper1.o .libs/libgap_la-lists.o .libs/libgap_la-precord.o .libs/libgap_la-string.o .libs/libgap_la-costab.o .libs/libgap_la-gasman.o .libs/libgap_la-macfloat.o .libs/libgap_la-range.o .libs/libgap_la-sysfiles.o   -L/usr/local/src/sage/sage-5.6.beta2/local/lib /usr/local/src/sage/sage-5.6.beta2/local/lib/libgmp.dylib    -install_name  /usr/local/src/sage/sage-5.6.beta2/local/lib/libgap.0.dylib -compatibility_version 1 -current_version 1.0 -Wl,-single_module
Undefined symbols:
  "_environ", referenced from:
      _libgap_initialize in libgap_la-libgap.o
      _libGAP_SyExecuteProcess in libgap_la-sysfiles.o
ld: symbol(s) not found
collect2: ld returned 1 exit status
make[2]: *** [libgap.la] Error 1

comment:15 in reply to: ↑ 14 Changed 7 years ago by dimpase

Replying to dimpase:

Even after applying #14057 the modified libgap does not build on OSX 10.6.8. Does it need more libtool magic?

Specifying -no-undefined only for CYGWIN will fix this. I don't know how to do this properly, though. Any tips?

comment:16 Changed 7 years ago by jpflori

Yup Ill post a patch. First time I see -no-undefined break something. Nice!

comment:17 Changed 7 years ago by jpflori

But without undefined do you get a shared library on OS X ?

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

And I don't really feel #14057 could fix anything here. It provides a fix for Python and it does not seem to me that libgap links to Python anyway... But I agree we would need a similar fix in libgap, that is properly define environ in libgap (src/libgap.c) and not assume it is available.

comment:19 Changed 7 years ago by jpflori

comment:20 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by dimpase

Replying to jpflori:

And I don't really feel #14057 could fix anything here. It provides a fix for Python and it does not seem to me that libgap links to Python anyway...

yes, I agree. I'll remove it from the deps.

But I agree we would need a similar fix in libgap, that is properly define environ in libgap (src/libgap.c) and not assume it is available.

I wonder if this is actually an OSX-specific libgap bug, to use -undefined dynamic_lookup to get environ. (or maybe this part of the code is never used in libgap setting)

OSX docs say that it's recommended to use getenv() etc, but that's GAPs design to use environ directly (and it's needed for spawn() etc there). And environ is not available to shared libraries, according to loc.cit., and one should use _NSGetEnviron() routine from <crt_externs.h>. (So one could add this to src/sysfiles.c if needed.)

comment:21 Changed 7 years ago by dimpase

  • Dependencies #14057 deleted

comment:22 in reply to: ↑ 20 Changed 7 years ago by jpflori

Could you post a log of a succesful build of libgap on your OS X somewhere? And a failed one when no-undefined is passed to libtool? I'd be interested in seeing what happens.

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

Whatever, I guess that passing -no-undefined to libtool just overrides its default behavior to pass -undefined dynamic_lookup to ld on OS X.

So we have two solutions:

  • only pass -no-undefined where it is really needed, that is on Cygwin,
  • use _NSGetEnviron() routine from <crt_externs.h> to define environ on OS X.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 7 years ago by dimpase

Replying to jpflori:

Whatever, I guess that passing -no-undefined to libtool just overrides its default behavior to pass -undefined dynamic_lookup to ld on OS X.

indeed.

So we have two solutions:

  • only pass -no-undefined where it is really needed, that is on Cygwin,
  • use _NSGetEnviron() routine from <crt_externs.h> to define environ on OS X.

let us try to see if the latter is needed. My understanding is that currently the corresponding part of GAP functionality (ExecuteProcess()) is not exposed, and/or not implemented, in libGAP, and so it's not visible whether it is a bug to use -undefined dynamic_lookup to get (or not?) environ on OSX.

Or, perhaps, Volker knows the answer already? Perhaps he's not going to use this part of the orginal GAP code in libGAP at all?

comment:25 in reply to: ↑ 24 Changed 7 years ago by dimpase

Replying to dimpase:

Replying to jpflori:

Whatever, I guess that passing -no-undefined to libtool just overrides its default behavior to pass -undefined dynamic_lookup to ld on OS X.

indeed.

So we have two solutions:

  • only pass -no-undefined where it is really needed, that is on Cygwin,
  • use _NSGetEnviron() routine from <crt_externs.h> to define environ on OS X.

let us try to see if the latter is needed. My understanding is that currently the corresponding part of GAP functionality (ExecuteProcess()) is not exposed, and/or not implemented, in libGAP, and so it's not visible whether it is a bug to use -undefined dynamic_lookup to get (or not?) environ on OSX.

My understanding from e.g. this thread is that extern char** environ with -no-undefined for a shared library used with the OSX ld and _NSGetEnviron() lead to equivalent results.

So it seems that to make this change non-OSX only, or Cygwin-only, is right. AFAIK, Volker is currently away, so you can have a go at it yourself if you have time.

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

I don't really follow you here. From what you linked, it seems that using an extern environ won't work (unless we allow undefined vars at compile time and hope that at runtime something will have properly defined and exported environ and we can link to it, which I find bad practice), and that instead one should use _NSGetEnviron() (which is available and can be linked in at compile time).

Even though only defining -no-undefined on Cygwin can only smoothen things, I don't think it would be a mistake to define it unconditionally, provided we correctly define environ in all situations.

comment:27 Changed 7 years ago by jpflori

Could you try the spkg at http://boxen.math.washington.edu/home/jpflori/libgap-4.5.7.p1.spkg ?

Just to see if using _NSGetEnviron is enough to solve our problem on OS X.

comment:28 in reply to: ↑ 26 Changed 7 years ago by dimpase

Replying to jpflori:

I don't really follow you here. From what you linked, it seems that using an extern environ won't work (unless we allow undefined vars at compile time and hope that at runtime something will have properly defined and exported environ and we can link to it, which I find bad practice),

Well, that seems to be the usual practice on OSX, to delay such resolutions to ld. If you read the last message in the tread I mentioned, that's exactly what it says.

I just tried your new spkg, and you still have at least one place (in src/src/sysfiles.c, I guess) where environ is undefined:

libtool: link: gcc -dynamiclib  -o .libs/libgap.0.dylib  .libs/libgap_la-ariths.o .libs/libgap_la-c_random.o .libs/libgap_la-gmpints.o .libs/libgap_la-objccoll.o .libs/libgap_la-rational.o .libs/libgap_la-system.o .libs/libgap_la-blister.o .libs/libgap_la-c_type1.o .libs/libgap_la-gvars.o .libs/libgap_la-objcftl.o .libs/libgap_la-read.o .libs/libgap_la-tietze.o .libs/libgap_la-bool.o .libs/libgap_la-cyclotom.o .libs/libgap_la-integer.o .libs/libgap_la-objects.o .libs/libgap_la-records.o .libs/libgap_la-vars.o .libs/libgap_la-calls.o .libs/libgap_la-dt.o .libs/libgap_la-intfuncs.o .libs/libgap_la-objfgelm.o .libs/libgap_la-saveload.o .libs/libgap_la-vec8bit.o .libs/libgap_la-c_filt1.o .libs/libgap_la-dteval.o .libs/libgap_la-intrprtr.o .libs/libgap_la-objpcgel.o .libs/libgap_la-scanner.o .libs/libgap_la-vecffe.o .libs/libgap_la-c_meths1.o .libs/libgap_la-exprs.o .libs/libgap_la-iostream.o .libs/libgap_la-objscoll.o .libs/libgap_la-sctable.o .libs/libgap_la-vecgf2.o .libs/libgap_la-code.o .libs/libgap_la-finfield.o .libs/libgap_la-libgap.o .libs/libgap_la-opers.o .libs/libgap_la-set.o .libs/libgap_la-vector.o .libs/libgap_la-compiler.o .libs/libgap_la-funcs.o .libs/libgap_la-listfunc.o .libs/libgap_la-permutat.o .libs/libgap_la-stats.o .libs/libgap_la-weakptr.o .libs/libgap_la-compstat.o .libs/libgap_la-gap.o .libs/libgap_la-listoper.o .libs/libgap_la-plist.o .libs/libgap_la-streams.o .libs/libgap_la-c_oper1.o .libs/libgap_la-lists.o .libs/libgap_la-precord.o .libs/libgap_la-string.o .libs/libgap_la-costab.o .libs/libgap_la-gasman.o .libs/libgap_la-macfloat.o .libs/libgap_la-range.o .libs/libgap_la-sysfiles.o   -L/usr/local/src/sage/sage-5.6.beta2/local/lib /usr/local/src/sage/sage-5.6.beta2/local/lib/libgmp.dylib    -install_name  /usr/local/src/sage/sage-5.6.beta2/local/lib/libgap.0.dylib -compatibility_version 1 -current_version 1.0 -Wl,-single_module

Undefined symbols:
  "_environ", referenced from:
      _libGAP_SyExecuteProcess in libgap_la-sysfiles.o

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

The spkg is updated (I've not commited anything as I don't want to deal with the external repository hell).

And I still do not understand what you meant after reading all the thread. The last message basically says that using _NSGetEnviron() worked, and in other messages its told that the solution used is not to remove no-undefined but to use _NSGetEnviron().

By ld do you mean run-time linking? because I don't think its ld that takes care of that. And passing -undefined dynamic_lookup won't let ld resolve anything at linking time. It is just a promise that at runtime environ will be available somehow, which could perfectly not be the case if the proper libraries are not loaded.

comment:30 in reply to: ↑ 29 Changed 7 years ago by dimpase

Replying to jpflori:

The spkg is updated (I've not commited anything as I don't want to deal with the external repository hell).

OK, it builds and seemingly works.

And I still do not understand what you meant after reading all the thread. The last message basically says that using _NSGetEnviron() worked, and in other messages its told that the solution used is not to remove no-undefined but to use _NSGetEnviron().

No, I think it says that the library built with the -undefined dynamic_lookup option works.

By ld do you mean run-time linking? because I don't think its ld that takes care of that.

OK, here is the example:

$ cat enlib.c
extern char** environ;
char *** environ_addr (void) { return &environ; }

$ glibtool --mode=compile gcc -c enlib.c 
glibtool: compile:  gcc -c enlib.c  -fno-common -DPIC -o .libs/enlib.o
glibtool: compile:  gcc -c enlib.c -o enlib.o >/dev/null 2>&1

$ glibtool --mode=link gcc -o libenlib.la enlib.lo -rpath /tmp
glibtool: link: rm -fr  .libs/libenlib.a .libs/libenlib.la
glibtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libenlib.0.dylib  .libs/enlib.o      -install_name  /tmp/libenlib.0.dylib -compatibility_version 1 -current_version 1.0 -Wl,-single_module
glibtool: link: dsymutil .libs/libenlib.0.dylib || :
warning: no debug symbols in executable (-arch x86_64)
glibtool: link: (cd ".libs" && rm -f "libenlib.dylib" && ln -s "libenlib.0.dylib" "libenlib.dylib")
glibtool: link: (cd ".libs" && rm -f "libenlib.0.0.0.dylib" && ln -s "libenlib.0.dylib" "libenlib.0.0.0.dylib")
glibtool: link: ar cru .libs/libenlib.a  enlib.o
glibtool: link: ranlib .libs/libenlib.a
glibtool: link: ( cd ".libs" && rm -f "libenlib.la" && ln -s "../libenlib.la" "libenlib.la" )

$ cat en.c
#include <stdio.h>
#include <crt_externs.h>
#define macenviron (*_NSGetEnviron())
extern char *** environ_addr (void); /* comes from my enlib */
int main()
{
  printf(" from the shared lib:\n  %s \n", **environ_addr());
  printf(" from macenviron :\n  %s \n", *macenviron);
}

$ glibtool --mode=link gcc -o en en.c libenlib.la 
glibtool: link: gcc -o .libs/en en.c  ./.libs/libenlib.0.0.0.dylib

$ ./en
 from the shared lib:
  NNTPSERVER=news.gmane.org 
 from macenviron :
  NNTPSERVER=news.gmane.org 

So you see, it works with -undefined dynamic_lookup just fine. (printf(%s) isn't the rght way of parsing of environ, so the output is cut at the first \n, I guess).

comment:31 Changed 7 years ago by jpflori

  • Authors changed from Volker Braun to Volker Braun, Jean-Pierre Flori
  • Description modified (diff)
  • Work issues set to integrate changes upstream

Volker, would you mind integrating the changes from the last version I posted "upstream" and repackage a "proper" spkg?

comment:32 Changed 7 years ago by vbraun

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

comment:33 Changed 7 years ago by jpflori

  • Reviewers changed from Jean-Pierre Flori to Jean-Pierre Flori, Dmitrii Pasechnik
  • Work issues integrate changes upstream deleted

Dima, could you give it a shot on Mac OS so that we we put it as positive review? As far as I'm cocnerned, everything looks fine.

comment:34 Changed 7 years ago by dimpase

  • Status changed from needs_review to positive_review

good; works on OSX too, including the standalone test in sage/libs/gap/test/. Positive review.

comment:35 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:36 Changed 7 years ago by jdemeyer

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