#30375 closed enhancement (fixed)

Gather and clean up compiler flags

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.3
Component: build Keywords: compile flags, sd111
Cc: slelievre, dimpase Merged in:
Authors: Jonathan Kliem Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 3bb3099 (Commits, GitHub, GitLab) Commit: 3bb309944b7e8542b2ac88ed3c9d9a60e68644d7
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

We gather and clean up compiler flags. Also we

  • add --enable-fat-binary to configure (as an alternative to setting SAGE_FAT_BINARY)
  • add --enable_debug={no|symbols|yes} as alternative to setting SAGE_DEBUG
  • compile with -Og -g if SAGE_DEBUG=yes
  • compile with -O2 if SAGE_DEBUG=no (some packages with O3 instead)
  • compile with -O2 -g otherwise (some packages with O3 instead)

During the build of Sage there are now flags that can be exported in spkg_install.in with one line, instead of checking SAGE_DEBUG etc. for each individual package, e.g. by

  • export CFLAGS=$CFLAGS (redundant, use flags as above)
  • export CFLAGS=$CFLAGS_O3 (O3 instead of O2, but only if not in debug mode)
  • export CFLAGS=$ORIGINAL_CFLAGS (use $CFLAGS as set before configure)

Likewise we do with CXXFLAGS, FCFLAGS, F77FLAGS.

We try to respect the user's choice and do not overwrite the flags if already set by the user, but only add compile flags if necessary for packages to work.

Inidividual packages can still be compiled with specified flags or in debug mode using

make CFLAGS=-O2 some-package

or

make SAGE_DEBUG="yes" some-package

This ticket prepares #27122, which adds -march=native to the defaults.

Change History (70)

comment:1 Changed 21 months ago by gh-kliem

  • Milestone changed from sage-9.2 to sage-9.3
  • Status changed from new to needs_review

comment:2 Changed 21 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

comment:3 Changed 19 months ago by mkoeppe

One point that probably needs discussion is the following.

When an spkg is built, the current design of sage-spkg is to create a self-contained script spkg-install that sets all necessary environment variables (by sourcing sage-env etc.). (This makes it relatively easy to debug the installation process.)

Making package installs depend on exported environment variables in Makefile.in breaks this design.

It would probably be better to create a new script sage-build-env (to complement sage-build-env-config) to take care of these environment variable settings.

comment:4 follow-up: Changed 19 months ago by gh-kliem

Ok, I think I'm beginning to understand.

Create a new file build/bin/sage-build-env.in and move the stuff that I added to the makefile there?

Then make sure build/bin/sage-build-env is added to the correct places?

Why should I not add the things to build/bin/sage-build-env-config directly?

comment:5 in reply to: ↑ 4 Changed 19 months ago by mkoeppe

Replying to gh-kliem:

Create a new file build/bin/sage-build-env.in and move the stuff that I added to the makefile there?

Then make sure build/bin/sage-build-env is added to the correct places?

Yes

Why should I not add the things to build/bin/sage-build-env-config directly?

Just a suggestion, to keep the analogy to src/bin/sage-env (which implements some logic) and src/bin/sage-env-config (which purely sets some variables). Not very important.

comment:6 Changed 19 months ago by git

  • Commit changed from 2ff56300b6ba53c0588f30e789f30108bf0fb179 to b6cf7c1b6bc23828c26d77eb666d9e2e9661e43b

Branch pushed to git repo; I updated commit sha1. New commits:

b6cf7c1Merge branch 'u/gh-kliem/gather_and_clean_up_compile_flags' of git://trac.sagemath.org/sage into u/gh-kliem/gather_and_clean_up_compile_flags_new

comment:7 Changed 19 months ago by git

  • Commit changed from b6cf7c1b6bc23828c26d77eb666d9e2e9661e43b to 5f4d0b1c9696ea47784348164c4832a5c51178e4

Branch pushed to git repo; I updated commit sha1. New commits:

5f4d0b1bash syntax again

