Opened 10 years ago
Closed 10 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 )
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
) ofulong
, by changingulong
to a macro. - Don't hardcode the zn_poly version number in
spkg-install
; instead read it from the fileVERSION
, like the generatedmakefile
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, respectingCC
,CXX
,CFLAGS
,CXXFLAGS
,CPPFLAGS
etc. with their *usual* meaning (i.e., not usingCPP
to compile C++!), and usingLDFLAGS
consistently, also not hardcoding e.g.-m64
(which was added by Sage). - Do not add
-O3
toCFLAGS
(inspkg-install
) without the possibility to get overridden by user-providedCFLAGS
. Also honorSAGE_DEBUG=yes
by completely disabling optimization in that case. - Fix typo in
spkg-check
, which certainly would break building the test program whenSAGE64=yes
. (Although it is actually already built from withinspkg-install
.) - Clean up
spkg-install
andspkg-check
; redirect error messages tostderr
, add more error checks, use$MAKE
inspkg-check
as well, quote more environment variables, usecp [-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)
Change History (21)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Reviewers set to Leif Leonhardy
- Status changed from needs_review to needs_work
Same here: $CC
has to be quoted.
comment:3 Changed 10 years ago by
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 10 years ago by
Diff between the previous spkg in Sage and my new spkg. For reference / review only.
comment:4 Changed 10 years ago by
- 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: ↓ 6 Changed 10 years ago by
Personal note: Add a work-around for GCC 4.7.0 on ia64 (cf. #12765).
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 10 years ago by
comment:7 in reply to: ↑ 6 Changed 10 years ago by
- 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)
comment:8 Changed 10 years ago by
- Keywords sd40.5 added
- Reviewers changed from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer
Changed 10 years ago by
New / modified patch to src/makemakefile.py
. For easier review only; do not apply.
comment:9 Changed 10 years ago by
- Description modified (diff)
comment:10 Changed 10 years ago by
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 10 years ago by
- Status changed from needs_review to positive_review
comment:12 follow-up: ↓ 13 Changed 10 years ago by
I have two questions :
- have the modifications to upstream makemakefile.py been forwarded upstream?
- wouldn't it have been simpler to write an autotools-based build system? [to forward upstream too]
comment:13 in reply to: ↑ 12 Changed 10 years ago by
Replying to Snark:
I have two questions :
- have the modifications to upstream makemakefile.py been forwarded upstream?
I don't think so.
- 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 10 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:15 Changed 10 years ago by
- Merged in set to sage-5.2.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
for review purposes