Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#22646 closed enhancement (fixed)

./configure CC=/path/to/gcc ...

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.1
Component: build Keywords:
Cc: jdemeyer, fbissey Merged in:
Authors: François Bissey Reviewers: Matthias Koeppe, Jeroen Demeyer, Vincent Delecroix, John Palmieri, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 99eb31f (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

Standard autotools packages allow variables such as CC to be set at configure time using

./configure CC=/path/to/gcc

In this ticket, we add support for this. (The syntax already works, but the settings right now only end up in the unused file build/make/Makefile-auto.)

New configure tarball: https://trac.sagemath.org/raw-attachment/ticket/22646/configure-235.tar.gz

Attachments (1)

configure-235.tar.gz (95.7 KB) - added by fbissey 2 years ago.

Download all attachments as: .zip

Change History (136)

comment:1 Changed 3 years ago by mkoeppe

  • Cc fbissey added

comment:2 Changed 3 years ago by fbissey

I guess that should be extended to CXX, FC, possibly F77 and CPP (although I actually don't recommend to set that one and let the package decides unless it require hand holding. That's from my experience setting it in sage-env that sometimes backfire because $CC -E may work slightly differently and is what many package assume instead of plain cpp.

Would that need to over-ride sage-env if sage's gcc has been installed? Useful for mad scientist experiments but probably not for common users.

comment:3 Changed 3 years ago by mkoeppe

Yes, all of CC, CXX, FC, etc. As a guideline, one can look into the list of variables that appear in an automake-generated Makefile.

In my opinion, if the user provides a specific CC, CXX in this way and configure finds out that they are not suitable, that should be an error.

A gcc package should only be installed if no CC, CXX were provided in this way by the user. Only in that case, sage-env should do its special magic to set these variables to have the sage gcc used.

comment:4 Changed 3 years ago by fbissey

OK, past experience shows that if you set something special at build time, it is forgotten at runtime (see #21701) so we will need to leverage sage-env-config, considering the magic in sage-env that means moving quite a bit of stuff.

comment:5 Changed 3 years ago by mkoeppe

Yes, sage-env-config is what needs to be used.

comment:6 Changed 3 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/compilers
  • Commit set to 483106af4a85ea41461791619dab3f1cef9daa11

This is a first draft for this. I am dropping the setting for CPP. If there are packages, not using autotools, that need it to be set, I'll reconsider.


New commits:

483106aFirst draft of moving compiler settings to sage-env-config

comment:7 Changed 3 years ago by fbissey

  • Status changed from new to needs_review

OK I am looking to make #12426 on this soon and rework slightly the toolchain building in the case were we only build gfortran. In the absence of further comments, I am putting this for review. I'll attach a configure tarball.

comment:8 Changed 3 years ago by fbissey

  • Description modified (diff)

comment:9 Changed 3 years ago by fbissey

  • Description modified (diff)

comment:10 Changed 2 years ago by git

  • Commit changed from 483106af4a85ea41461791619dab3f1cef9daa11 to 945e8243c7fccc612354b616904ac1e2ef51c8ae

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

d6e87c8Merge branch 'develop' into compiler
6048dffForgot to commit the new configure tarball
f7eff64Merge branch 'develop' into compiler
945e824bump config tarball

comment:11 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:12 Changed 2 years ago by git

  • Commit changed from 945e8243c7fccc612354b616904ac1e2ef51c8ae to 32a0b00e746fd57a0830efc7176a6b8502c824fa

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

32a0b00one AC_SUBST per variable only :(

comment:13 Changed 2 years ago by git

  • Commit changed from 32a0b00e746fd57a0830efc7176a6b8502c824fa to 11dc29ac731fcf912a739f9ba8ec7fd5c66b46e8

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

11dc29aconfigure/automake doesn't like variables only defined in conditionals

comment:14 Changed 2 years ago by git

  • Commit changed from 11dc29ac731fcf912a739f9ba8ec7fd5c66b46e8 to ef9f1b37c462bb61c036262a23b3588292d8f392

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

ef9f1b3missing "fi" in sage-env-config.in

comment:15 follow-ups: Changed 2 years ago by dimpase

regarding CPP, there are some packages (gf2x) that need CPP and CXXCPP to be set on FreeBSD.

comment:16 in reply to: ↑ 15 Changed 2 years ago by fbissey

Replying to dimpase:

regarding CPP, there are some packages (gf2x) that need CPP and CXXCPP to be set on FreeBSD.

Is it because CC/CXX is different from what you need for C{,XX}PP? Or is there something missing in configure? By default AC_PROG_C{,XX}PP check C{C,XX} -E first.

comment:17 Changed 2 years ago by fbissey

Just looked over at gf2x' git repo. configure.ac doesn't check for CPP or CXXPP we may want to get that done for the next release. In the meantime, I'd rather have individual spkg-install set it as needed, because I don't want to interfere with what configure script may be looking for.

The thing that I particularly don't like is

if [ -x "$SAGE_LOCAL/bin/cpp" ]; then
    CPP=cpp
fi

in sage-env. Without it package using autoconf should naturally use gcc -E, but because of this, when you install sage's gcc you get cpp.

comment:18 in reply to: ↑ 15 Changed 2 years ago by fbissey

Replying to dimpase:

regarding CPP, there are some packages (gf2x) that need CPP and CXXCPP to be set on FreeBSD.

Reported to gf2x upstream at https://gforge.inria.fr/tracker/index.php?func=detail&aid=21377&group_id=1874&atid=6979

comment:19 follow-up: Changed 2 years ago by vdelecroix

I would like to test this with gcc-6 because my defafult gcc 7.1 is causing troubles (see this sage thread). Is that all what I should do on a freshly cloned Sage?

$ ./bootstrap
$ ./configure CC=/usr/bin/gcc-6 CXX=/usr/bin/g++-6
$ make

comment:20 in reply to: ↑ 19 Changed 2 years ago by fbissey

Replying to vdelecroix:

I would like to test this with gcc-6 because my defafult gcc 7.1 is causing troubles (see this sage thread). Is that all what I should do on a freshly cloned Sage?

$ ./bootstrap
$ ./configure CC=/usr/bin/gcc-6 CXX=/usr/bin/g++-6
$ make

Pretty much.

$ ./bootstrap
$ CC=/usr/bin/gcc-6 CXX=/usr/bin/g++-6 make

should work too. Actually it would work without this ticket. But without this ticket once sage is built your settings for CC/CXX/FC is forgotten. You may want to read #21701 on the potential consequences of that amnesia.

comment:21 Changed 2 years ago by vdelecroix

I am trying the ./configure way right now. I will report in few hours.

comment:22 Changed 2 years ago by vdelecroix

... even sooner, compilation started but then Sage tries to download gcc-5.4! Is that expected? Should I also set SAGE_INSTALL_GCC=no somewhere?

comment:23 Changed 2 years ago by fbissey

I was writing that when your comment hit: Something I didn't think about. sage may try to build its own gcc because the gfortran version is not the same as gcc.

Either provide the matching gfortran or uses #12426 which is build on top of this ticket and decouples gfortran (because clang).

comment:24 follow-up: Changed 2 years ago by vdelecroix

It seems that archlinux do not provide the matching gfortran eventhough there is the Fortran runtime libraries for gcc-6... will try #12426

comment:25 in reply to: ↑ 24 ; follow-up: Changed 2 years ago by dimpase

Replying to vdelecroix:

It seems that archlinux do not provide the matching gfortran eventhough there is the Fortran runtime libraries for gcc-6... will try #12426

sometimes gcc can be called to compile fortran, e.g. something like gcc6 -x f95 might be able to compile fortran.

comment:26 in reply to: ↑ 25 Changed 2 years ago by fbissey

Replying to dimpase:

Replying to vdelecroix:

It seems that archlinux do not provide the matching gfortran eventhough there is the Fortran runtime libraries for gcc-6... will try #12426

sometimes gcc can be called to compile fortran, e.g. something like gcc6 -x f95 might be able to compile fortran.

You would need f951 to be installed for that to work. gcc, g++ and gfortran are front end compiler drivers. cc1, cxx1 and f951 are the real compilers. When you do something like gcc -x f95 you are basically instructing the main compiler driver to drive for f951 rather than cc1. f951 still needs to be present (usually in /usr/libexec/gcc/$host/$version/).

Last edited 2 years ago by fbissey (previous) (diff)

comment:27 Changed 2 years ago by vdelecroix

Success (for both make build and sage -i polymake)! Tell me if you want me to test more stuff.

comment:28 Changed 2 years ago by fbissey

Well, I'd like you to run the test suite (make ptestlong). Of course you ended up using the full #12426.

comment:29 Changed 2 years ago by vdelecroix

On top of #12426, ptestlong gives

----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

comment:30 Changed 2 years ago by git

  • Commit changed from ef9f1b37c462bb61c036262a23b3588292d8f392 to a50d44f8da9bf9d1408e8cb945f07d227c884fb9

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

d76da7bMerge branch 'develop' into compiler
a50d44fbump config tarball

comment:31 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:32 Changed 2 years ago by dimpase

I am trying

 ./configure CC=clang CXX=clang++

on Fedora 23 (clang 3.7) with SAGE_INSTALL_GCC=no and I still see it doing some g++ tests:

...
checking for complex.h... yes
checking whether clang++ supports C++11 features by default... no
checking whether clang++ supports C++11 features with -std=gnu++11... yes
checking for clang option to accept ISO C99... none needed
checking for Fortran flag needed to accept free-form source... -ffree-form
checking if gcc accepts -dumpversion option... yes
checking gcc version... 4.2.1
checking if g++ accepts -dumpversion option... yes
checking g++ version... 4.2.1
checking for sqrt in -lm... yes
...

And I don't even get how it gets 4.2.1 there, as gcc/g++/gfortran installed is 5.3.1. Is it "gcc emulation mode"?!

comment:33 Changed 2 years ago by fbissey

clang fakes gcc. Not any version of gcc, just 4.2.1. Same thing on OS X. So this is totally expected. I wouldn't call it an emulation mode. clang just happen to support all the pragmas identifying itself as gcc 4.2.1.

comment:34 Changed 2 years ago by dimpase

So this ends up with a silly error in libfplll:

...
checking dependency style of clang... (cached) gcc3
checking whether make sets $(MAKE)... (cached) yes
checking whether clang++ supports C++11 features by default... no
checking whether clang++ supports C++11 features with -std=c++11... no
checking whether clang++ supports C++11 features with -std=c++0x... no
checking whether clang++ supports C++11 features with +std=c++11... no
checking whether clang++ supports C++11 features with -h std=c++11... no
configure: error: *** A compiler with support for C++11 language features is required.
Error configuring fplll
...

Is this known, are there any patches needed?

comment:35 follow-up: Changed 2 years ago by fbissey

I have no problem with clang 3.9. It looks like 3.7 may not have enough features. You may have noticed that sage's configure said that clang 3.7 could support c+11 with -std=gnu++11. It didn't check with -std=c++11. However fplll doesn't want gnu++11

AX_CXX_COMPILE_STDCXX([11],[noext],[mandatory])

the noext in fplll's configure.ac prevents you from using gnu++11. It would probably work though.

comment:36 Changed 2 years ago by dimpase

As well, I have some linking gevalt with flint:

...
clang  -r ../build/ulong_extras/mulmod_precomp.lo ../build/ulong_extras/factor_trial_partial.lo ../build/ulong_extras/sizeinbase.lo .
./build/ulong_extras/randbits.lo ../build/ulong_extras/is_square.lo ../build/ulong_extras/inlines.lo ../build/ulong_extras/clog.lo ..
/build/ulong_extras/mulmod2_preinv.lo ../build/ulong_extras/factor_SQUFOF.lo ../build/ulong_extras/cbrt_newton_iteration.lo ../build/
ulong_extras/nth_prime_bounds.lo ../build/ulong_extras/is_probabprime_lucas.lo ../build/ulong_extras/primes_clear.lo ../build/ulong_e
xtras/jacobi.lo ../build/ulong_extras/factor_partial.lo ../build/ulong_extras/mod2_precomp.lo ../build/ulong_extras/flog.lo ../build/
ulong_extras/is_probabprime_fibonacci.lo ../build/ulong_extras/factorial_mod2_preinv.lo ../build/ulong_extras/invmod.lo ../build/ulon
g_extras/factor_pp1.lo ../build/ulong_extras/prime_inverses_arr_readonly.lo ../build/ulong_extras/is_probabprime_BPSW.lo ../build/ulo
ng_extras/powmod_preinv.lo ../build/ulong_extras/gcdinv.lo ../build/ulong_extras/randprime.lo ../build/ulong_extras/rootrem.lo ../bui
ld/ulong_extras/powmod_precomp.lo ../build/ulong_extras/randint.lo ../build/ulong_extras/is_prime_pseudosquare.lo ../build/ulong_extras/primes_init.lo ../build/ulong_extras/primes_sieve_range.lo ../build/ulong_extras/mod2_preinv.lo ../build/ulong_extras/is_perfect_power235.lo ../build/ulong_extras/moebius_mu.lo ../build/ulong_extras/sqrtrem.lo ../build/ulong_extras/factor_insert.lo ../build/ulong_extras/revbin.lo ../build/ulong_extras/mod_precomp.lo ../build/ulong_extras/cbrt_estimate.lo ../build/ulong_extras/primitive_root_prime.lo ../build/ulong_extras/factor.lo ../build/ulong_extras/factor_trial.lo ../build/ulong_extras/is_strong_probabprime2_preinv.lo ../build/ulong_extras/lll_mod_preinv.lo ../build/ulong_extras/is_prime_pocklington.lo ../build/ulong_extras/pow.lo ../build/ulong_extras/factor_power235.lo ../build/ulong_extras/sqrt.lo ../build/ulong_extras/remove2_precomp.lo ../build/ulong_extras/sqrtmodn.lo ../build/ulong_extras/root_estimate.lo ../build/ulong_extras/cbrtrem.lo ../build/ulong_extras/discrete_log_bsgs.lo ../build/ulong_extras/prime_pi.lo ../build/ulong_extras/sqrtmod_primepow.lo ../build/ulong_extras/randlimb.lo ../build/ulong_extras/sqrtmod.lo ../build/ulong_extras/root.lo ../build/ulong_extras/divrem2_precomp.lo ../build/ulong_extras/factorial_fast_mod2_preinv.lo ../build/ulong_extras/cbrt.lo ../build/ulong_extras/remove.lo ../build/ulong_extras/is_probabprime.lo ../build/ulong_extras/is_squarefree.lo ../build/ulong_extras/is_probabprime_fermat.lo ../build/ulong_extras/randtest.lo ../build/ulong_extras/primes_arr_readonly.lo ../build/ulong_extras/primes_jump_after.lo ../build/ulong_extras/is_prime.lo ../build/ulong_extras/nextprime.lo ../build/ulong_extras/compute_primes.lo ../build/ulong_extras/gcd.lo ../build/ulong_extras/nth_prime.lo ../build/ulong_extras/factor_lehman.lo ../build/ulong_extras/ll_mod_preinv.lo ../build/ulong_extras/prime_pi_bounds.lo ../build/ulong_extras/primes_extend_small.lo ../build/ulong_extras/is_strong_probabprime_precomp.lo ../build/ulong_extras/is_oddprime_binary.lo ../build/ulong_extras/cbrt_chebyshev_approximation.lo ../build/ulong_extras/cleanup_primes.lo ../build/ulong_extras/euler_phi.lo ../build/ulong_extras/factor_trial_range.lo ../build/ulong_extras/factor_one_line.lo ../build/ulong_extras/cbrt_binary_search.lo ../build/ulong_extras/xgcd.lo ../build/ulong_extras/is_oddprime_small.lo ../build/ulong_extras/powmod2_preinv.lo ../build/ulong_extras/mulmod_preinv.lo -o ../build/ulong_extras/../ulong_extras.lo -nostdlib
clang: warning: argument unused during compilation: '-r'
/usr/bin/ld: warning: cannot find entry symbol _start; defaulting to 00000000004001f0
../build/ulong_extras/is_square.lo: In function `n_is_square':
/home/scratch/dimpase/sage/sage-clang/local/var/tmp/sage/build/flint-2.5.2.p1/src/ulong_extras/is_square.c:54: undefined reference to `sqrt'
...

Is this all due to -stdlib=libstdc++ ?

comment:37 follow-up: Changed 2 years ago by fbissey

Ha... the fix for compiling flint on debian 9 with pie breaks that compiler... That's because flint insist on not using autotools (duck for cover).

I don't know how your clang 3.7 is built, it may also be using g++ libstdc++ in which case the version of g++ is important for c++11 support. Which version of gcc/g++ is this?

comment:38 follow-up: Changed 2 years ago by dimpase

with giac it's definitely some lib(std)c++-related trouble:

./index.h:572:11: error: no template named 'hash_map' in namespace 'std'; did you mean '__gnu_cxx::hash_map'?
  typedef std::hash_map< index_t,index_m,hash_function_object > hash_index ;
          ^~~~~~~~~~~~~
          __gnu_cxx::hash_map

Should we abandon efforts to run on clang++ with libstdc++, and only allow libc++ with clang++ ?

comment:39 in reply to: ↑ 37 Changed 2 years ago by dimpase

Replying to fbissey:

Ha... the fix for compiling flint on debian 9 with pie breaks that compiler... That's because flint insist on not using autotools (duck for cover).

hear, hear, @wbhart :-)

I don't know how your clang 3.7 is built, it may also be using g++ libstdc++ in which case the version of g++ is important for c++11 support. Which version of gcc/g++ is this?

It's g++ 5.3.1.

comment:40 in reply to: ↑ 38 Changed 2 years ago by fbissey

Replying to dimpase:

with giac it's definitely some lib(std)c++-related trouble:

./index.h:572:11: error: no template named 'hash_map' in namespace 'std'; did you mean '__gnu_cxx::hash_map'?
  typedef std::hash_map< index_t,index_m,hash_function_object > hash_index ;
          ^~~~~~~~~~~~~
          __gnu_cxx::hash_map

Should we abandon efforts to run on clang++ with libstdc++, and only allow libc++ with clang++ ?

Possibly. More checks to devise. That clang 3.7 is obviously not good enough.

comment:41 Changed 2 years ago by kcrisman

I'm trying this to test #12426 but I get a download error. I already created an upstream/ directory and placed the tarball on this ticket on it, so I'm not sure why it's even trying to download it. That said, I started completely from scratch with a new git clone. Error was mine, or my browser's, perhaps.

Last edited 2 years ago by kcrisman (previous) (diff)

comment:42 in reply to: ↑ 35 Changed 2 years ago by dimpase

Replying to fbissey:

I have no problem with clang 3.9. It looks like 3.7 may not have enough features. You may have noticed that sage's configure said that clang 3.7 could support c+11 with -std=gnu++11. It didn't check with -std=c++11. However fplll doesn't want gnu++11

Shouldn't Sage ./configure check for C++11 support with-std=c++11? Currently it checks for C++11 support -std=gnu++11; e.g. on FreeBSD I see

checking whether clang++-devel supports C++11 features with -std=gnu++11... yes

comment:43 Changed 2 years ago by fbissey

If you are using #12426 to test this you need the configure tarball from #12426.

The c++11 test is a bit more subtle than that. If you don't say anything it will check for both in turn, but starts with gnu++11 and stops if that works. If you want to force the test for std=c++11 you have to pass noext to the macro. gnu++11 should be a super set of c++11 which is why it stops there. I am not sure why fplll insists that you shouldn't use gnu++11.

comment:44 follow-ups: Changed 2 years ago by zimmerma

concerning GF2X on FreeBSD (cf comment 16), does the following patch fix the issue?

--- a/configure.ac
+++ b/configure.ac
@@ -93,10 +93,10 @@ AC_ARG_ENABLE([fft-interface],
 AM_CONDITIONAL([ENABLE_FFT_INTERFACE],[test "x$enable_fft_interface" = xyes])

 AC_PROG_CC
-AC_PROG_CPP
 AC_PROG_CXX
 AC_COMPILE_WARNINGS
 AC_PROG_CC_C99
+AC_PROG_CPP

 GF2X_PROG_CC_FOR_BUILD
 GF2X_PROG_EXEEXT_FOR_BUILD

comment:45 in reply to: ↑ 44 Changed 2 years ago by fbissey

Replying to zimmerma:

concerning GF2X on FreeBSD (cf comment 16), does the following patch fix the issue?

--- a/configure.ac
+++ b/configure.ac
@@ -93,10 +93,10 @@ AC_ARG_ENABLE([fft-interface],
 AM_CONDITIONAL([ENABLE_FFT_INTERFACE],[test "x$enable_fft_interface" = xyes])

 AC_PROG_CC
-AC_PROG_CPP
 AC_PROG_CXX
 AC_COMPILE_WARNINGS
 AC_PROG_CC_C99
+AC_PROG_CPP

 GF2X_PROG_CC_FOR_BUILD
 GF2X_PROG_EXEEXT_FOR_BUILD

The patch is difficult to apply considering there is no AC_PROG_CPP in gf2x-1.1, I think you missed thome's first commit adding it and you are only quoting the one moving it. I don't think the move matters too much by the way.

comment:46 in reply to: ↑ 44 ; follow-up: Changed 2 years ago by dimpase

Replying to zimmerma:

concerning GF2X on FreeBSD (cf comment 16), does the following patch fix the issue?

--- a/configure.ac
+++ b/configure.ac
@@ -93,10 +93,10 @@ AC_ARG_ENABLE([fft-interface],
 AM_CONDITIONAL([ENABLE_FFT_INTERFACE],[test "x$enable_fft_interface" = xyes])

 AC_PROG_CC
-AC_PROG_CPP
 AC_PROG_CXX
 AC_COMPILE_WARNINGS
 AC_PROG_CC_C99
+AC_PROG_CPP

 GF2X_PROG_CC_FOR_BUILD
 GF2X_PROG_EXEEXT_FOR_BUILD

At present even with CPP and CXXCPP properly set, I still need GF2X_CONFIGURE="--disable-hardware-specific-code $GF2X_CONFIGURE" - otherwise I get non-sensical errors at tune stage, saying something along the lines of "this code can only be run on a 64-bit system" (see #22679, the last commit)

I was not able to figure out what goes wrong. Perhaps it's indeed this change that is needed to get the toolchain right, any idea?

comment:47 in reply to: ↑ 46 Changed 2 years ago by fbissey

Replying to dimpase:

I still need GF2X_CONFIGURE="--disable-hardware-specific-code $GF2X_CONFIGURE" - otherwise I get non-sensical errors at tune stage, saying something along the lines of "this code can only be run on a 64-bit system" (see #22679, the last commit)

I was not able to figure out what goes wrong. Perhaps it's indeed this change that is needed to get the toolchain right, any idea?

Setting CPP wouldn't help I think. Really we would need the failure (compilation line+ error message) and config.log. One thing you could try is to pull the current git master and see if that works.

comment:48 Changed 2 years ago by dimpase

this works nicely on FreeBSD (I did not try building gfortran though), and I do need a small patch on top of #12426, see #22679.

comment:49 Changed 2 years ago by git

  • Commit changed from a50d44f8da9bf9d1408e8cb945f07d227c884fb9 to f5657399ab53fcfe274dee50d0cf5d04bb83ae94

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

5434c70Merge branch 'develop' into compiler
f565739bump config tarball

comment:50 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:51 follow-up: Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why did you drop the checks like if [ -z "$CC" ] when moving the code from sage-env to sage-env-config? It should still be possible to override $CC at runtime.

comment:52 Changed 2 years ago by jdemeyer

Why AC_PROG_OBJC([clang]) instead of AC_PROG_OBJC()? Doesn't autoconf search for clang by default? If not, you should at least add cc to the search list (see the old sage-env).

comment:53 in reply to: ↑ 51 ; follow-up: Changed 2 years ago by fbissey

Replying to jdemeyer:

Why did you drop the checks like if [ -z "$CC" ] when moving the code from sage-env to sage-env-config? It should still be possible to override $CC at runtime.

Giving people the means to shoot themselves in the foot? I guess overriding CC at runtime is relatively safe, CXX and FC not so much.

As for AC_PROG_OBJC, I cannot find the default order anymore. It is only used by R on OS X, the idea was to cut the chase to what we really want there. But with such a short list all it does over just setting it is checking the presence. I guess I do overreach there.

comment:54 in reply to: ↑ 53 Changed 2 years ago by jdemeyer

Replying to fbissey:

I guess overriding CC at runtime is relatively safe, CXX and FC not so much.

I think that's not for you to decide. The autoconf philosophy is to have sensible defaults but always allow customizing everything. There will always be (obscure) cases where you would want to override some environment variable and it's not fun if you need to edit sage-env-config for that.

comment:55 Changed 2 years ago by fbissey

It all depends where you put the balance between convenience and safety, and all that kind of stuff. At least I am sure you know the price for convenience.

comment:56 Changed 2 years ago by jdemeyer

Well, it has always been possible to customize $CC on the fly and I don't think that this has led to many problems. So I see no reason to start disallowing it in this ticket.

comment:57 follow-up: Changed 2 years ago by mkoeppe

To keep consistent with autotools packages, setting an environment variable such as CC should NOT override what has been configured using "configure". (Rather, to override, a user would use make CC=.....)

comment:58 in reply to: ↑ 57 ; follow-up: Changed 2 years ago by jdemeyer

Replying to mkoeppe:

To keep consistent with autotools packages, setting an environment variable such as CC should NOT override what has been configured using "configure". (Rather, to override, a user would use make CC=.....)

I see your point. However, not all Sage package use make, so this cannot work for Sage.

Alternatively, we need to ensure that make CC=... really does work in all cases.

comment:59 in reply to: ↑ 58 ; follow-up: Changed 2 years ago by mkoeppe

Replying to jdemeyer:

Replying to mkoeppe:

To keep consistent with autotools packages, setting an environment variable such as CC should NOT override what has been configured using "configure". (Rather, to override, a user would use make CC=.....)

I see your point. However, not all Sage package use make, so this cannot work for Sage.

This unrelated to what build systems Sage packages use. They just use the environment prepared by Sage's top-level build scripts and Makefiles.

comment:60 in reply to: ↑ 59 Changed 2 years ago by jdemeyer

Replying to mkoeppe:

This unrelated to what build systems Sage packages use.

No, it's not. GNU make passes variables to sub-makes, so a top-level make CC=foo will pass CC=foo when running make inside a specific package.

comment:61 follow-up: Changed 2 years ago by fbissey

"because we always did..." is a terrible reason. But there is a point to both parties. If you want to be able to recompile a package with CC=foo ./sage -f bar then it is very convenient. But then would make CC=foo bar work instead? Of course, it will not work when we set a compiler in spkg-install (and we do that quite a bit on OS X).

What I don't like about overriding the compilers at runtime (to use sage proper rather its packaging system) is that you can get in the situation of #21701 where for a while I had no clue what was happening. When you invoke sagelib, and you do some compilation with cython from sagelib, having the right compiler is rather critical. I don't like the idea of that compiler be able to be changed. It is an invitation to broken behavior.

comment:62 in reply to: ↑ 61 Changed 2 years ago by jdemeyer

Replying to fbissey:

"because we always did..." is a terrible reason.

Let me rephrase it as "because it always was possible". You are proposing to remove a feature: being able to change compilers at any time. If you remove a feature, there should be a good reason for that.

comment:63 follow-up: Changed 2 years ago by dimpase

I presume we can add CC= etc. as options to sage -i and sage -f. This would allow per package overriding. Does this go far enough to make everyone happy?

comment:64 in reply to: ↑ 63 Changed 2 years ago by fbissey

Replying to dimpase:

I presume we can add CC= etc. as options to sage -i and sage -f. This would allow per package overriding. Does this go far enough to make everyone happy?

Let's not bother with that. We could do a survey on whether or not people are using this "feature", that is purely accidental, on sage-devel. We have enough people that we would get a few yeses, even if it is just for the principle of the thing.

Driving changes through people's throat is usually not a good idea. The best ways are

  • to offer something in exchange of greater value one way or another. The current offer is obviously not appropriate.
  • let people figure themselves that things needs to be changed, usually for their own good.

In that context I am happy to provide the "shoot in the foot gun", if it ever become painful enough, someone else will drive the change. I don't expect it will happen anytime soon because the reality is that it is a very low use "feature" for people that don't mind taking it in the foot on a regular basis in the first place.

Last edited 2 years ago by fbissey (previous) (diff)

comment:65 follow-up: Changed 2 years ago by fbissey

@ Jeroen question on implementation of said feature:

Currently (not this branch) an installed gcc installed by sage will be selected even if you provide an alternative compiler. Do you want to continue with that behavior or would you prefer things to be consistent and have a provided compiler always be the one used?

comment:66 in reply to: ↑ 65 ; follow-up: Changed 2 years ago by jdemeyer

Replying to fbissey:

Currently (not this branch) an installed gcc installed by sage will be selected even if you provide an alternative compiler. Do you want to continue with that behavior or would you prefer things to be consistent and have a provided compiler always be the one used?

That's a good question. I think it's not a very relevant question, since the people who like to play with different compilers will usually not use the Sage-compiled GCC.

Still, for consistency, it makes more sense if an environment variable would override the compiler in all cases. So, the compiler should be determined by the following in decreasing order of priority:

  1. Environment variables
  1. Sage-installed GCC
  1. ./configure setting

comment:67 in reply to: ↑ 66 ; follow-up: Changed 2 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

Currently (not this branch) an installed gcc installed by sage will be selected even if you provide an alternative compiler. Do you want to continue with that behavior or would you prefer things to be consistent and have a provided compiler always be the one used?

That's a good question. I think it's not a very relevant question, since the people who like to play with different compilers will usually not use the Sage-compiled GCC.

Still, for consistency, it makes more sense if an environment variable would override the compiler in all cases. So, the compiler should be determined by the following in decreasing order of priority:

  1. Environment variables
  1. Sage-installed GCC
  1. ./configure setting

Exactly my opinion of what it should be for consistency, we are in agreement - but since that is changing the behavior and all, better check ;)

Now to the best way of achieving all that....

comment:68 in reply to: ↑ 67 Changed 2 years ago by dimpase

Replying to fbissey:

Replying to jdemeyer:

Replying to fbissey:

Currently (not this branch) an installed gcc installed by sage will be selected even if you provide an alternative compiler. Do you want to continue with that behavior or would you prefer things to be consistent and have a provided compiler always be the one used?

That's a good question. I think it's not a very relevant question, since the people who like to play with different compilers will usually not use the Sage-compiled GCC.

For people who use OSX it's not a play, it's matter of having working Sage, or not...

comment:69 Changed 2 years ago by git

  • Commit changed from f5657399ab53fcfe274dee50d0cf5d04bb83ae94 to f4d6748d5ae0826d0f30af78e68e627121d616d4

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

f4d6748Change the setting of compilers in the order: environment, sage installed compilers, configured compilers.

comment:70 Changed 2 years ago by git

  • Commit changed from f4d6748d5ae0826d0f30af78e68e627121d616d4 to 4c41390b5340b250aed08628f1ee24b598a72319

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

eb6f027fix objective compiler detection. Don't try to negate using the macro. Bad syntax that no one seem to have noticed.
4c41390Regenerate configure tarball

comment:71 Changed 2 years ago by fbissey

  • Status changed from needs_work to needs_review

New tarball will be up in the morning in my time zone. Otherwise back to review.

comment:72 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think that this logic

# On OS X 10.7 (Lion) or later when building GCC, use clang as
# default C compiler as some older versions of XCode 4 ship broken
# versions of GCC which fail to bootstrap GCC-4.6.3.
# See http://trac.sagemath.org/sage_trac/ticket/12820
if [ "$SAGE_BUILD_TOOLCHAIN" = yes ] && [ "$UNAME" = "Darwin" ] && [ "$MACOSX_VERSION" -ge 11 ; then
    CC=`which clang 2>/dev/null`
    CXX=`which clang++ 2>/dev/null`

should also be moved to configure.

Unfortunately, autoconf 2.69 (which is still the latest release despite being 5 years old) does not support clang.

So I suggest something along the lines of

if test -z "$CC"; then
    if test -n "$MACOSX_VERSION" && test "$MACOSX_VERSION" -ge 11 ; then
        AC_CHECK_TOOL(CC, clang)
    fi
fi
AC_PROG_CC()

comment:73 follow-ups: Changed 2 years ago by fbissey

I think that's just scope creep. But I am willing to make some concessions. In that particular case I think we could just remove that particular piece of code. It seems to go back to when apple was still shipping a "real" apple patched gcc rather than having it being a clang wrapper. I don't think we should continue to support such snowflakes. In fact I don't think we realistically support anything older than xcode 6 but most likely 7.

comment:74 in reply to: ↑ 73 ; follow-up: Changed 2 years ago by jdemeyer

Replying to fbissey:

In fact I don't think we realistically support anything older than xcode 6 but most likely 7.

So you are saying that Sage 8.0 wouldn't build anyway on OS X 10.7 for example?

comment:75 in reply to: ↑ 74 Changed 2 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

In fact I don't think we realistically support anything older than xcode 6 but most likely 7.

So you are saying that Sage 8.0 wouldn't build anyway on OS X 10.7 for example?

Depends which version of xcode you can put on there. But if you cannot install xcode 7, I'd say with difficulties. I could be wrong but I expect it wouldn't build out of the box. May be with a bit of hand holding. Do we know what versions we have on the build bots? A lot of the third party software binaries these days don't go further back than 10.9 in backward compatibility support. I think there are serious limitations in going further back.

comment:76 in reply to: ↑ 73 Changed 2 years ago by jdemeyer

Replying to fbissey:

It seems to go back to when apple was still shipping a "real" apple patched gcc rather than having it being a clang wrapper.

So, with XCode, the gcc and clang programs are exactly the same? That would indeed mean that there is no reason for the code in 72. Should we just remove it?

comment:77 Changed 2 years ago by fbissey

xcode 8:

Mirage:~ fbissey$ /usr/bin/gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 8.1.0 (clang-802.0.42)
Target: x86_64-apple-darwin16.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

xcode 7 and 6 were similar. I don't remember much before that. But yes you are now seeing it my way: removal seems a better idea.

comment:78 follow-ups: Changed 2 years ago by dimpase

IMHO we should drop OSX 10.6. On the other hand, Apple continues to sell OSX 10.7---it's probably the oldest system they currently support.

comment:79 in reply to: ↑ 78 ; follow-up: Changed 2 years ago by fbissey

Replying to dimpase:

IMHO we should drop OSX 10.6. On the other hand, Apple continues to sell OSX 10.7---it's probably the oldest system they currently support.

Really??? Is it just an upgrade path or is it for "vintage" hardware?

comment:80 in reply to: ↑ 79 Changed 2 years ago by dimpase

Replying to fbissey:

Replying to dimpase:

IMHO we should drop OSX 10.6. On the other hand, Apple continues to sell OSX 10.7---it's probably the oldest system they currently support.

Really??? Is it just an upgrade path or is it for "vintage" hardware?

the latter, I think---one can buy an official DVD: https://www.apple.com/uk/shop/product/D6106ZM/A/os-x-lion

comment:81 Changed 2 years ago by fbissey

Could be an upgrade path as well. You need at least 10.7 to have the app store through which you can install OS X upgrades. I think you cannot do that from 10.6, you need a media or at the very least a .dmg. I had an old imac like that for which I would have needed the media (it's dead now, the gpu died).

Or I could have my versions mixed up.

comment:82 in reply to: ↑ 78 Changed 2 years ago by kcrisman

IMHO we should drop OSX 10.6. On the other hand, Apple continues to sell OSX 10.7---it's probably the oldest system they currently support.

Given that people with hardware for 10.6 can probably upgrade to 10.7, this might seem reasonable.

comment:83 Changed 2 years ago by dimpase

I just checked that one can still get official OSX 10.6 just as well... (but no 10.5). 10.6 has a feature missing in 10.7, it can run PPC applications. Other than that, I don't see why people would want to stick with it.

comment:84 Changed 2 years ago by git

  • Commit changed from 4c41390b5340b250aed08628f1ee24b598a72319 to 38273c6c529d844cceea62e57bbbb993e2000515

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

38273c6Remove bits specific to outdated xcode toolchain

comment:85 Changed 2 years ago by git

  • Commit changed from 38273c6c529d844cceea62e57bbbb993e2000515 to ab61da6c3615e09e1b3ad8aedbe64a8e019b1060

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

ab61da6update configure tarball

comment:86 Changed 2 years ago by fbissey

  • Status changed from needs_work to needs_review

OK so removed that old cruft for now. May be we'll discover one of the bots still has a very old version of OS X.

comment:87 Changed 2 years ago by git

  • Commit changed from ab61da6c3615e09e1b3ad8aedbe64a8e019b1060 to 58208125c2616a6b2d027b8573db65c3844c9c82

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

5820812Merge branch 'develop' into compiler and bump configure tarball

comment:88 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:89 follow-ups: Changed 2 years ago by mkoeppe

Two comments:

1) Strong objection to mixing an autoconf feature with sage-specific behavior (environment CC overrides configured CC), which makes it different from any other autoconf-based package. It is an important feature that the configured variables stick in the build and that this works even in the presence of some polluted environment.

If it is really necessary to make overriding possible using environment settings, you can invent new variables such as SAGE_OVERRIDE_CC etc.

2) I would suggest to only set variables in sage-env-config and keep all logic (if-then) in sage-env. For example:

SAGE_CONFIGURED_CC="@CC@"

comment:90 in reply to: ↑ 89 ; follow-ups: Changed 2 years ago by fbissey

Replying to mkoeppe:

Two comments:

1) Strong objection to mixing an autoconf feature with sage-specific behavior (environment CC overrides configured CC), which makes it different from any other autoconf-based package. It is an important feature that the configured variables stick in the build and that this works even in the presence of some polluted environment.

If it is really necessary to make overriding possible using environment settings, you can invent new variables such as SAGE_OVERRIDE_CC etc.

No. I just tried an autotooled package (gf2x), you can configure with one compiler and over-ride it at build time with CC and co. However I discourage anyone to over-ride it at runtime if you need to compile cython files from inside sage. That's my usual dichotomy problem between sage the packaging stack and sage, the python runtime on which you do your math.

2) I would suggest to only set variables in sage-env-config and keep all logic (if-then) in sage-env. For example:

SAGE_CONFIGURED_CC="@CC@"

As the requester on this ticket I wish you had expressed yourself earlier on that particular design. It is a fine idea to confine sage-env-config to be a variable store without any logic in it. I just wish I had that requirement earlier. I had the first draft of the current design up 2 months ago and really opened to criticism 7 weeks ago.

comment:91 in reply to: ↑ 89 Changed 2 years ago by jdemeyer

Replying to mkoeppe:

Strong objection to mixing an autoconf feature with sage-specific behavior (environment CC overrides configured CC), which makes it different from any other autoconf-based package.

I don't consider Sage to be a "package" (it's a distribution of packages), so I don't see it as an issue that it behaves differently from other packages. It's comparing an apple with oranges. It has always been the case in Sage that a CC environment variable overrides the default CC and nobody has ever considered this a problem.

If it is really necessary to make overriding possible using environment settings, you can invent new variables such as SAGE_OVERRIDE_CC etc.

In that case, we should simply support make CC=... from the top-level. But that will require some additional work.

comment:92 in reply to: ↑ 90 Changed 2 years ago by mkoeppe

Replying to fbissey:

Replying to mkoeppe:

1) Strong objection to mixing an autoconf feature with sage-specific behavior (environment CC overrides configured CC), which makes it different from any other autoconf-based package. It is an important feature that the configured variables stick in the build and that this works even in the presence of some polluted environment.

No. I just tried an autotooled package (gf2x), you can configure with one compiler and over-ride it at build time with CC and co.

You don't say what exactly you tried. My point is that you can override by using makefile variables like this:

   make CC=xyzzy

but you cannot override using environment variables:

  CC=xyzzy make

Any implementation for Sage should copy this behavior.

comment:93 in reply to: ↑ 90 Changed 2 years ago by mkoeppe

Replying to fbissey:

As the requester on this ticket I wish you had expressed yourself earlier on that particular design. It is a fine idea to confine sage-env-config to be a variable store without any logic in it. I just wish I had that requirement earlier. I had the first draft of the current design up 2 months ago and really opened to criticism 7 weeks ago.

Right, and I wish I had more time for Sage development lately. In any case, this one is just a suggestion. If #21707 is still something that we want to do, we can change the structure of these scripts at that time.

comment:94 Changed 2 years ago by fbissey

@ mkoeppe I guess we are all there in regards to time [in fact I started writting this answer more than 24 hours ago now...]. I was just venting.

I have no vested interest in #21707. If anything I think this is misguided at this point. I think some other work like the decoupling of "sagelib" from the packaging system should be done first and would be beneficial to some of what you seek in #21707 in the long run (personal opinion).

I will turn sage-env-config in a variable store in this ticket. It is not going to be included but could be material early in the 8.1 cycle.

On that note I will leave the current behavior for over-riding CC. I don't want to be part of that debate more than is necessary. Feel free to open a ticket to get the behavior you want, but I think you need to get traction from sage-devel first, since there is opposition.

Last edited 2 years ago by fbissey (previous) (diff)

comment:95 Changed 2 years ago by git

  • Commit changed from 58208125c2616a6b2d027b8573db65c3844c9c82 to 79d0963576906f01449ba9b4d3ad4e307b8523b6

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

79d0963Remove compiler selecting logic from sage-env-config and put it back in sage-env.

comment:96 Changed 2 years ago by fbissey

  • Milestone changed from sage-8.0 to sage-8.1

comment:97 Changed 2 years ago by git

  • Commit changed from 79d0963576906f01449ba9b4d3ad4e307b8523b6 to 1015a1fc8fa01cfcbb8faaecd8348682ed72491c

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

1015a1fMerge branch 'master' into compiler and configure tarball bump

comment:98 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:99 Changed 2 years ago by mkoeppe

From looking at the latest patch, $ seems to be missing in front of CONFIGURED_CXX etc. in several places in src/bin/sage-env. (Haven't tested.)

comment:100 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:101 Changed 2 years ago by git

  • Commit changed from 1015a1fc8fa01cfcbb8faaecd8348682ed72491c to c4fce28f6568eadb16c161ed9a5d02ba17a8229e

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

c4fce28fix missing "$" in sage-env

comment:102 Changed 2 years ago by fbissey

  • Status changed from needs_work to needs_review

Indeed, fixed.

comment:103 Changed 2 years ago by git

  • Commit changed from c4fce28f6568eadb16c161ed9a5d02ba17a8229e to bbee7b8529268fe8f8721d098a9f677030d456c2

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

bbee7b8Merge branch 'develop' into compiler and bump configure tarball

comment:104 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:105 follow-up: Changed 2 years ago by jhpalmieri

It would be nice to get this merged soon, so it is early in the 8.1 release cycle. What are the obstacles to this?

comment:106 in reply to: ↑ 105 ; follow-up: Changed 2 years ago by fbissey

Replying to jhpalmieri:

It would be nice to get this merged soon, so it is early in the 8.1 release cycle. What are the obstacles to this?

Possibly bikering over what behavior it should enforce. Currently CC=.. ./sage -f ... or CC=... make .... will over-ride the configure time compiler. Some people think it shouldn't, only make ... CC=... should.

comment:107 Changed 2 years ago by fbissey

But be my guest in reviewing this.

comment:108 in reply to: ↑ 106 Changed 2 years ago by dimpase

Replying to fbissey:

Possibly bikering over what behavior it should enforce. Currently CC=.. ./sage -f ... or CC=... make .... will over-ride the configure time compiler. Some people think it shouldn't, only make ... CC=... should.

Would a simple renaming of the the configure parameters from CC= to --c-compiler=, etc, alleviate at least some of concerns (unless there is some hidden magic in naming the parameter CC---I hope not)? Personally, every time I do ./configure CC=... I get deja vu---maybe it's just me though.

comment:109 follow-up: Changed 2 years ago by fbissey

I am not sure what that would achieve. AC_PROG_CC makes sure that CC, which is used in standard makefile rules is set to something sensible - by going through a list or checking that the current setting of CC is sane. You may have deja vu because you haven't been on a system with a variety of compilers available out of the box and where you often wants something different from the default.

If you are going to use something non-standard like --c-compiler=I am almost tempted to tell you not to use it to set CC either. But then you lose some of the work AC_PROG_CC does for you [test that the compiler work, if it is gcc (real or fake)].

comment:110 in reply to: ↑ 109 ; follow-up: Changed 2 years ago by dimpase

Replying to fbissey:

I am not sure what that would achieve. AC_PROG_CC makes sure that CC, which is used in standard makefile rules is set to something sensible - by going through a list or checking that the current setting of CC is sane. You may have deja vu because you haven't been on a system with a variety of compilers available out of the box and where you often wants something different from the default.

I certainly have been there and wanted that, my point is that having CC=... as a parameter to a script (rather than an environment variable) is akin to having two girlfriends with the same given name---sometimes convenient, but sometimes making you dazed and confused... ;-)

If you are going to use something non-standard like --c-compiler=I am almost tempted to tell you not to use it to set CC either. But then you lose some of the work AC_PROG_CC does for you [test that the compiler work, if it is gcc (real or fake)].

I don't see what you mean here. E.g. in configure.ac one can probably do something like

CCORIG=gcc
AC_SUBST(CC,$CCORIG)
AC_ARG_WITH(cc,AS_HELP_STRING(--with-cc=...,specify the full path to the C compiler ($CC)),CC=$withval)
AC_PROG_CC()
...
AC_ARG_WITH(cxx,...

to create ./configure arguments such as --with-cc=, and set CC, etc to the right values.

Last edited 2 years ago by dimpase (previous) (diff)

comment:111 in reply to: ↑ 110 ; follow-ups: Changed 2 years ago by fbissey

Replying to dimpase:

Replying to fbissey:

I am not sure what that would achieve. AC_PROG_CC makes sure that CC, which is used in standard makefile rules is set to something sensible - by going through a list or checking that the current setting of CC is sane. You may have deja vu because you haven't been on a system with a variety of compilers available out of the box and where you often wants something different from the default.

I certainly have been there and wanted that, my point is that having CC=... as a parameter to a script (rather than an environment variable) is akin to having two girlfriends with the same given name---sometimes convenient, but sometimes making you dazed and confused... ;-)

You choose to see it as a parameter because it is put at the end of the line. But really it is just setting that environment variable while running that command. You could put it in front with the same result, the main point of putting it in a "parameter" position is that it is captured in config.log. But whether it is passed as, a parameter, a single shot environment variable or exported from the environment, it all give the same result.

There is no duality, it is always an environment variable. And setting it on that command line, either in front, or in parameter position, is all about not polluting your main environment with the setting in question.

But my feeling is that this is beside the point of this ticket.

comment:112 in reply to: ↑ 111 Changed 2 years ago by dimpase

Replying to fbissey:

It did indeed escape me that CC= is not a script parameter, sorry. No objections then.

comment:113 in reply to: ↑ 111 ; follow-up: Changed 2 years ago by mkoeppe

Replying to fbissey:

You could put it in front with the same result, the main point of putting it in a "parameter" position is that it is captured in config.log.

... and config.status, of course, so that the setting persists across config.status --recheck etc.

For positive review, I would like to see:

  • the difference in behavior to standard autotools (comment 92) documented somewhere
  • a follow-up ticket for making the behavior equal to standard autotools should be created
  • the ticket summary should be updated (support "for all SAGE_... variables" is not implemented in the branch) - follow-up ticket?

comment:114 in reply to: ↑ 113 Changed 2 years ago by fbissey

Replying to mkoeppe:

Replying to fbissey:

You could put it in front with the same result, the main point of putting it in a "parameter" position is that it is captured in config.log.

... and config.status, of course, so that the setting persists across config.status --recheck etc.

For positive review, I would like to see:

  • the difference in behavior to standard autotools (comment 92) documented somewhere

Yes, certainly some documentation is warranted. Note that since "sage" is not an autotool "product", I have in fact no problem with CC=... ./sage changing the compiler. It is consistent with how I am changing the environment for one off runs on the command line for many applications. Example OMP_NUM_THREADS=4 ./my_openmp_app. May be I am wrong to put it in prefix position and putting it postfix would work, but I wasn't taught any better.

  • a follow-up ticket for making the behavior equal to standard autotools should be created

I can create a follow up ticket and copy you onto it but I probably won't do any work on it. I will leave that to someone else.

  • the ticket summary should be updated (support "for all SAGE_... variables" is not implemented in the branch) - follow-up ticket?

I completely forgot about those I must say. This is more complicated as we will, at the very least, need to generate an exhaustive list of those variables, preferably automatically. Definitely another ticket.

comment:115 Changed 2 years ago by fbissey

Actually, while I may add a bit of documentation on configuration, the current documentation is quite explicit about how CC and others is used

- :envvar:`CC` - while some programs allow you to use this to specify your C
  compiler, **not every Sage package recognizes this**.
  If GCC is installed within Sage, :envvar:`CC` is ignored and Sage's ``gcc``
  is used instead.

When you have to use a variable in postfix position it is not treated as an environment variable. I may add anote that this differs from the behavior you expect from autotools.

Last edited 2 years ago by fbissey (previous) (diff)

comment:116 Changed 2 years ago by fbissey

  • Description modified (diff)

Requested follow up at #23569 and #23570. I am working on the documentation (doc/en/installation/source.rst the document only exists in English so far) suggestions welcome.

comment:117 Changed 2 years ago by git

  • Commit changed from bbee7b8529268fe8f8721d098a9f677030d456c2 to e485a75b7c89bb283b82db7a30583dbb3227270e

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

e485a75Merge branch 'develop' into compiler and bump configure tarball

comment:118 Changed 2 years ago by fbissey

  • Description modified (diff)

comment:119 Changed 2 years ago by mkoeppe

Progress on the documentation?

comment:120 Changed 2 years ago by fbissey

I have a draft somewhere. I am just on too spread out.

comment:121 Changed 2 years ago by git

  • Commit changed from e485a75b7c89bb283b82db7a30583dbb3227270e to a8c8904577f481f82c63e9354afdf7aecc743f87

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

3772ae5Merge branch 'develop' into compiler
a8c8904Add some documentation and update configure tarball

comment:122 Changed 2 years ago by fbissey

  • Description modified (diff)

Ok rebased and added some documentation. I don't think it is particularly great feedback appreciated.

Changed 2 years ago by fbissey

comment:123 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:124 Changed 2 years ago by jhpalmieri

A few minor suggestions for the documentation: in the first sentence, it is ambiguous about which variables need to be specified just for OS X. Maybe something like this instead:

Some environment variables deserve a special mention: CC, CXX, and FC; and on OS X, OBJC and OBJCXX.

Also, change "over-riden" to "over-ridden" and change "sage" to "Sage" (twice).

Otherwise, I don't have anything to add to it; I think it's fine.

comment:125 Changed 2 years ago by git

  • Commit changed from a8c8904577f481f82c63e9354afdf7aecc743f87 to 99eb31f851a4fcdf1e17bd1671b5218ae9acc0be

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

99eb31fMinor edits from John Palmieri's feedback

comment:126 Changed 2 years ago by fbissey

OK, revision included. It is ready for someone to push the positive review button.

comment:127 Changed 2 years ago by dimpase

  • Reviewers set to Matthias Koeppe, Jeroen Demeyer, Vincent Delecroix, John Palmieri, Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:128 follow-ups: Changed 2 years ago by jhpalmieri

Great! Now on to #12426?

comment:129 in reply to: ↑ 128 Changed 2 years ago by dimpase

Replying to jhpalmieri:

Great! Now on to #12426?

yes, sure. I'll test it for building gfortran on FreeBSD.

comment:130 in reply to: ↑ 128 ; follow-up: Changed 2 years ago by fbissey

Replying to jhpalmieri:

Great! Now on to #12426?

Thanks everyone!

For #12426. It would be nice to have it in 8.1 but some people may feel the shift warrants it to go first thing in 8.2.beta0. We could take the debate to sage-devel. Things needed in my opinion for #12426:

  • new release of eclib - that won't solve all the problems but working with clang lead to improvement of the code.
  • update and fix ccache
  • a good discussion about the giac test

comment:131 in reply to: ↑ 130 Changed 2 years ago by dimpase

Replying to fbissey:

Replying to jhpalmieri:

Great! Now on to #12426?

Thanks everyone!

For #12426. It would be nice to have it in 8.1 but some people may feel the shift warrants it to go first thing in 8.2.beta0. We could take the debate to sage-devel. Things needed in my opinion for #12426:

  • new release of eclib - that won't solve all the problems but working with clang lead to improvement of the code.

eclib already took advantage of clang, it helped finding a couple of ugly memory leaks... I think it can be done after this ticket - working without a properly supported toolchain is tricky.

  • update and fix ccache

Why is this so urgent, and again, such things are harder to fix without a properly supported toolchain.

  • a good discussion about the giac test

What is this about? giac certainly would benefit from more uniform following c++11 or/and gnu++11...

  • How about clang on Linux? Are we aiming at it now here? Or, again, this could wait for another ticket? (I'd rather do the latter; it's after all not that urgent).

comment:132 Changed 2 years ago by fbissey

ccache is not urgent. While it is not a blocker we should aim at supporting a maximum of optional packages out of the box. So fixing the known problems make sense.

More urgent is doing all that is possible for standard packages that have a testsuite to pass it with clang. This won't happen at this stage for eclib and giac. eclib because it is hard, giac because there is currently a test that orders results differently depending on the compiler used :( - we would need a patching option in spkg-check to fix this kind of case and it would need a little extra infrastructure.

clang on linux is just a nice side effect :) and the last ticket for it to work once #12426 is merged is in.

Note that #12426 makes it possible to use icc and it works, at say ~95%, out of the box for standard sage. sqlite needs a fix that belongs upstream and pari optimisation needs to be tuned down on top of my head.

comment:133 Changed 2 years ago by dimpase

IMHO, this all should be dealt with on follow-up tickets, and #12426 ought to get in without them.

comment:134 Changed 2 years ago by vbraun

  • Branch changed from u/fbissey/compilers to 99eb31f851a4fcdf1e17bd1671b5218ae9acc0be
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:135 Changed 2 years ago by jdemeyer

  • Commit 99eb31f851a4fcdf1e17bd1671b5218ae9acc0be deleted

Follow-up: #23789

Note: See TracTickets for help on using tickets.