comment:8 Changed 19 months ago by gh-kliem

I think I have mostly figured it out.

But it seems that this is no longer applied. If I configure with some CFLAGS, it seems that when making still the standard CFLAGS are applied. It is consistent with the CFLAGS in sage -sh at least.

comment:9 Changed 19 months ago by git

  • Commit changed from 5f4d0b1c9696ea47784348164c4832a5c51178e4 to a913bbd97fa2deff1daa54c27b6dd5b17996e289

Branch pushed to git repo; I updated commit sha1. New commits:

a913bbdcorrect detextion of empty values

comment:10 Changed 19 months ago by gh-kliem

Ok, seems to work now.

And in the sage -buildsh this thing is run correctly.

comment:11 Changed 19 months ago by mkoeppe

Looking good; but I think sage-build-env would ideally not be a generated file but rather get its settings from sage-build-env-config.

comment:12 Changed 19 months ago by mkoeppe

(If both are generated, there is really no point in splitting into these two files)

comment:13 Changed 19 months ago by git

  • Commit changed from a913bbd97fa2deff1daa54c27b6dd5b17996e289 to 3b82fe901b7141a798b714a4931a852ec09f7eec

Branch pushed to git repo; I updated commit sha1. New commits:

3b82fe9Merge branch 'u/gh-kliem/gather_and_clean_up_compile_flags' of git://trac.sagemath.org/sage into u/gh-kliem/gather_and_clean_up_compile_flags_new

comment:14 Changed 19 months ago by git

  • Commit changed from 3b82fe901b7141a798b714a4931a852ec09f7eec to d979c2251c191e1a661fcfd5cd89dc5f83750fb7

Branch pushed to git repo; I updated commit sha1. New commits:

d979c22do not generate `sage-build-env`; respect environment settings at make time

comment:15 Changed 19 months ago by mkoeppe

Would it not be cleaner if instead of

+# The configured compilation flags.
+if [ "x$CFLAGS" == "x" ]; then
+    export CFLAGS="@ORIGINAL_CFLAGS@"
+fi

in build-env-config, you would unconditionally set a variable ORIGINAL_CFLAGS?

comment:16 Changed 19 months ago by git

  • Commit changed from d979c2251c191e1a661fcfd5cd89dc5f83750fb7 to e898e33f2e3fb3413bc2d94e8e6aa6c94c0abe0f

Branch pushed to git repo; I updated commit sha1. New commits:

e898e33more natural naming

comment:17 Changed 19 months ago by gh-kliem

I don't understand anything at this point anymore.

  • $ export CFLAGS='-march=native'
    $ export CXXFLAGS='-O1'
    $ ./configure
    $ sage --buildsh
    echo $CFLAGS
    -march=native
    echo $CXXFLAGS
    -O1
    
    Okay, this is exactly what we want. But
    export CFLAGS='-march=native'
    ./configure CXXFLAGS='-O1'
    sage --buildsh
    echo $CFLAGS
    -march=native
    echo $CXXFLAGS
    -march=native
    
    Why? build/bin/sage-build-env-config was configured exactly the same. However, if you echo $CXXFLAGS at the start of build/bin/sage-build-env/config, you see that CXXFLAGS was already set to -march=native. Were does this come from?
  • At the moment I overwrite CFLAGS etc. conditionally so that a user may run make CFLAGS='-Og' <some package>. ORIGINAL_CFLAGS should be the same as this value in this case, shouldn't it (and all other related values as well: CFLAGS_O3 etc). Any package may choose any such environment variable and append flags, but should never discard flags the user handed in.

It is really confusing however, that the exported stuff at configure time is called ORIGINAL_CFLAGS, as @CFLAGS@ already tells us that this is from configure time. I changed that.


New commits:

e898e33more natural naming

comment:18 Changed 19 months ago by mkoeppe

Note that also sage-env and sage-env-config are sourced.

comment:19 Changed 18 months ago by mkoeppe

Shall we try to get this into 9.3...?

