Opened 7 years ago

Closed 7 years ago

#12433 closed defect (fixed)

Make zn_poly respect the CC environment variable, work around GCC 4.7.0 bug on ia64 (and clean up the spkg)

Reported by: ohanar Owned by: GeorgSWeber
Priority: major Milestone: sage-5.2
Component: packages: standard Keywords: spkg hardcoded hard-coded CC CXX compiler sd40.5
Cc: roed Merged in: sage-5.2.beta1
Authors: R. Andrew Ohana, Leif Leonhardy Reviewers: Leif Leonhardy, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The zn_poly spkg currently does not respect the CC environment variable, we should fix this (for instance for #12426).

New spkg: http://boxen.math.washington.edu/home/leif/Sage/spkgs/zn_poly-0.9.p8.spkg

md5sum: f0a331ed68c28b748c2d3ca3bade9865 zn_poly-0.9.p8.spkg

zn_poly-0.9.p8 (Leif Leonhardy, April 20th, 2012)

  • #12433: Further reviewer changes / additions.
  • Work around GCC 4.7.0 bug on ia64 (by almost completely disabling optimi- zation if the compiler is GCC 4.7.x, of course only on that platform); cf. #12751, #12765.
  • Add a patch to avoid conflicting definitions (i.e., redundant typedefs) of ulong, by changing ulong to a macro.
  • Don't hardcode the zn_poly version number in spkg-install; instead read it from the file VERSION, like the generated makefile does.
  • Remove some of the code previously just commented out, as well as some obsolete comments.
  • Document patches (and correct ticket reference in an old changelog entry).

zn_poly-0.9.p7 (Leif Leonhardy, April 8th, 2012)

  • #12433: Reviewer changes.
  • Restore upstream sources. (One file in src/tune/ was already patched.)
  • Remove the obsolete Debian dist/ directory.
  • Use patch to apply the patches.
  • Remove patches/ from .hgignore! (And remove the prepatched files.)
  • Add Python to the dependencies, since (some) Python is needed to create the Makefile during build / configure. (spkg/standard/deps already reflects this.)
  • Rework (upstream's) makemakefile.py to create a proper Makefile, respecting CC, CXX, CFLAGS, CXXFLAGS, CPPFLAGS etc. with their *usual* meaning (i.e., not using CPP to compile C++!), and using LDFLAGS consistently, also not hardcoding e.g. -m64 (which was added by Sage).
  • Do not add -O3 to CFLAGS (in spkg-install) without the possibility to get overridden by user-provided CFLAGS. Also honor SAGE_DEBUG=yes by completely disabling optimization in that case.
  • Fix typo in spkg-check, which certainly would break building the test program when SAGE64=yes. (Although it is actually already built from within spkg-install.)
  • Clean up spkg-install and spkg-check; redirect error messages to stderr, add more error checks, use $MAKE in spkg-check as well, quote more environment variables, use cp [-f] instead of $CP, don't create an absolute symbolic link on Cygwin.

zn_poly-0.9.p6 (R. Andrew Ohana, February 4th, 2012)

  • #12433: Make spkg respect global CC flag

Attachments (6)

zn_poly-0.9.p6.patch (1.7 KB) - added by ohanar 7 years ago.
for review purposes
zn_poly-0.9.p6-p7.diff (58.4 KB) - added by leif 7 years ago.
Diff between Andrew's and my new spkg. For reference / review only.
zn_poly-0.9.p5-p7.diff (58.4 KB) - added by leif 7 years ago.
Diff between the previous spkg in Sage and my new spkg. For reference / review only.
zn_poly-0.9.p7-p8.diff (15.1 KB) - added by leif 7 years ago.
Diff between my p7 and p8 spkgs. For reference / review only.
zn_poly-0.9.p5-p8.diff (63.0 KB) - added by jdemeyer 7 years ago.
Diff between the p5 and p8 spkgs. For reference / review only.
makemakefile.py.patch (6.7 KB) - added by jdemeyer 7 years ago.
New / modified patch to src/makemakefile.py. For easier review only; do not apply.

Download all attachments as: .zip

Change History (21)

Changed 7 years ago by ohanar

for review purposes

comment:1 Changed 7 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to needs_work

Same here: $CC has to be quoted.

comment:3 Changed 7 years ago by leif

Oh, the zn_poly spkg is a real mess... :-)

I'll make a p7 cleaning up spkg-install and spkg-check, removing the obsolete Debian stuff, using patch to apply the patches, and -- more importantly -- modifying makemakefile.py to produce a proper Makefile, respecting CC (and CXX, not CPP!, likewise CXXFLAGS instead of CPPFLAGS, and using CPPFLAGS for what they're meant to be used), using LDFLAGS consistently...

