Opened 2 years ago

Closed 2 years ago

#22856 closed defect (fixed)

Upgrade libhomfly

Reported by: tscrim Owned by:
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords: libhomfly cygwin
Cc: embray, jpflori, gouezel, mmarco Merged in:
Authors: Erik Bray, Travis Scrimshaw Reviewers: Travis Scrimshaw, Erik Bray
Report Upstream: N/A Work issues:
Branch: b4c2ac9 (Commits) Commit: b4c2ac935a80d4f6526a78096cac10205ed5f809
Dependencies: Stopgaps:

Description (last modified by tscrim)

We need to upgrade libhomfly to build on Cygwin

Errors out with messages like this when trying to build the corresponding Cython file:

/home/Travis/sage/local/var/tmp/sage/build/libhomfly-1.0/src/lib/control.c:209:(.text+0x7a9): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `GC_malloc'

Upstream tarball: ​https://riemann.unizar.es/~mmarco/libhomfly-1.0r2.tar.gz

Attachments (1)

win32-dll.patch (15.6 KB) - added by embray 2 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 2 years ago by mmarco

That seems to be rellated to boehm_gc. A version mismatch maybe?

comment:2 Changed 2 years ago by tscrim

I forgot to check if boehm_gc was installed. I will do that. Although IIRC, Cygwin can be more picky about linking order, and I saw a similar error coming from this.

comment:3 follow-up: Changed 2 years ago by embray

Is this an optional package? Definitely a blocker if you're using it but not in general. I can probably fix this; linking problems are very common.

comment:4 Changed 2 years ago by embray

(Lazyweb question: Is there an easy way to just install *all* optional packages, and better still run tests that use them? Or is it entirely piecemeal?)

comment:5 in reply to: ↑ 3 Changed 2 years ago by mmarco

Replying to embray:

Is this an optional package? Definitely a blocker if you're using it but not in general. I can probably fix this; linking problems are very common.

It is optional now, but has been for almost a year and I was planning to propose to promote it to standard after the one year period.

comment:6 Changed 2 years ago by embray

Okay--either way I do want all optional packages to work too of course. It's just lower priority than getting the standard build fully working. But now that it is we can start looking at more of the optional packages too.

Last edited 2 years ago by embray (previous) (diff)

comment:7 Changed 2 years ago by embray

So libhomfly did build for me (just ran make libhomfly). Though it only built a static library, not shared, and I do have a warning:

[libhomfly-1.0] /bin/sh ../libtool  --tag=CC   --mode=link gcc  -g -O2 -lgc -export-symbols-regex '(c_)?homfly' -L/home/embray/src/sagemath/sage-cygwin/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage-cygwin/local/lib  -o libhomfly.la -rpath /home/embray/src/sagemath/sage-cygwin/local/lib bound.lo control.lo dllink.lo homfly.lo knot.lo model.lo order.lo poly.lo
[libhomfly-1.0] libtool: warning: undefined symbols not allowed in x86_64-unknown-cygwin shared libraries; building static only
[libhomfly-1.0] libtool: link: ar cru .libs/libhomfly.a  bound.o control.o dllink.o homfly.o knot.o model.o order.o poly.o
[libhomfly-1.0] libtool: link: ranlib .libs/libhomfly.a
[libhomfly-1.0] libtool: link: ( cd ".libs" && rm -f "libhomfly.la" && ln -s "../libhomfly.la" "libhomfly.la" )
[libhomfly-1.0] make[3]: Leaving directory '/home/embray/src/sagemath/sage-cygwin/local/var/tmp/sage/build/libhomfly-1.0/src/lib'

Which it was usually happens with my libtool if there are undefined symbols when linking a shared library (is it possible you have different default settings for libtool?)

I haven't investigated further yet, but I've found a lot of automake packages don't have the necessary bits to get DLLs building properly on Windows. See for example #21957, and I think some others I've worked on.

comment:8 Changed 2 years ago by embray

Basically the same boilerplate that I described here should work here too; I will give it a try.

Changed 2 years ago by embray

comment:9 Changed 2 years ago by embray

The attached patch should fix it. I will add a branch with a version of this patch that only patches the generated files (and not the source files like configure.ac) so that applying the patch doesn't cause autotools invocation.

I also noted that even with this patch, a static library is created by default. Unless we specifically want a static version of libhomfly for some reason it would be good to disable this, just for disk space savings if nothing else.

comment:10 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/ticket-22856
  • Commit set to cbf7a7a388d93b1f1821becbc1a8931952bbf78a
  • Keywords cygwin added
  • Status changed from new to needs_review

New commits:

cbf7a7aPatch libhomfly to build a shared library on Cygwin

comment:11 Changed 2 years ago by tscrim

I install libhomfly as part of my usual optional packages, so that is my interest.

The library built okay for me too, but the problem (for me) comes from when you try to run sage -b to build the necessary Cython bindings. Did you also try this too? I also didn't change any libtool settings, so I doubt that is the problem.

Miguel, would you merge this upstream (provided it builds on your systems too)?

comment:12 Changed 2 years ago by embray

The patch should probably fix it. The reason linking failed for you was that libhomfly was only being built as a static library.

comment:13 Changed 2 years ago by embray

I re-ran ./sage -b after installing libhomfly and successfully ran what appeared to be the most relevant tests:

$ ./sage -t --long src/sage/knots/ src/sage/libs/homfly.pyx
Running doctests with ID 2017-04-24-16-26-16-363ee458.
Git branch: u/embray/develop-cygwin
Using --optional=atlas,ccache,libhomfly,mpir,python2,sage
Doctesting 5 files.
sage -t --long --warn-long 177.9 src/sage/knots/all.py
    [0 tests, 0.00 s]
sage -t --long --warn-long 177.9 src/sage/knots/knot.py
    [42 tests, 14.08 s]
sage -t --long --warn-long 177.9 src/sage/knots/link.py
    [407 tests, 21.00 s]
sage -t --long --warn-long 177.9 src/sage/knots/__init__.py
    [0 tests, 0.00 s]
sage -t --long --warn-long 177.9 src/sage/libs/homfly.pyx
    [6 tests, 1.77 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 38.3 seconds
    cpu time: 28.2 seconds
    cumulative wall time: 36.9 seconds

comment:14 Changed 2 years ago by tscrim

Thanks. I'm building the 8.0.beta3 right now, and this is my next test afterwards.

comment:15 Changed 2 years ago by mmarco

I could apply the patch upstream and make a new release (so we would need to update the libhomfly version in Sage). But I don't have a windows machine to test it on.

Another option is to just apply Eric's patch in Sage's install script.

What option do you think would be better?

comment:16 Changed 2 years ago by tscrim

I think it would be better to make this a version bump of libhomfly. Erik has tested it on Cygwin, and I am in the process of testing it (since I've gone to beta3, I'm stuck at Python3). So if it works on Linux (which I can test) and/or OSX (which I would be surprised if it didn't), then version bump is definitely the way to go IMO.

comment:17 Changed 2 years ago by embray

Yes, an upstream fix would be best.

comment:18 Changed 2 years ago by mmarco

Can you please check that this tarball:

https://riemann.unizar.es/~mmarco/libhomfly-1.0r2.tar.gz

builds correctly and passes the make check test?

comment:19 Changed 2 years ago by tscrim

  • Branch changed from u/embray/cygwin/ticket-22856 to u/tscrim/libhomfly_1_0r2-22856
  • Commit changed from cbf7a7a388d93b1f1821becbc1a8931952bbf78a to b4c2ac935a80d4f6526a78096cac10205ed5f809
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

I've checked on Linux. I'm still fighting the 8.0.beta3 build on Cygwin, but since it built for Erik, I'm setting a positive review.


New commits:

b4c2ac9Version bump of libhomfly.

comment:20 Changed 2 years ago by tscrim

  • Status changed from positive_review to needs_review

Correction, once someone checks my changes, then it can be set to positive review.

comment:21 Changed 2 years ago by tscrim

  • Description modified (diff)
  • Summary changed from libhomfly doesn't build on Cygwin to Upgrade libhomfly

comment:22 Changed 2 years ago by mmarco

Everything seems ok on Linux. If it solves the problem in cygwin, set it to positive review.

comment:23 Changed 2 years ago by embray

  • Authors changed from Erik Bray to Erik Bray, Travis Scrimshaw
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Erik Bray
  • Status changed from needs_review to positive_review

comment:24 Changed 2 years ago by tscrim

Now I can (finally, thanks to Erik) confirm this works on my cygwin as well.

comment:25 Changed 2 years ago by vbraun

  • Branch changed from u/tscrim/libhomfly_1_0r2-22856 to b4c2ac935a80d4f6526a78096cac10205ed5f809
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.