comment:20 Changed 18 months ago by gh-kliem

Yes, please.

However, I might need some help.

comment:21 Changed 18 months ago by mkoeppe

A suggestion:

Set similar shell variables in build/bin/sage-build-env-config.in that are set in src/bin/sage-env-config.in:

CONFIGURED_CC="@CC@"
CONFIGURED_CXX="@CXX@"
CONFIGURED_FC="@FC@"
CONFIGURED_OBJC="@OBJC@"
CONFIGURED_OBJCXX="@OBJCXX@"

and move all code for modifying environment variables such as CFLAGS to one place, build/bin/sage-build-env...

comment:22 Changed 18 months ago by git

  • Commit changed from e898e33f2e3fb3413bc2d94e8e6aa6c94c0abe0f to 908ded5da2708e491bd5c6238cc8b8f860f54caf

Branch pushed to git repo; I updated commit sha1. New commits:

908ded5Merge branch 'u/gh-kliem/gather_and_clean_up_compile_flags' of git://trac.sagemath.org/sage into u/gh-kliem/gather_and_clean_up_compile_flags_reb

comment:23 Changed 18 months ago by gh-kliem

Merged into develop.

comment:24 follow-up: Changed 18 months ago by gh-kliem

Should I remove the printing of things in build/pkgs/gcc/spkg-configure.m4.

The only thing that really needs to done there, is to check whether -march=native is supported in general (not this ticket). Everything else we redo later anyway.

comment:25 Changed 18 months ago by git

  • Commit changed from 908ded5da2708e491bd5c6238cc8b8f860f54caf to 77bc7433b983a4187c5c36e872f724cb75181f7d

Branch pushed to git repo; I updated commit sha1. New commits:

77bc743move all logic to buil/bin/sage-build-env

comment:26 Changed 18 months ago by git

  • Commit changed from 77bc7433b983a4187c5c36e872f724cb75181f7d to e2023d20fe0ea3475877884a3b36206621223018

Branch pushed to git repo; I updated commit sha1. New commits:

e2023d2removed message in gcc/spkg-configure.m4

comment:27 in reply to: ↑ 24 Changed 18 months ago by gh-kliem

Replying to gh-kliem:

Should I remove the printing of things in build/pkgs/gcc/spkg-configure.m4.

The only thing that really needs to done there, is to check whether -march=native is supported in general (not this ticket). Everything else we redo later anyway.

Ok, I just went ahead an did this. It's a lot simpler now. Almost everything is at one place. configure.ac needs to save the flags and build/bin/sage-build-env-config.in picks them up again as CONFIGURED_CFLAGS etc.

comment:28 Changed 18 months ago by mkoeppe

  • Status changed from needs_review to needs_work

This looks great but I think you'll need to modify build/bin/sage-spkg as well so that the install scripts for normal packages source sage-build-env.

comment:29 Changed 18 months ago by mkoeppe

Also, in sage-build-env, remove spaces around = in export -- this is not valid shell syntax

comment:30 Changed 18 months ago by git

  • Commit changed from e2023d20fe0ea3475877884a3b36206621223018 to 098ca15de64d761f058dd038b66e81780904de2e

Branch pushed to git repo; I updated commit sha1. New commits:

098ca15Merge branch 'u/gh-kliem/gather_and_clean_up_compile_flags' of git://trac.sagemath.org/sage into u/gh-kliem/gather_and_clean_up_compile_flags

comment:31 Changed 18 months ago by git

  • Commit changed from 098ca15de64d761f058dd038b66e81780904de2e to 9c5e8a245aae24288d3a922666c3abbe41a44747

Branch pushed to git repo; I updated commit sha1. New commits:

b6e5b11fixed shell syntax
431d14csource in sage-spkg
9c5e8a2add references to `bin/sage-build-env` and `bin/sage-build-env-config`

comment:32 Changed 18 months ago by git

  • Commit changed from 9c5e8a245aae24288d3a922666c3abbe41a44747 to 4b8933b0aec03722e34fa1a6d2573a22c3edfedc