Then we won't have to use $MAKE ... CC=... etc.

Changed 7 years ago by leif

Diff between Andrew's and my new spkg. For reference / review only.

Changed 7 years ago by leif

Diff between the previous spkg in Sage and my new spkg. For reference / review only.

comment:4 Changed 7 years ago by leif

  • Authors set to R. Andrew Ohana, Leif Leonhardy
  • Component changed from build to packages
  • Description modified (diff)
  • Keywords spkg hardcoded hard-coded CC CXX compiler added
  • Status changed from needs_work to needs_review
  • Summary changed from make zn_poly respect global CC flag to Make zn_poly respect the CC environment variable (and clean up the spkg)

Ok, here's something to review... :P

Left a few TODOs, as well as a lot of (hopefully obsolete) code I just commented out right now.

I've attached diffs between the p5, p6 and p7 spkgs, and the new patch to makemakefile.py, too, for easier reviewing. (You don't have to apply any of these.)

comment:5 follow-up: Changed 7 years ago by leif

Personal note: Add a work-around for GCC 4.7.0 on ia64 (cf. #12765).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by leif

Replying to leif:

Personal note: Add a work-around for GCC 4.7.0 on ia64 (cf. #12765).

... and probably add a patch to #define rather than typedef ulong.

comment:7 in reply to: ↑ 6 Changed 7 years ago by leif

  • Cc roed added
  • Description modified (diff)
  • Summary changed from Make zn_poly respect the CC environment variable (and clean up the spkg) to Make zn_poly respect the CC environment variable, work around GCC 4.7.0 bug on ia64 (and clean up the spkg)

Replying to leif:

Replying to leif:

Personal note: Add a work-around for GCC 4.7.0 on ia64 (cf. #12765).

... and probably add a patch to #define rather than typedef ulong.

Did so.

David, does that fix your issue?

Changed 7 years ago by leif

Diff between my p7 and p8 spkgs. For reference / review only.

Changed 7 years ago by jdemeyer

Diff between the p5 and p8 spkgs. For reference / review only.

comment:8 Changed 7 years ago by jdemeyer

  • Keywords sd40.5 added
  • Reviewers changed from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer

Changed 7 years ago by jdemeyer

New / modified patch to src/makemakefile.py. For easier review only; do not apply.

comment:9 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 7 years ago by jdemeyer

Looks good to me. There will be a follow-up at #12751 to remove the GCC-4.7.x Itanium work-around.

comment:11 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:12 follow-up: Changed 7 years ago by Snark

I have two questions :

  1. have the modifications to upstream makemakefile.py been forwarded upstream?
  2. wouldn't it have been simpler to write an autotools-based build system? [to forward upstream too]

comment:13 in reply to: ↑ 12 Changed 7 years ago by jdemeyer

Replying to Snark:

I have two questions :

  1. have the modifications to upstream makemakefile.py been forwarded upstream?

I don't think so.

  1. wouldn't it have been simpler to write an autotools-based build system? [to forward upstream too]

Certainly not simpler. Maybe better, yes. Also, would upstream accept it?

comment:14 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:15 Changed 7 years ago by jdemeyer

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