Opened 9 years ago

Closed 4 years ago

#12443 closed defect (invalid)

the sage library doesn't respect global CC and CXX flags

Reported by: ohanar Owned by: GeorgSWeber
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: Merged in:
Authors: Reviewers: R. Andrew Ohana, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Apply trac12443.patch to the Sage library.

Attachments (1)

trac12443.patch (5.3 KB) - added by ohanar 9 years ago.
updated to address issues with earlier version

Download all attachments as: .zip

Change History (29)

comment:1 Changed 9 years ago by ohanar

  • Description modified (diff)
  • Status changed from new to needs_review

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

  • Authors set to R. Andrew Ohana
  • Status changed from needs_review to needs_work

This will break if $CC includes arguments:

compilerinclude = Popen([os.environ['CC'], '--print-file-name=include'], stdout=PIPE)

What's the point of the use_gnu, use_sun stuff?? What does "tools" do anyway?

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

Replying to jdemeyer:

This will break if $CC includes arguments:

compilerinclude = Popen([os.environ['CC'], '--print-file-name=include'], stdout=PIPE)

True, thankfully this is a trivial fix if we assume there are no spaces in compiler path.

What's the point of the use_gnu, use_sun stuff?? What does "tools" do anyway?

By default scons searches the system and to set up its environment, if you specify tools it skips this search and just sets stuff up with the given tools. Currently all this does is speed up the configuring process, but this can be used to make sage respect the CC, CXX, LD, AR,... variables.

comment:4 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:5 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

comment:6 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The SConstruct file doesn't work. First make a symbolic link "gplusplus" to "g++". Then:

jdemeyer@arcanis:/usr/local/src/sage-5.0.beta1$ gplusplus --version
gplusplus (GCC) 4.6.3 20120123 (prerelease)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

jdemeyer@arcanis:/usr/local/src/sage-5.0.beta1$ CXX="gplusplus" ./sage -b

----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
AttributeError: SConsEnvironment instance has no attribute 'SharedLibrary':
  File "/usr/local/src/sage-5.0.beta1/devel/sage-main/c_lib/SConstruct", line 138:
    lib = env.SharedLibrary( "csage", [ "src/" + x for x in srcFiles ],
ERROR: There was an error building c_lib.

So, I think

# the following two lines should probably be replaced with a better test

should actually be done. You should check features, not names of executables.

comment:7 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, I've updated the patch so that it should fix the issue you were having. This ran successfully for me:

$ CC="gplusplus -x none" MAKE="make -j8" ./sage -ba

I still don't actually check for suncc since I don't have any (open)solaris system with suncc installed to make a good check, so I just put a guess at what the check might look like.

comment:8 follow-up: Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I really don't understand the utter brokenness of this "tools" variable. I should show this to every person who claims that autoconf sucks.

Anyway, c_lib still doesn't build with clang:

bsd:sage-5.0.beta4-gcc jdemeyer$ CC=clang ./sage -b

----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
scons: *** [libcsage.dylib] Source file: src/convert.c is static and is not compatible with shared target: libcsage.dylib
ERROR: There was an error building c_lib.

comment:9 Changed 9 years ago by jdemeyer

The Sun compilers don't understand --version. They do report their version with -V. The output is something like

cc: Sun C 5.9 SunOS_sparc Patch 124867-01 2007/07/12

or

c99: Sun C 5.10 SunOS_i386 2009/06/03

So grepping for "sunos" should be fine.

Changed 9 years ago by ohanar

updated to address issues with earlier version

comment:10 in reply to: ↑ 8 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Anyway, c_lib still doesn't build with clang

Should be fixed now, although sage still doesn't build with clang (although I'm fairly confident this is due to other spkgs at this point).

I also threw in something to test for suncc, but I can't test it, so if you can let me know how it works, that would be great.

comment:11 Changed 9 years ago by leif

Related, and equally important:

If CFLAGS are set (even to ""), Python's CFLAGS are dropped, which include -fno-strict-aliasing, which means that you currently always have to manually add -fno-strict-aliasing when setting/modifying CFLAGS (and building with GCC), since otherwise invalid code is generated for (at least some) Cython modules.

(-fstrict-aliasing in contrast is enabled by most of GCC's O-levels.)


Luckily, if you build Python with CC or CXX set, the Sage library just picks up these settings, so I personally never had trouble with the main topic of this ticket, while other spkgs (like Lcalc, ratpoints, zn_poly and even Singular!) still suck...

comment:12 follow-up: Changed 8 years ago by leif

Should we rename the ticket to "Building c_lib with SCons doesn't respect CC and CXX settings"? :-P

(And is there anything in Sage besides PolyBoRi that does use SCons, too?)

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jdemeyer

Replying to leif:

And is there anything in Sage besides PolyBoRi that does use SCons, too?

According to spkg/standard/deps, no.

So we could consider to stop using SCons for c_lib, since it only adds complications compared to autotools (which respects CC and CXX out of the box).

comment:14 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by ohanar

Replying to jdemeyer:

So we could consider to stop using SCons for c_lib, since it only adds complications compared to autotools (which respects CC and CXX out of the box).

I think felix is planning on doing this as part of his gsoc.

comment:15 follow-ups: Changed 8 years ago by leif

IMHO even a simple Makefile would do better, probably (but not necessarily) also using libtool.

comment:16 in reply to: ↑ 15 Changed 8 years ago by leif

Replying to leif:

IMHO even a simple Makefile would do better, probably (but not necessarily) also using libtool.

(I.e., the few C and C++ modules could be built with default rules, only creating the shared library from them needs the appropriate platform-specific flags, AFAICS.)

comment:17 in reply to: ↑ 14 Changed 8 years ago by leif

Replying to ohanar:

I think felix is planning on doing this as part of his gsoc.

Doesn't seem he already had a trac account...

comment:18 in reply to: ↑ 15 Changed 8 years ago by felixs

Replying to leif:

IMHO even a simple Makefile would do better, \[..\]

better than scons. autotools does more than that. i think VPATH builds, dependency tracking and dist-helpers are absolutely worthwile...

comment:19 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:20 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:21 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:22 Changed 7 years ago by leif

ping -c5

comment:23 Changed 7 years ago by ohanar

See also #15594

comment:24 Changed 7 years ago by malb

  • Status changed from needs_review to needs_work

This isn't a git branch

comment:25 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:26 Changed 5 years ago by fbissey

Is this still valid?

comment:27 Changed 5 years ago by jdemeyer

  • Authors R. Andrew Ohana deleted
  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Reviewers changed from Jeroen Demeyer to R. Andrew Ohana, Jeroen Demeyer
  • Status changed from needs_work to positive_review

Sage is built using distutils and distutils supports standard environment variables like CC. It does use CC instead of CXX for compiling C++ files but that is a long-standing upstream issue (it's not the job of Sage to fix that).

comment:28 Changed 4 years ago by vbraun

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.