Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mkoeppe opened this issue Mar 19, 2017 · 157 comments
Closed

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

mkoeppe opened this issue Mar 19, 2017 · 157 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 19, 2017

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://github.com/sagemath/sage/files/ticket22646/configure-235.tar.gz

CC: @jdemeyer @kiwifb

Component: build

Author: François Bissey

Branch: 99eb31f

Reviewer: Matthias Koeppe, Jeroen Demeyer, Vincent Delecroix, John Palmieri, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/22646

@mkoeppe mkoeppe added this to the sage-8.0 milestone Mar 19, 2017
@kiwifb
Copy link
Member

kiwifb commented Apr 18, 2017

comment:2

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2017

comment:3

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.

@kiwifb
Copy link
Member

kiwifb commented Apr 18, 2017

comment:4

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2017

comment:5

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

@kiwifb
Copy link
Member

kiwifb commented May 8, 2017

Author: François Bissey

@kiwifb
Copy link
Member

kiwifb commented May 8, 2017

Branch: u/fbissey/compilers

@kiwifb
Copy link
Member

kiwifb commented May 8, 2017

comment:6

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

@kiwifb
Copy link
Member

kiwifb commented May 8, 2017

Commit: 483106a

@kiwifb
Copy link
Member

kiwifb commented May 19, 2017

comment:7

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.

@kiwifb

This comment has been minimized.

@kiwifb

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2017

Changed commit from 483106a to 945e824

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2017

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

@kiwifb

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2017

Changed commit from 945e824 to 32a0b00

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2017

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

32a0b00one AC_SUBST per variable only :(

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 20, 2017

Changed commit from 32a0b00 to 11dc29a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 20, 2017

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 20, 2017

Changed commit from 11dc29a to ef9f1b3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 20, 2017

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

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

@dimpase
Copy link
Member

dimpase commented Jun 20, 2017

comment:15

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

@kiwifb
Copy link
Member

kiwifb commented Jun 20, 2017

comment:16

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 20, 2017

comment:17

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 20, 2017

comment:18

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

@videlec
Copy link
Contributor

videlec commented Jun 21, 2017

comment:19

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2017

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

e485a75Merge branch 'develop' into compiler and bump configure tarball

@kiwifb

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2017

comment:119

Progress on the documentation?

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2017

comment:120

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2017

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2017

Changed commit from e485a75 to a8c8904

@kiwifb
Copy link
Member

kiwifb commented Aug 29, 2017

comment:122

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

@kiwifb

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Aug 30, 2017

comment:123

Attachment: configure-235.tar.gz

@dimpase

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:124

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 30, 2017

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

99eb31fMinor edits from John Palmieri's feedback

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 30, 2017

Changed commit from a8c8904 to 99eb31f

@kiwifb
Copy link
Member

kiwifb commented Aug 30, 2017

comment:126

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

@dimpase
Copy link
Member

dimpase commented Aug 30, 2017

Reviewer: Matthias Koeppe, Jeroen Demeyer, Vincent Delecroix, John Palmieri, Dima Pasechnik

@jhpalmieri
Copy link
Member

comment:128

Great! Now on to #12426?

@dimpase
Copy link
Member

dimpase commented Aug 30, 2017

comment:129

Replying to @jhpalmieri:

Great! Now on to #12426?

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

@kiwifb
Copy link
Member

kiwifb commented Aug 30, 2017

comment:130

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

@dimpase
Copy link
Member

dimpase commented Aug 30, 2017

comment:131

Replying to @kiwifb:

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

@kiwifb
Copy link
Member

kiwifb commented Aug 30, 2017

comment:132

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.

@dimpase
Copy link
Member

dimpase commented Aug 31, 2017

comment:133

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

@vbraun
Copy link
Member

vbraun commented Sep 4, 2017

Changed branch from u/fbissey/compilers to 99eb31f

@jdemeyer
Copy link

jdemeyer commented Sep 6, 2017

comment:135

Follow-up: #23789

@jdemeyer
Copy link

jdemeyer commented Sep 6, 2017

Changed commit from 99eb31f to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants