Opened 2 years ago
Closed 6 months ago
#30876 closed defect (fixed)
sage_build_cython: Do not rely on CC environment variable being set
Reported by: | Matthias Köppe | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-9.7 |
Component: | build | Keywords: | |
Cc: | Dima Pasechnik, Michael Orlitzky, François Bissey | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | François Bissey |
Report Upstream: | N/A | Work issues: | |
Branch: | 805bf82 (Commits, GitHub, GitLab) | Commit: | 805bf8253765f79d39a716c6ef89ae58ed377bd0 |
Dependencies: | Stopgaps: |
Description (last modified by )
(from #30580):
src/sage_setup/command/sage_build_cython.py
contains this code:
# Work around GCC-4.8 bug which miscompiles some sig_on() statements: # * http://trac.sagemath.org/sage_trac/ticket/14460 # * http://trac.sagemath.org/sage_trac/ticket/20226 # * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56982 if subprocess.call("""$CC --version | grep -i 'gcc.* 4[.]8' >/dev/null """, shell=True) == 0: extra_compile_args.append('-fno-tree-copyrename')
This gives an (ignored) error when CC
is not set -- which can happen if invoked outside of sage-env
, for example when building an sdist.
$ python3 -u setup.py --no-user-cfg sdist >/tmp/res /bin/sh: 1: --version: not found
We remove this code, which is obsolete after #33316 (Drop support for GCC < 6.3 in Sage 9.7)
Change History (26)
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
Yes, distutils. So distutils should also be used to construct the command line for this version test.
comment:3 Changed 2 years ago by
it looks doable, but ugly as hell:
from distutils.ccompiler import new_compiler, get_default_compiler new_compiler(get_default_compiler()).compile("foo.c")
and then
$ strings hello.o | grep GCC GCC: (Gentoo 8.3.1-r2 p4) 8.3.1 20190518
allows version extraction.
Wouldn't it be easier to, e.g., parse the banner of Python?
$ python Python 3.7.9 (default, Sep 15 2020, 15:01:07) [GCC 8.3.1 20190518] on linux Type "help", "copyright", "credits" or "license" for more information. >>>
comment:4 Changed 2 years ago by
I think the call to gcc --version
can be implemented in a simple and clean way in sage_build_ext
, which has access to self.compiler.compiler
comment:5 Changed 2 years ago by
If I do
>>> cc=new_compiler(get_default_compiler()) >>> _=cc.compile(["foo.c"], extra_preargs=['-dumpversion']) 8.3.1
then I see the version value. So one can wrap this in a script.
(one can also use --version
instead of -dumpversion
for a longer output.)
comment:6 Changed 2 years ago by
I've also asked on stackoverflow, nobody knows... https://stackoverflow.com/questions/64768024/finding-distutils-c-compiler-version
comment:7 Changed 22 months ago by
Milestone: | sage-9.3 → sage-9.4 |
---|
Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.
comment:8 Changed 18 months ago by
Milestone: | sage-9.4 → sage-9.5 |
---|
comment:9 follow-up: 10 Changed 14 months ago by
Cc: | Michael Orlitzky added |
---|
We already reject gcc-4.7 and older in spkg-configure.m4
. Would anyone notice if we rejected 4.8 (May, 2014) as well?
And if we do want to continue supporting gcc-4.8 from the system, couldn't we append -fno-tree-copyrename
to CFLAGS
to fix this? The optimization is buggy in 4.8.x so I don't think it's unjustified.
comment:10 Changed 14 months ago by
Replying to mjo:
Would anyone notice if we rejected 4.8 (May, 2014) as well?
Yes, this is the compiler on ubuntu-trusty
and centos-7
, which are still widely used.
We are dropping support for a platform with gcc 4.9 (debian-jessie
) in Sage 9.5 (for #25009) because it turns to be more broken than 4.8.
comment:12 Changed 14 months ago by
nobody nowadays is using gcc 4 on Centos 7. There are system packages to get you gcc 9. cf. e.g. https://stackoverflow.com/a/67212990/557937
Same story with Ubuntu, more or less. https://askubuntu.com/a/1149383/309919
We can and should drop gcc 4, without dropping OSs.
comment:13 Changed 14 months ago by
We can and should add variants of fedora/centos with devtoolset
to our tox/GH Actions. So far this is neither tested nor documented anywhere.
comment:15 Changed 14 months ago by
probably even more relevant than these semi-vanilla cases would be the Azure docker container used in cibuildwheel to build manylinux wheels.
comment:16 Changed 13 months ago by
Milestone: | sage-9.5 → sage-9.6 |
---|
comment:17 Changed 9 months ago by
Milestone: | sage-9.6 → sage-9.7 |
---|
comment:18 Changed 6 months ago by
Branch: | → u/mkoeppe/sage_build_cython__do_not_rely_on_cc_environment_variable_being_set |
---|
comment:19 Changed 6 months ago by
Authors: | → Matthias Koeppe |
---|---|
Commit: | → 53c55d20d17b2778473899d366457c4989237c79 |
Description: | modified (diff) |
Status: | new → needs_review |
comment:20 Changed 6 months ago by
Cc: | François Bissey added |
---|
comment:21 Changed 6 months ago by
Is the branch really in the state you want? The last commit basically remove the content of the previous one. Otherwise I am all for removing useless bits.
comment:22 Changed 6 months ago by
Commit: | 53c55d20d17b2778473899d366457c4989237c79 → 805bf8253765f79d39a716c6ef89ae58ed377bd0 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
805bf82 | src/sage_setup/command/sage_build_cython.py: Remove workaround for GCC 4.8
|
comment:24 Changed 6 months ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → positive_review |
We should really get that in quick. It has been waiting long enough.
comment:26 Changed 6 months ago by
Branch: | u/mkoeppe/sage_build_cython__do_not_rely_on_cc_environment_variable_being_set → 805bf8253765f79d39a716c6ef89ae58ed377bd0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
What does happen if CC is not set, how is setup.py getting the compilers to use? Via distutils? Can't CC be set in setup.py if needed?