Opened 9 years ago

Closed 9 years ago

#12682 closed defect (fixed)

Fix "hardcoded" 'gcc' in ratpoints 2.1.3 [p2]

Reported by: leif Owned by: leif
Priority: major Milestone: sage-5.0
Component: packages: standard Keywords: rd2 spkg CC compiler hard-coded
Cc: rlm, jdemeyer, ohanar, mjo Merged in: sage-5.0.beta9
Authors: Leif Leonhardy Reviewers: R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

The upstream Makefile defines CC to gcc, which could be overridden by using make CC="$CC" ... in spkg-install (similar for INSTALL_DIR, which the Makefile defines to /usr/local).

The updated spkg patches the Makefile to let CC just default to gcc, and take the value of the environment variable CC if the latter is defined (which is always the case in Sage, but that doesn't matter). Similar for INSTALL_DIR, such that spkg-install can (and does now) make use of the install-lib target, also quoting the target filenames to allow spaces in $SAGE_LOCAL.

I've also cleaned up SPKG.txt and spkg-install a little; the latter now also uses $MAKE instead of make.

I'm not sure whether we should add running some tests (there's a test target which does use the built library), or even add an spkg-check script; if so, perhaps on a follow-up ticket. [See also my comment in SPKG.txt.]


New spkg: http://boxen.math.washington.edu/home/leif/Sage/spkgs/ratpoints-2.1.3.p3.spkg

md5sum: 5e5c2cf5a05196e6146251eb6ccad1b6 ratpoints-2.1.3.p3.spkg

ratpoints-2.1.3.p3 (Leif Leonhardy, March 17th 2012)

  • #12682: Patch Makefile such that the CC (and INSTALL_DIR) environment variable(s) override(s) the setting in the Makefile.
  • Some clean-up; use $MAKE instead of make, also install the library with make (i.e., $MAKE) rather than "by hand".
  • TODO:
    • The Makefile has a test target; don't know whether we should run some tests, and whether it's worth an spkg-check script (for which we'd presumably have to duplicate the whole CCFLAGS* setup).

Attachments (2)

ratpoints-2.1.3.p2-p3.diff (6.4 KB) - added by leif 9 years ago.
Diff between the p2 and my p3. For reference / review only.
ratpoints-2.1.3-Makefile.vanilla-p3.diff (1.1 KB) - added by leif 9 years ago.
Diff between the vanilla Makefile and the (patched) one of the p3. For reference and (better) review only.

Download all attachments as: .zip

Change History (10)

Changed 9 years ago by leif

Diff between the p2 and my p3. For reference / review only.

Changed 9 years ago by leif

Diff between the vanilla Makefile and the (patched) one of the p3. For reference and (better) review only.

comment:1 Changed 9 years ago by leif

  • Authors set to Leif Leonhardy
  • Cc rlm jdemeyer ohanar added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by mjo

  • Cc mjo added

I would just pass the environment variables to $MAKE to keep the size of our patches down. Our goal should be zero patches. The $INSTALL_DIR stuff should be quoted, though. Can we report that upstream?

comment:3 in reply to: ↑ 2 Changed 9 years ago by leif

Replying to mjo:

I would just pass the environment variables to $MAKE to keep the size of our patches down. Our goal should be zero patches. The $INSTALL_DIR stuff should be quoted, though. Can we report that upstream?

Well, of course we could. Using ?= is IMHO also more convenient and less error-prone (although less portable, i.e., not supported by all makes, which probably matters to upstream), especially if you have a couple of invocations of make, and/or a couple of variables to override.

If we submit a patch upstream, we should also add an alias check for the test target, and probably more. I just needed a quick solution... ;-)

The best thing is always(?) to have a configure script which generates the Makefile(s) such that we don't have to patch them. (Cf. Lcalc, which Rishikesh wanted to "autotoolize" a while ago...) For some (small) packages this would of course be a bit overkill.

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

  • Reviewers set to R. Andrew Ohana
  • Status changed from needs_review to positive_review

works well and looks good!

FYI, it doesn't appear as if upstream is too concerned with portability, considering ratpoints uses a few gnu extensions (such as inline functions)

comment:5 Changed 9 years ago by ohanar

  • Keywords rd2 added

comment:6 in reply to: ↑ 4 Changed 9 years ago by jdemeyer

Replying to ohanar:

FYI, it doesn't appear as if upstream is too concerned with portability, considering ratpoints uses a few gnu extensions (such as inline functions)

I think you mean nested functions. "inline functions" are perfectly fine C99 or C++.

comment:7 Changed 9 years ago by ohanar

yes, that is what I meant :)

comment:8 Changed 9 years ago by jdemeyer

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