Linking of Pynac to GMP

[tscrim]

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.

Upstream: Completely fixed; Fix reported upstream

CC: @sagetrac-gouezel @jpflori @rwst

Component: packages: standard

Keywords: pynac, underlinking

Reviewer: Travis Scrimshaw

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

[nexttime]

comment:2

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

[tscrim]

comment:3

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

[nexttime]

comment:4

The .pc file also lacks -lgmp.

[nexttime]

Changed keywords from pynac, linking to pynac, underlinking

[nexttime]

comment:5

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.

[sagetrac-gouezel]

Author: Sebastien Gouezel

[sagetrac-gouezel]

comment:6

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

[sagetrac-gouezel]

Branch: u/gouezel/pynac_lgmp

[sagetrac-gouezel]

Commit: 1ffa348

[nexttime]

comment:7

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).

[sagetrac-gouezel]

comment:8

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.

[rwst]

comment:9

It's an oversight. I have opened pynac/pynac#102. Thanks for the suggestions.

[nexttime]

comment:10

Replying to @rwst:

It's an oversight. I have opened pynac/pynac#102. Thanks for the suggestions.

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

[rwst]

comment:11

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

[nexttime]

comment:12

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)?

[nexttime]

Upstream: Reported upstream. Developers acknowledge bug.

[jdemeyer]

comment:14

Replying to @nexttime:

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

Fixed :-)

[sagetrac-gouezel]

Changed branch from u/gouezel/pynac_lgmp to u/gouezel/pynac_lgmp3

[sagetrac-gouezel]

Changed commit from 1ffa348 to 9499581

[sagetrac-gouezel]

comment:15

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

[jdemeyer]

comment:16

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.

[nexttime]

comment:17

Replying to @sagetrac-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". ;-)

[nexttime]

comment:18

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

[kiwifb]

comment:19

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.

[rwst]

comment:20

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.

[rwst]

comment:21

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

[rwst]

comment:22

Please test #19744 on Cygwin.

[tscrim]

Changed author from Sebastien Gouezel to none

[tscrim]

comment:23

This has been superseded by #19744, correct?

[tscrim]

Changed upstream from Reported upstream. Developers acknowledge bug. to Completely fixed; Fix reported upstream

[rwst]

comment:24

Replying to @tscrim:

This has been superseded by #19744, correct?

Yes, thanks.

[tscrim]

Changed branch from u/gouezel/pynac_lgmp3 to none

[tscrim]

Changed commit from 9499581 to none

[tscrim]

Reviewer: Travis Scrimshaw

[kiwifb]

comment:27

Replying to @rwst:

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.