Branch pushed to git repo; I updated commit sha1. New commits:

4b8933bremove incorrect `$` in `build/bin/sage-build-env`

comment:33 Changed 18 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:34 Changed 18 months ago by mkoeppe

sage-build-env and sage-build-env-config are in build/bin, not src/bin. They should not be listed in src/setup.py, build/pkgs/sagelib/src/setup.py.

comment:35 Changed 18 months ago by mkoeppe

Also the comments in build/make/Makefile.in introduce the wrong path - it should be build/bin

comment:36 Changed 18 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:37 Changed 18 months ago by git

  • Commit changed from 4b8933b0aec03722e34fa1a6d2573a22c3edfedc to 071b8ccc520b1b1c6b419465da76a26cec99f694

Branch pushed to git repo; I updated commit sha1. New commits:

071b8ccRevert "add references to `bin/sage-build-env` and `bin/sage-build-env-config`"

comment:38 Changed 18 months ago by git

  • Commit changed from 071b8ccc520b1b1c6b419465da76a26cec99f694 to c51e752495cf323c27f089c2cd2d870fbd3abad6

Branch pushed to git repo; I updated commit sha1. New commits:

c51e752correct comments to makefile

comment:39 Changed 18 months ago by gh-kliem

  • Status changed from needs_work to needs_review

Sorry, I got confused.


New commits:

c51e752correct comments to makefile

comment:40 Changed 18 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

This seems to work well.

Let's get this merged for wider testing by developers.

comment:41 Changed 18 months ago by gh-kliem

Thank you.

comment:42 follow-up: Changed 18 months ago by gh-kliem

One thing is actually strange.

I guess the gfortran or something else sets the default FCFLAGS to -ffree-form.

I didn't consider this, so efficiently -O2 -g isn't added anymore.

comment:43 in reply to: ↑ 42 Changed 18 months ago by gh-kliem

Replying to gh-kliem:

One thing is actually strange.

I guess the gfortran or something else sets the default FCFLAGS to -ffree-form.

I didn't consider this, so efficiently -O2 -g isn't added anymore.

Anyway, I fixed that in #27122, but can also fix it here, if that makes sense.

comment:44 Changed 18 months ago by mkoeppe

  • Status changed from positive_review to needs_work

Best to fix it here already

comment:45 Changed 18 months ago by git

  • Commit changed from c51e752495cf323c27f089c2cd2d870fbd3abad6 to aa7ecd6ef0fd94e9d3a42c1c496f2138cc76fc91

Branch pushed to git repo; I updated commit sha1. New commits:

aa7ecd6fix that we might have changed $FCFLAGS during configure

comment:46 Changed 18 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:47 Changed 18 months ago by mkoeppe

I'm not sure if I like this fix...

comment:48 Changed 18 months ago by gh-kliem

It works. But it's probably not perfect.

It appears that this is set by the gfortran configure. It is definitely in the compiled configure

for ac_flag in none -ffree-form -FR -free -qfree -Mfree -Mfreeform

I suppose that is enforced by AC_FC_FREEFORM and will end up adding something to FCFLAGS.

comment:49 Changed 18 months ago by gh-kliem

I agree, that this solution changes behavior.

Calling AC_FC_FREEFORM appends -ffree-form. I don't know why. Do we want this or is this just for testing and we really should call AC_SUBST(FCFLAGS) before this?

comment:50 Changed 18 months ago by mkoeppe

  • Cc dimpase added

I think we shouldn't pass on the FCFLAGS discovered by this to avoid subtle change of behavior. The individual packages are in charge of figuring out the flags that they need for the Fortran compiler.

So I think it's best to save and restore the flags around the spkg-configure code

comment:51 Changed 18 months ago by git

  • Commit changed from aa7ecd6ef0fd94e9d3a42c1c496f2138cc76fc91 to 94326db15e38576990696e59347cb857a5ecb73e

