Opened 4 years ago

Closed 4 years ago

#22812 closed defect (fixed)

correct linking flags and options of gfan

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.0
Component: porting Keywords:
Cc: fbissey, embray Merged in:
Authors: Dima Pasechnik, François Bissey Reviewers: François Bissey, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 613c825 (Commits, GitHub, GitLab) Commit: 613c825abc213c3138281fd1d7e94292af22690c
Dependencies: Stopgaps:

Status badges

Description

sometimes LDFLAGS must come first, but in gfan's makefile LDFLAGS are put last. This makes it impossible to build on FreeBSD without extra hack, such as redefining CXX to include LDFLAGS.

We clean this mess up by making use of special variables (CDD-related) in its makefile, and remove LDFLAGS hack from spkg-install.

Change History (11)

comment:1 Changed 4 years ago by dimpase

  • Cc fbissey embray added
  • Status changed from new to needs_review
  • Summary changed from correct linking flags of gfan to correct linking flags and options of gfan

Does this work OK on OSX and Cygwin?

comment:2 follow-up: Changed 4 years ago by fbissey

I don't see why not. Your new patch is more invasive than the previous one for no visible reason (to me)

-ifeq ($(cddpath),)
-CDD_LINKOPTIONS = -L/usr/local -lcddgmp
+CDD_LINKOPTIONS = -L$SAGE_LOCAL/lib -lcddgmp -lgmp -lm
 CDD_INCLUDEOPTIONS =
-else
-CDD_LINKOPTIONS = $(cddpath)/lib/libcddgmp.a
-CDD_INCLUDEOPTIONS = -I $(cddpath)/include
-endif

Why do you need the above? The malformed is not a big problem. I guess a bigger problem is if the variable LIBRARY_PATH is not used with clang on freeBSD. But then I would expect you to have trouble in other places.

comment:3 in reply to: ↑ 2 Changed 4 years ago by dimpase

Replying to fbissey:

I don't see why not. Your new patch is more invasive than the previous one for no visible reason (to me)

Well, if you mean to say that I should leave the else part, which is never triggered under Sage, intact, I certainly can do this. (Although I see no harm in removing things which are not used).

-ifeq ($(cddpath),)
-CDD_LINKOPTIONS = -L/usr/local -lcddgmp
+CDD_LINKOPTIONS = -L$SAGE_LOCAL/lib -lcddgmp -lgmp -lm
 CDD_INCLUDEOPTIONS =
-else
-CDD_LINKOPTIONS = $(cddpath)/lib/libcddgmp.a
-CDD_INCLUDEOPTIONS = -I $(cddpath)/include
-endif

Why do you need the above? The malformed is not a big problem. I guess a bigger problem is if the variable LIBRARY_PATH is not used with clang on freeBSD. But then I would expect you to have trouble in other places.

Apart from the if/else thing (as I said, I can leave it in), this is done to clean up the flags a bit; presently that -L/usr/local -lcddgmp is worked around in spkg-install, basically by appending the correct -L$SAGE_LOCAL/lib -lcddgmp -lgmp -lm to LDFLAGS (the latter are appended in the makefile). I cannot keep doing this, I really need LDFLAGS in front, for this to work.

Besides, needless to say, if you really happen to have libcddgmp in /usr/local/, all the bets are off, you might end up with a linker error, or worse.

I could have plugged LDFLAGS into the 1st position into the right line of of the makefile, and leave the rest as it is, but this is ugly IMHO.

comment:4 follow-up: Changed 4 years ago by fbissey

OK, but the only thing you really need to do is remove -L/usr/local the rest is under control. Adding -lgmp is not useful as it is already provided by GMP_LINKOPTIONS. If you really need to add -lm because of some system where dynamical linking is foobar, add it at the end of GMP_LINKOPTIONS.

comment:5 in reply to: ↑ 4 Changed 4 years ago by dimpase

Replying to fbissey:

OK, but the only thing you really need to do is remove -L/usr/local the rest is under control. Adding -lgmp is not useful as it is already provided by GMP_LINKOPTIONS. If you really need to add -lm because of some system where dynamical linking is foobar, add it at the end of GMP_LINKOPTIONS.

I merely move adding -lm, needed on other systems, not only FreeBSD, from spkg-install to the makefile directly.

-# set LDFLAGS with proper flags to link against gmp/mpir and cddlib
-LDFLAGS="-L$SAGE_LOCAL/lib -lcddgmp -lgmp -lm $LDFLAGS"

If you think that all this can be done better, please do a branch...

comment:6 Changed 4 years ago by fbissey

There are several way to skin a cat as they say, I go for either minimal or complete revamp - which would feel nice here. Quite a number of things have moved since the original spkg-install making it easier to be minimalistic.

I'll do a branch...

comment:7 Changed 4 years ago by fbissey

  • Branch changed from u/dimpase/ldflagsord to u/fbissey/ldflagsord
  • Commit changed from 2d30f4911305c6ab3f17bb4146671a5d93c6fe55 to 1b21e8630fc92d114e830bd3cfefd9251acd568b

I invite you to test my small changes on my branch. I kept 90% of your changes, I only changed the Makefile patch with what I think is necessary.


New commits:

1b21e86Minimal change to Makefile

comment:8 Changed 4 years ago by dimpase

  • Authors changed from Dima Pasechnik to Dima Pasechnik, François Bissey
  • Reviewers set to François Bissey, Dima Pasechnik
  • Status changed from needs_review to needs_info

looks good to me. Should we bump the package version too?

comment:9 Changed 4 years ago by dimpase

  • Branch changed from u/fbissey/ldflagsord to u/dimpase/ldflagsord
  • Commit changed from 1b21e8630fc92d114e830bd3cfefd9251acd568b to 613c825abc213c3138281fd1d7e94292af22690c
  • Status changed from needs_info to positive_review

comment:10 Changed 4 years ago by fbissey

Jeroen always wants to bump. I am not always thrilled by it, but it certainly means more testing, which is his point.

comment:11 Changed 4 years ago by vbraun

  • Branch changed from u/dimpase/ldflagsord to 613c825abc213c3138281fd1d7e94292af22690c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.