Opened 2 years ago

Closed 4 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:

Status badges

Description (last modified by Matthias Köppe)

(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 Dima Pasechnik

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?

comment:2 Changed 2 years ago by Matthias Köppe

Yes, distutils. So distutils should also be used to construct the command line for this version test.

comment:3 Changed 2 years ago by Dima Pasechnik

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.
>>> 

Last edited 2 years ago by Dima Pasechnik (previous) (diff)

comment:4 Changed 2 years ago by Matthias Köppe

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 Dima Pasechnik

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 Dima Pasechnik

comment:7 Changed 21 months ago by Matthias Köppe

Milestone: sage-9.3sage-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 16 months ago by Matthias Köppe

Milestone: sage-9.4sage-9.5

comment:9 Changed 12 months ago by Michael Orlitzky

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 in reply to:  9 Changed 12 months ago by Matthias Köppe

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:11 Changed 12 months ago by Matthias Köppe

A trivial fix is to just replace $CC by ${CC-gcc}

comment:12 Changed 12 months ago by Dima Pasechnik

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.

Last edited 12 months ago by Dima Pasechnik (previous) (diff)

comment:13 Changed 12 months ago by Matthias Köppe

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:14 Changed 12 months ago by Matthias Köppe

I've opened #32965/#32966 for this.

comment:15 Changed 12 months ago by Dima Pasechnik

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 12 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

comment:17 Changed 7 months ago by Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:18 Changed 4 months ago by Matthias Köppe

Branch: u/mkoeppe/sage_build_cython__do_not_rely_on_cc_environment_variable_being_set

comment:19 Changed 4 months ago by Matthias Köppe

Authors: Matthias Koeppe
Commit: 53c55d20d17b2778473899d366457c4989237c79
Description: modified (diff)
Status: newneeds_review

New commits:

fd0a71fsrc/sage_setup/command/sage_build_cython.py: Use gcc if CC unset
53c55d2src/sage_setup/command/sage_build_cython.py: Remove workaround for GCC 4.8

comment:20 Changed 4 months ago by Matthias Köppe

Cc: François Bissey added

comment:21 Changed 4 months ago by François Bissey

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 4 months ago by git

Commit: 53c55d20d17b2778473899d366457c4989237c79805bf8253765f79d39a716c6ef89ae58ed377bd0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

805bf82src/sage_setup/command/sage_build_cython.py: Remove workaround for GCC 4.8

comment:23 Changed 4 months ago by Matthias Köppe

Thanks. I've squashed/rebased it. Yes, all of it can go away

comment:24 Changed 4 months ago by François Bissey

Reviewers: François Bissey
Status: needs_reviewpositive_review

We should really get that in quick. It has been waiting long enough.

comment:25 Changed 4 months ago by Matthias Köppe

Thanks!

comment:26 Changed 4 months ago by Volker Braun

Branch: u/mkoeppe/sage_build_cython__do_not_rely_on_cc_environment_variable_being_set805bf8253765f79d39a716c6ef89ae58ed377bd0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.