sage_build_cython: Do not rely on CC environment variable being set
mkoeppe opened this issue · 27 comments
(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:
# * https://github.com/sagemath/sage-prod/issues/14460
# * https://github.com/sagemath/sage-prod/issues/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)
CC: @dimpase @orlitzky @kiwifb
Component: build
Author: Matthias Koeppe
Branch/Commit: 805bf82
Reviewer: François Bissey
Issue created by migration from https://trac.sagemath.org/ticket/30876
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?
Yes, distutils. So distutils should also be used to construct the command line for this version test.
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 20190518allows 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.
>>>
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
If I do
>>> cc=new_compiler(get_default_compiler())
>>> _=cc.compile(["foo.c"], extra_preargs=['-dumpversion'])
8.3.1then 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.)
I've also asked on stackoverflow, nobody knows...
https://stackoverflow.com/questions/64768024/finding-distutils-c-compiler-version
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.
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.
Replying to @orlitzky:
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.
A trivial fix is to just replace $CC by ${CC-gcc}
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.
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.
probably even more relevant than these semi-vanilla cases would be the Azure docker container used in cibuildwheel to build manylinux wheels.
Author: Matthias Koeppe
Description changed:
---
+++
@@ -17,5 +17,5 @@
/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)
-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.
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 |
Thanks. I've squashed/rebased it. Yes, all of it can go away
Reviewer: François Bissey
We should really get that in quick. It has been waiting long enough.
Thanks!
Changed branch from u/mkoeppe/sage_build_cython__do_not_rely_on_cc_environment_variable_being_set to 805bf82