Opened 15 months ago

Last modified 6 weeks ago

#30876 new defect

sage_build_cython: Do not rely on CC environment variable being set

Reported by: mkoeppe Owned by:
Priority: minor Milestone: sage-9.6
Component: build Keywords:
Cc: dimpase, mjo Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

(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

Change History (16)

comment:1 Changed 15 months ago by dimpase

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 15 months ago by mkoeppe

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

comment:3 Changed 15 months ago by dimpase

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 15 months ago by dimpase (previous) (diff)

comment:4 Changed 15 months ago by mkoeppe

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 15 months ago by dimpase

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 15 months ago by dimpase

comment:7 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.3 to 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 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:9 follow-up: Changed 8 weeks ago by mjo

  • Cc mjo 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 8 weeks ago by mkoeppe

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 8 weeks ago by mkoeppe

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

comment:12 Changed 8 weeks ago by dimpase

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 8 weeks ago by dimpase (previous) (diff)

comment:13 Changed 8 weeks ago by mkoeppe

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 8 weeks ago by mkoeppe

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

comment:15 Changed 8 weeks ago by dimpase

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 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6
Note: See TracTickets for help on using tickets.