Opened 4 years ago

Closed 4 years ago

#19678 closed defect (fixed)

Linking of Pynac to GMP

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords: pynac, underlinking
Cc: gouezel, jpflori, rws Merged in:
Authors: Reviewers: Travis Scrimshaw
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

When building on Cygwin32, I get the following error:

.libs/libpynac_la-numeric.o: In function `ZN5GiNaC7numericD2Ev':
/home/Travis/sage/local/var/tmp/sage/build/pynac-0.5.2/src/ginac/numeric.cpp:621:
 undefined reference to `_imp____gmpq_clear'

This indicated to me there is a linking issue with Pynac to GMP. Full log is here on #19663.


Fixed by the update to Pynac 0.5.3 on #19744.

Change History (28)

comment:1 Changed 4 years ago by jdemeyer

  • Cc rws added

comment:2 Changed 4 years ago by leif

Note that this is unrelated to the change in #19606 (merged before #19312, which reverted it).

comment:3 Changed 4 years ago by tscrim

I was trying to build on Cygwin with 6.10.beta7, which contains both #19606 and #19312.

comment:4 Changed 4 years ago by leif

  • Keywords underlinking added; linking removed

The .pc file also lacks -lgmp.

comment:5 Changed 4 years ago by leif

FWIW, Pynac's configure doesn't check for libgmp, just for its header.

In GiNaC's Makefile we still have

#The -no-undefined breaks Pynac on OS X 10.4.  See #9135
if CYGWIN
libpynac_la_LDFLAGS = -version-info $(LT_VERSION_INFO) -no-undefined
else
libpynac_la_LDFLAGS = -version-info $(LT_VERSION_INFO)
endif

though, so it must fail on Cygwin (unless we link to libgmp there).


The libtool versioning (0.0.0) IMHO doesn't make sense either.

comment:6 Changed 4 years ago by gouezel

  • Authors set to Sebastien Gouezel
  • Branch set to u/gouezel/pynac_lgmp
  • Commit set to 1ffa348731653aee97857980c6db05b08bbadaea
  • Status changed from new to needs_review

Added linking to gmp on Cygwin in the makefile. With this, compilation on cygwin works properly.


New commits:

1ffa348 #19678: linking pynac to gmp under cygwin

comment:7 Changed 4 years ago by leif

Hmmm, do we want to link with GMP on Cygwin only?

It would have been sufficient to add -lgmp to LIBS in configure[.ac], either manually or by callingAC_CHECK_LIB() (but upstream should decide; not sure why they don't do this already).

comment:8 Changed 4 years ago by gouezel

I agree it is probably safe to link everywhere to GMP. I was scared by the comment The -no-undefined breaks Pynac on OS X 10.4. See #9135, since it is also usually safe to add no-undefined everywhere. As I have no OS X to test, I opted for the safest solution. Anyway, I agree upstream should decide.

comment:9 follow-up: Changed 4 years ago by rws

It's an oversight. I have opened https://github.com/pynac/pynac/issues/102. Thanks for the suggestions.

comment:10 in reply to: ↑ 9 Changed 4 years ago by leif

Replying to rws:

It's an oversight. I have opened https://github.com/pynac/pynac/issues/102. Thanks for the suggestions.

Should we wait for an upstream patch or merge as is?

comment:11 Changed 4 years ago by rws

Please don't wait. I'm in minimum maintenance mode (accept patches, do urgent releases).

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

I'd prefer to not just add it on Cygwin; on the other hand, the component is still (just) "Porting: Cygwin", and the current patch doesn't affect other systems (so doesn't need further testing on MacOS X, say).

Thoughts (by others than Ralph)?

comment:13 Changed 4 years ago by leif

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

comment:14 in reply to: ↑ 12 Changed 4 years ago by jdemeyer

  • Component changed from porting: Cygwin to packages: standard

Replying to leif:

the component is still (just) "Porting: Cygwin"

Fixed :-)

comment:15 follow-up: Changed 4 years ago by gouezel

  • Branch changed from u/gouezel/pynac_lgmp to u/gouezel/pynac_lgmp3
  • Commit changed from 1ffa348731653aee97857980c6db05b08bbadaea to 9499581124cf0ceea5a14af04115a2dc1db267e8

New branch linking to gmp on all platforms, adding the proper test in configure.

The patch itself is one line

AC_CHECK_LIB(gmp, __gmpz_init, , AC_MSG_ERROR([This package needs libgmp]))

in configure.ac (this is the patch in patches/build/). The patch generated by autotools, however, is much bigger since my versions of the tools do not match with the versions previously used by pynac (my versions are more recent, so it should be for the better).


New commits:

9499581 #19678: link to gmp in pynac

comment:16 Changed 4 years ago by jdemeyer

The change to configure.ac looks sensible.

Is upstream (@rws) willing to make a new release for this? Then we don't need the patching.

comment:17 in reply to: ↑ 15 Changed 4 years ago by leif

Replying to gouezel:

New branch linking to gmp on all platforms, adding the proper test in configure.

Thanks. If I had the time, I would have done the same in a pull request...

@jdemeyer: Simply set upstream to "Fixed upstream, but not in a stable release". ;-)

comment:18 Changed 4 years ago by leif

P.S.: Of course a patch using the same autotools versions is preferable.

comment:19 Changed 4 years ago by fbissey

I already submitted a PR upstream (two actually, the .pc was also sick) and they have been merged. Not sure when Ralph will want to do a release.

comment:20 follow-up: Changed 4 years ago by rws

The search lib command is already merged. I'll add the config.h.in patch. Maybe I'll do a release at the weekend. Note that I do not have a means for testing it but you seem to have had success with these patches.

comment:21 Changed 4 years ago by rws

Ah that gets generated, so I can do the release today, as well.

comment:22 Changed 4 years ago by rws

Please test #19744 on Cygwin.

comment:23 follow-up: Changed 4 years ago by tscrim

  • Authors Sebastien Gouezel deleted
  • Milestone changed from sage-6.10 to sage-duplicate/invalid/wontfix
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Completely fixed; Fix reported upstream

This has been superseded by #19744, correct?

comment:24 in reply to: ↑ 23 Changed 4 years ago by rws

Replying to tscrim:

This has been superseded by #19744, correct?

Yes, thanks.

comment:25 Changed 4 years ago by tscrim

  • Branch u/gouezel/pynac_lgmp3 deleted
  • Commit 9499581124cf0ceea5a14af04115a2dc1db267e8 deleted
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

comment:26 Changed 4 years ago by leif

  • Description modified (diff)

comment:27 in reply to: ↑ 20 Changed 4 years ago by fbissey

Replying to rws:

The search lib command is already merged. I'll add the config.h.in patch. Maybe I'll do a release at the weekend. Note that I do not have a means for testing it but you seem to have had success with these patches.

That was not really useful but it doesn't hurt and is consistent with common practise I guess.

comment:28 Changed 4 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.