Branch pushed to git repo; I updated commit sha1. New commits:

94326dbbetter solution for dealing with -ffree-form

comment:52 Changed 18 months ago by gh-kliem

Actually, replacing AC_SUBST(FCFLAGS, "$FCFLAGS") by AC_SUBST(CONFIGURED_FCFLAGS, "$FCFLAGS"), does the job.

Apparently this is somewhat split into two. First we set FCFLAGS=$FLAGS and later the substitution is triggered. So any changes that are applied later to FCFLAGS will still have an effect, but not if we change the name.

Now that apparently the gfortran configure changes FCLFAGS is a different story.

comment:53 Changed 18 months ago by mkoeppe

Note that FCFLAGS is automatically AC_SUBST'ed as well:

$ grep FCFLAGS config.status
ac_cs_config="'PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig:/usr/local/opt/sqlite/lib/pkgconfig:/usr/local/opt/readline/lib/pkgconfig:/usr/local/opt/openssl/lib/pkgconfig:/usr/local/opt/openblas/lib/pkgconfig:/usr/local/lib/pkgconfig:' 'FCFLAGS=-g -g -g'"
  set X /bin/sh './configure'  'PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig:/usr/local/opt/sqlite/lib/pkgconfig:/usr/local/opt/readline/lib/pkgconfig:/usr/local/opt/openssl/lib/pkgconfig:/usr/local/opt/openblas/lib/pkgconfig:/usr/local/lib/pkgconfig:' 'FCFLAGS=-g -g -g' $ac_configure_extra_args --no-create --no-recursion
S["FCFLAGS"]="-g -g -g -ffree-form"
S["CONFIGURED_FCFLAGS"]="-g -g -g"

I would much prefer the solution using save/restore of flags instead of introducing a new substitution variable. This would keep things closer to automake.

comment:54 follow-up: Changed 18 months ago by gh-kliem

From https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Fortran-Compiler.html

The AC_FC_FREEFORM tries to ensure that the Fortran compiler ($FC) allows free-format source code (as opposed to the older fixed-format style from Fortran 77). If necessary, it may add some additional flags to FCFLAGS.

So, AC_FC_FREEFORM has added -ffree-form to my flags, because it feels that this is needed. So I think, we should either respect that or not call AC_FC_FREEFORM.

If we want to respect this (starting with this ticket), what do you think would be the best way? Somehow I must store that FCFLAGS wasn't set by the user and I may add our defaults to this.

comment:55 in reply to: ↑ 54 Changed 18 months ago by mkoeppe

Replying to gh-kliem:

From https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Fortran-Compiler.html

The AC_FC_FREEFORM tries to ensure that the Fortran compiler ($FC) allows free-format source code (as opposed to the older fixed-format style from Fortran 77). If necessary, it may add some additional flags to FCFLAGS.

So, AC_FC_FREEFORM has added -ffree-form to my flags, because it feels that this is needed. So I think, we should either respect that or not call AC_FC_FREEFORM.

We run this to find out whether the configured Fortran compiler has a free form mode.

We do this for our decision whether we have to install gfortran or not.

But it must be up to the individual packages how they invoke the Fortran compiler.

Last edited 18 months ago by mkoeppe (previous) (diff)

comment:56 Changed 18 months ago by git

  • Commit changed from 94326db15e38576990696e59347cb857a5ecb73e to 87dae61577f33d1572ae140ee32fe32b3470e7d4

Branch pushed to git repo; I updated commit sha1. New commits:

87dae61keep FCFLAGS during gfortran configure

comment:57 Changed 18 months ago by gh-kliem

Ok. I'm storing and restoring the FCFLAGS during gfortran configure now. Defining a macro was the only way, I could enforce the order. But maybe there is a better way of doing this.

comment:58 Changed 18 months ago by mkoeppe

That's fine, yes, AC_FC_FREEFORM is executed out of order.

These lines:

