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

build/pkgs/gfortran/spkg-configure.m4 works incorrectly if CC and CXX are already there #26715

Closed
dimpase opened this issue Nov 18, 2018 · 43 comments

Comments

@dimpase
Copy link
Member

dimpase commented Nov 18, 2018

As a result of refactoring configure.ac in #24919,
absence of (g)fortran now forces installation of full gcc, instead of just gfortran part of it.

This is particularly crucial on OSX, where we don't normally build full gcc any more, but only build gfortran.

To reproduce, move gfortran out of your PATH, and re-run ./configure. You'll see that now gcc and gfortran are scheduled for installation...

CC: @embray @jhpalmieri @vbraun @kiwifb

Component: build: configure

Author: François Bissey

Branch: 3185e91

Reviewer: Erik Bray

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

@dimpase dimpase added this to the sage-8.5 milestone Nov 18, 2018
@dimpase

This comment has been minimized.

@dimpase
Copy link
Member Author

dimpase commented Nov 18, 2018

comment:2

It appears that all what's needed is

--- a/build/pkgs/gfortran/spkg-configure.m4
+++ b/build/pkgs/gfortran/spkg-configure.m4
@@ -14,7 +14,6 @@ SAGE_SPKG_CONFIGURE([gfortran], [
         # compiler.
         if test -z "$FC"; then
             sage_spkg_install_gfortran=yes
-            SAGE_MUST_INSTALL_GCC([a Fortran compiler is missing])
         fi
 
         # see http://www.gnu.org/software/hello/manual/autoconf/Fortran-Compiler.html

with this change, and no gfortran in PATH, everything looks as it should after running ./configure.

Could someone with access to OSX please try this? (Don't forget to run ./bootstrap before
./configure, i.e. you do need autotools for such a check...).

@kiwifb
Copy link
Member

kiwifb commented Nov 18, 2018

comment:3

This is all a bit tricky. What control logic is there when you don't have gfortran (so that the above check will say sage_spkg_install_gfortran=yes) but gcc must be installed as well?

@dimpase
Copy link
Member Author

dimpase commented Nov 18, 2018

comment:4

Replying to @kiwifb:

This is all a bit tricky. What control logic is there when you don't have gfortran (so that the above check will say sage_spkg_install_gfortran=yes) but gcc must be installed as well?

There is build/pkgs/gcc/spkg-configure.m4 which should take care of this.

@kiwifb
Copy link
Member

kiwifb commented Nov 18, 2018

comment:5

Replying to @dimpase:

Replying to @kiwifb:

This is all a bit tricky. What control logic is there when you don't have gfortran (so that the above check will say sage_spkg_install_gfortran=yes) but gcc must be installed as well?

There is build/pkgs/gcc/spkg-configure.m4 which should take care of this.

It doesn't but the the section of code you point too in the diff can only be reached if sage_spkg_install_gcc is no. Otherwise the variable I was worried about is set to no.

The only real issue to be careful here is that the gcc bit needs to be run before the gfortran bit. If we go by lexical order everything is fine in this particular case. But that may be something to remember if there is ever another package with an "optional split" or a where checking a dependency first is important.

@dimpase
Copy link
Member Author

dimpase commented Nov 18, 2018

comment:6

It seems to be the case for "package Bar provides functionality Foo" kind of stuff that one can dive in :-P

Should this Sage component be called YAPS (Yet Another Packaging System)? And by virtue of name it must be implemented in yaml...

@kiwifb
Copy link
Member

kiwifb commented Nov 18, 2018

comment:7

Amusing. But more seriously your diff certainly point to an issue that needs to be fixed and is logically what we are after. However the line probably should be replaced with a notice. It was probably the intent in the first place. When do you intend to submit a branch for review?

@embray
Copy link
Contributor

embray commented Nov 19, 2018

comment:9

Replying to @kiwifb:

Amusing. But more seriously your diff certainly point to an issue that needs to be fixed and is logically what we are after. However the line probably should be replaced with a notice. It was probably the intent in the first place. When do you intend to submit a branch for review?

I think having some kind of notice was the intent, though written exactly as I did was probably a mistake on my part. Looking at the diff from #24919, this is the original code I removed from configure.ac:

-# Check that the Fortran compiler accepts free-format source code
-# (as opposed to the older fixed-format style from Fortran 77).
-# This helps verify the compiler works too, so if some idiot
-# sets FC to /usr/bin/ls, we will at least know it's
-# not a working Fortran compiler.
-AC_LANG(Fortran)
-if test -z "$FC"; then
-    need_to_install_gfortran=yes
-else

and this is the equivalent code that was moved to build/pkgs/gfortran/spkg-configure.m4:

+        # Check that the Fortran compiler accepts free-format source code (as
+        # opposed to the older fixed-format style from Fortran 77).
+        # This helps verify the compiler works too, so if some idiot sets FC to
+        # /usr/bin/ls, we will at least know it's not a working Fortran
+        # compiler.
+        if test -z "$FC"; then
+            sage_spkg_install_gfortran=yes
+            SAGE_MUST_INSTALL_GCC([a Fortran compiler is missing])
+        fi

The use of the SAGE_MUST_INSTALL_GCC macro here is a mistake. It might be a remnant from before we created a separate "gfortran" package (that actually happened after I first introduced #24919, and while I tried to incorporate that change it looks like this might have been left in).

As Dima suggested it can just be removed and/or replaced with an AC_MSG_NOTICE.

@embray
Copy link
Contributor

embray commented Nov 19, 2018

comment:10

Actually, I think there needs to be a bit more logic restructuring too. That fi should be an else. There's no point in checking AC_FC_FREEFORM if $FC is empty, as in the original code.

@dimpase
Copy link
Member Author

dimpase commented Nov 19, 2018

comment:11

I'd say we're wedded to gfortran too much. As far as I know, flang is in a good state, and a good candidate to replace gfortran (well, yes, it's not that fast yet, but, OK...).

This ticket would be a good occasion to fix this, and check for presence of a meaningful Fortran compiler, not specifically gfortran.

@embray
Copy link
Contributor

embray commented Nov 19, 2018

comment:12

This really isn't specifically about gfortran. It is, in fact, just checking that a fortran compiler exists, and if not is installing gfortran. Let's not overcomplicate things here: We've identified a simple fix. I'll attach a patch if no one else does.

@dimpase
Copy link
Member Author

dimpase commented Nov 19, 2018

comment:13

Replying to @embray:

This really isn't specifically about gfortran. It is, in fact, just checking that a fortran compiler exists, and if not is installing gfortran. Let's not overcomplicate things here: We've identified a simple fix. I'll attach a patch if no one else does.

Please do.

@jhpalmieri
Copy link
Member

comment:14

Replying to @dimpase:

I'd say we're wedded to gfortran too much. As far as I know, flang is in a good state, and a good candidate to replace gfortran (well, yes, it's not that fast yet, but, OK...).

If flang is pretty good, then can you open another ticket to add it as an optional package? It would be nice to have a quicker way to build Sage on OS X for users who don't want to install their own gfortran.

@dimpase
Copy link
Member Author

dimpase commented Nov 19, 2018

comment:15

flang does not run on OSX yet. It does run on Linux and FreeBSD, though.

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

comment:16

flang is not the only option by far. When I started the work on clang I effectively widened the pool of C/C++ compiler usable - not limited to clang. In fact I compiled sage with the Intel C/C++ compiler (and gfortran) with minimal difficulties. But it's not widely advertised and certainly we cannot say it is supported. So widening fortran compiler is good and may be support a different one.

But building flang, OS X or not, is still a hardship. I am afraid Intel fortran will do horrible and unhelpful optimisation but should be tried and then there is the community PGI compiler. Those are closed sources but somewhat easily available across some platforms (may be not freeBSD).

@dimpase
Copy link
Member Author

dimpase commented Nov 19, 2018

comment:17

imho Sage should not be in business of building compilers.

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

Branch: u/fbissey/fortran

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

Commit: a666ff9

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

Author: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

comment:18

OK does this branch satisfy everyone.


New commits:

a666ff9Do not build gcc if only a fortran compiler is missing. More care about check logic. Don't restrict to gfortran.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 19, 2018

Changed commit from a666ff9 to c35c6ec

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 19, 2018

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

c35c6ecCorrect spacing

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

comment:20

Stuff is broken, trying to figure out why.


New commits:

c35c6ecCorrect spacing

@kiwifb
Copy link
Member

kiwifb commented Nov 19, 2018

comment:21

OK that was weird. After running bootstrap even after nicely reworking the syntax in the best possible ways and even in M4sh, I still had an empty else...fi section where I expected AC_FC_FREEFORM to be expanded.

It seems like that macro doesn't really like to live in a conditional. In fact it looks like it goes next to AC_PROG_FC all by itself. AC_PROG_FC (and also CC and CXX) is currently called in configure.ac, gcc/spkg-configure.m4 and of course gfortran/spkg-configure.m4.

I also learned a few things about AC_REQUIRE in the process.

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:22

I also did some testing with FC=/bin/ls because it was helpfully suggested in comments

checking whether we are using the GNU Fortran compiler... no
checking whether /bin/ls accepts -g... no
checking for Fortran flag needed to accept free-form source... unknown
configure: Your Fortran compiler does not accept free-format source code
configure: which means the compiler is either seriously broken, or
configure: is too old to build Sage.
configure: === checking whether to install the gfortran SPKG ===

This is also nicely triggered if there is no FC found so it is enough.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2018

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

2449240Do not check for a fortran compiler in the C/C++ detection macro. Leave it to the fortran macro.
3185e91Extreme streamlining of fortran package macro.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2018

Changed commit from c35c6ec to 3185e91

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:24

I left the issue of multiple calls to AC_PROG_* for later overall. Just touched gcc for cosmetics really.

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:25

For the record I just built and successfully doctested sage with this ticket and the PGI fortran compiler. A couple of issues with doing that though. More about it in the morning before I move to Intel compilers.

@jhpalmieri
Copy link
Member

comment:26

Builds for me on OS X Mojave with my gfortran compiler temporarily removed. The Sage gfortran package took more than twice as long to build as any other Sage package, which I think is typical, but also reinforces my hope that we can suggest other fortran compilers for use with Sage.

@embray
Copy link
Contributor

embray commented Nov 20, 2018

comment:27

The AC_FC_FREEFORM macro always got expanded in a strange place for me too, and I could never figure out exactly why! I didn't realize it was already called automatically by AC_PROG_FC, so thank you for figuring that out. This looks good to me.

@embray
Copy link
Contributor

embray commented Nov 20, 2018

Reviewer: Erik Bray

@embray
Copy link
Contributor

embray commented Nov 20, 2018

comment:28

Replying to @jhpalmieri:

Builds for me on OS X Mojave with my gfortran compiler temporarily removed. The Sage gfortran package took more than twice as long to build as any other Sage package, which I think is typical, but also reinforces my hope that we can suggest other fortran compilers for use with Sage.

That's because this involves compiling most, if not all, of gcc and only installing the gfortran bits.

@jhpalmieri
Copy link
Member

comment:29

Replying to @embray:

Replying to @jhpalmieri:

Builds for me on OS X Mojave with my gfortran compiler temporarily removed. The Sage gfortran package took more than twice as long to build as any other Sage package, which I think is typical, but also reinforces my hope that we can suggest other fortran compilers for use with Sage.

That's because this involves compiling most, if not all, of gcc and only installing the gfortran bits.

I understand that, I just wish we could build a fortran compiler more quickly. (That's why I install /usr/local/bin/gfortran on every OS X machine I use for Sage.)

@embray
Copy link
Contributor

embray commented Nov 20, 2018

comment:30

Totally happy if someone wants to look into that. Too bad flang is (according to François) still difficult to install on macOS, which would defeat some of the purpose :(

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:31

Replying to @embray:

Totally happy if someone wants to look into that. Too bad flang is (according to François) still difficult to install on macOS, which would defeat some of the purpose :(

It's a pain on all OS full stop. I think you have to have a private patched build of llvm, you cannot reuse clang bits if you have them. I won't recommend PGI fortran (needs hand holding) but ifc may be fine, I can try other suggestions if I have access to them.

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:32

OK wrap up for people interested in using something like pgfortran, you can get it from https://www.pgroup.com/ .
Before anyone ask, you cannot currently use pgcc/pgc++ to compile sage because they don't pretend to be gcc unlike clang and Intel. sage still wants a compiler that at least pretend to be gcc.

Set your environment

export PATH="/path_to_compiler_prefix/bin:$PATH"
export LDFLAGS="-Wl,-rpath=/path_to_compiler_prefix/lib"
export FPICFLAGS="-fPIC"
./configure FC=pgfortran

FPICFLAGS is specific to R.

  • I will send a couple of PR to openblas. openblas's f_check script does a pretty good job of finding all the linking flags needed for your compiler. But in the case of PGI, specifically, ruins the results completely - literally WTF? I will attach a patch for now.

  • R needs hand holding. Its configure script has section that are hand crafted instead of using autotools standard macros. The only reason I can think of for doing that is ignorance. This is why you have to define FPICFLAGS in advance for it. However, it is not sufficient for configure to work out how to mix fortran and C code. So once the build breaks with an error on R

FC="pgfortran -fPIC" ./sage -i r

and once done resume the build normally.

Don't define FC=/path_to_compiler/my_fortran_compiler. numpy's distutils extension get confused if you do that and scipy's compilation will fail because gnu options are passed to the compiler instead of appropriate ones.

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

patch to compile openblas properly with pgfortran

@dimpase
Copy link
Member Author

dimpase commented Nov 20, 2018

comment:33

Attachment: pgfortran.patch.gz

Does non-Apple clang pretend to be GCC? How? Just wondering.

@kiwifb
Copy link
Member

kiwifb commented Nov 20, 2018

comment:34

Replying to @dimpase:

Does non-Apple clang pretend to be GCC? How? Just wondering.

It does. Compiler identification is by pragma, and clang on OS X or otherwise has the pragmas saying it is gcc-4.2.1. The intel compiler is a bit different, at installation time it will look at your default gcc install and mimic all the needed pragmas to fake it. Both compiler still have their own vendor pragmas. But by default AC_PROG_CC will test the gcc pragma to set the variable IS_GCC (or something like that) and both clang (any OS) and Intel will set it to yes.

@vbraun
Copy link
Member

vbraun commented Nov 21, 2018

Changed branch from u/fbissey/fortran to 3185e91

@kiwifb
Copy link
Member

kiwifb commented Nov 23, 2018

comment:36

And at last for those still interested I did a build with gcc+ifort (intel fortran compiler). Same kind of set up as for PGI. I had to also provide LD_LIBRARY_PATH for the compiler's libraries because they don't know how to find each other in the same folder :(

openblas need the upgrade to 0.3.3 because of this OpenMathLib/OpenBLAS#1548

R is still fiddly and you still need to define FPICFLAGS manually. I think most of the difficulties there would disappear by using the intel compiler for C/C++ as well as fortran.

Anyway everything was sorted and built properly sage works and pass its doctests with flying colours on linux.

Adding intel for C/C++ requires adjustment in pari and possibly sqlite from what I remember. But this is an alternative that could really be made to work.

@kiwifb
Copy link
Member

kiwifb commented Nov 23, 2018

Changed commit from 3185e91 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

5 participants