+    sage_saved_fcflags=$FCFLAGS
+    FCFLAGS=$sage_saved_fcflags

need quoting around the assigned value.

comment:59 follow-up: Changed 18 months ago by mkoeppe

Also these macro calls are better without the use of the second argument (look at the generated shell code)

+# Save compiler flags as configured by the user.
+AC_SUBST(CFLAGS, "$CFLAGS")
+AC_SUBST(CXXFLAGS, "$CXXFLAGS")
+AC_SUBST(FCFLAGS, "$FCFLAGS")
+AC_SUBST(F77FLAGS, "$F77FLAGS")

comment:60 in reply to: ↑ 59 ; follow-up: Changed 18 months ago by gh-kliem

Replying to mkoeppe:

Also these macro calls are better without the use of the second argument (look at the generated shell code)

+# Save compiler flags as configured by the user.
+AC_SUBST(CFLAGS, "$CFLAGS")
+AC_SUBST(CXXFLAGS, "$CXXFLAGS")
+AC_SUBST(FCFLAGS, "$FCFLAGS")
+AC_SUBST(F77FLAGS, "$F77FLAGS")

Your proposed change does not work. AC_PROG_CC() changes the value of CFLAGS if not set. AC_SUBST(CFLAGS, "$CFLAGS") will implicitly set CFLAGS (even though possibly to the empty value), but prevents AC_PROG_CC from messing with it.

comment:61 Changed 18 months ago by git

  • Commit changed from 87dae61577f33d1572ae140ee32fe32b3470e7d4 to 7577dd41f29981722e970606664cfc8651b6a6bb

Branch pushed to git repo; I updated commit sha1. New commits:

7577dd4added quotes

comment:62 in reply to: ↑ 60 Changed 18 months ago by gh-kliem

Replying to gh-kliem:

Replying to mkoeppe:

Also these macro calls are better without the use of the second argument (look at the generated shell code)

+# Save compiler flags as configured by the user.
+AC_SUBST(CFLAGS, "$CFLAGS")
+AC_SUBST(CXXFLAGS, "$CXXFLAGS")
+AC_SUBST(FCFLAGS, "$FCFLAGS")
+AC_SUBST(F77FLAGS, "$F77FLAGS")

Your proposed change does not work. AC_PROG_CC() changes the value of CFLAGS if not set. AC_SUBST(CFLAGS, "$CFLAGS") will implicitly set CFLAGS (even though possibly to the empty value), but prevents AC_PROG_CC from messing with it.

I could deal with it the same way, I did with the FCFLAGS and save all of those flags beforehand. Maybe that is cleaner.

comment:63 Changed 18 months ago by gh-kliem

  • Keywords sd111 added

Maybe we can take care of this.

comment:64 Changed 17 months ago by gh-kliem

  • Status changed from needs_review to needs_work
  • Work issues set to make variables SAGE_DEBUG and SAGE_FAT_BINARY precious

comment:65 Changed 17 months ago by git

  • Commit changed from 7577dd41f29981722e970606664cfc8651b6a6bb to 3bb309944b7e8542b2ac88ed3c9d9a60e68644d7

Branch pushed to git repo; I updated commit sha1. New commits:

3bb3099prepare preciouos variables

comment:66 Changed 17 months ago by gh-kliem

  • Status changed from needs_work to needs_review
  • Work issues make variables SAGE_DEBUG and SAGE_FAT_BINARY precious deleted

Ok, at least it is prepared for precious variables.

#29507 would have to take care of using this.

comment:67 Changed 17 months ago by gh-kliem

Actually, it kind of works.

During make, reconfiguration will be done using ./config.status and this one does not alter the variables.

comment:68 Changed 17 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Looking great, let's get this in.

comment:69 Changed 17 months ago by gh-kliem

Thanks again.

comment:70 Changed 17 months ago by vbraun

  • Branch changed from u/gh-kliem/gather_and_clean_up_compile_flags to 3bb309944b7e8542b2ac88ed3c9d9a60e68644d7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.