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

[staging] gfortran: default to same gcc version as stdenv #148539

Merged
merged 6 commits into from
Apr 15, 2022

Conversation

r-burns
Copy link
Contributor

@r-burns r-burns commented Dec 4, 2021

This is a more expected default behavior, so that the unversioned
gcc/gfortran toplevel attrs have the same version. It's also more
robust in cases where C and Fortran objects are being linked together.

With this commit, gcc.version == gfortran.version == "10.3.0"
(on x86_64-linux), bumping the default gfortran version from 9 to 10.

Motivation for this change
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@r-burns
Copy link
Contributor Author

r-burns commented Dec 4, 2021

The only change noted on the 9 -> 10 migration guide for fortran is that mismatched arguments cause errors now: https://gcc.gnu.org/gcc-10/porting_to.html

This usage was already invalid fortran, and I haven't encountered any affected packages, but I'll keep testing to see if it's something we need to worry about.

@ofborg ofborg bot requested a review from Wulfsta December 4, 2021 22:12
@Wulfsta
Copy link
Member

Wulfsta commented Dec 4, 2021

The elmerfem changes are fine. Definitely need to pull in someone more knowledgeable about the gfortran change than myself though. I’ll complete the review once we’ve got someone more suited to look at this.

@r-burns
Copy link
Contributor Author

r-burns commented Dec 5, 2021

Thanks, feel free to cherry-pick to master if you'd like to update separately. Grepping for gfortran only nets about 100 matches so I can probably make sure all the direct dependers are still working. Gonna mark this as draft until then.

@r-burns r-burns marked this pull request as draft December 5, 2021 01:58
@Wulfsta Wulfsta mentioned this pull request Dec 26, 2021
13 tasks
@r-burns
Copy link
Contributor Author

r-burns commented Dec 27, 2021

Rebased to include cc690c8

@oxalica
Copy link
Contributor

oxalica commented Dec 27, 2021

Note that the gcc patch for riscv 0d1750e is only included by gcc 10 and 11. Thus on riscv platforms, current gfortran using gcc9 will suffer from sysdir issues.
I'd also think we should bump the default one.

@micahcc
Copy link

micahcc commented Mar 11, 2022

This is a pretty big deal, this should be merged immediately. Example of an issue that can arise from this:

python
>>> import scipy.spatial
>>> import torch
$ pgrep -f -a python
15935 /nix/store/afi0ysqw20yiiw2gr2d28dx40bc4ddf8-python3-3.9.10/bin/python
python  15935 micahc  mem    REG  254,0   1952240 35015494 /nix/store/v4i14fk4ddy86jcw0k4s1n2d91p2m3dk-gfortran-9.3.0-lib/lib/libstdc++.so.6.0.28
$ lsof -p 15935 | grep libstd
python
>>> import torch
$ pgrep -f -a python
16636 /nix/store/afi0ysqw20yiiw2gr2d28dx40bc4ddf8-python3-3.9.10/bin/python
python  16636 micahc  mem    REG  254,0   1903088 34770303 /nix/store/ndnqiz3nnifj1blhg9q626xlmkqq1nmh-gcc-10.3.0-lib/lib/libstdc++.so.6.0.28

This means that torch when imported after scipy.spatial will get a difference libstdc++ than it was compiled with. I haven't tested everything in torch, but I can guarantee you there are bugs when this occurs. I suspect this probably happens in many places with code that is compiled with both stdenv and gfortran.

@tpwrules
Copy link
Contributor

Can this be re-evaluated and un-drafted? It seems like a smart choice.

@r-burns
Copy link
Contributor Author

r-burns commented Apr 10, 2022

Ah yes, forgot about this for a minute. As I recall there are actually quite a few packages broken by this, but fixing them is usually as simple as adding -std=legacy or -fallow-argument-mismatch. (I'm not sure which one is more correct to use, the first seems more self-documenting and may be more correct going forward, but is potentially broader in scope.) Let me rebase what I have locally and see where we're at.

This is a more expected default behavior, so that the unversioned
gcc/gfortran toplevel attrs have the same version. It's also more
robust in cases where C and Fortran objects are being linked together.

With this commit, gcc.version == gfortran.version == "10.3.0"
(on x86_64-linux), bumping the default gfortran version from 9 to 10.
@r-burns
Copy link
Contributor Author

r-burns commented Apr 10, 2022

Some good news, some of the packages I previously needed to work around such as scalapack and elmerfem have been fixed upstream in the meantime.

@r-burns
Copy link
Contributor Author

r-burns commented Apr 11, 2022

Successfully ran:

nix build -j8 -f . arpack python3.pkgs.{numpy,scipy} openblas suitesparse sundials liblapack fftw{,Float} mpich cholmod-extra cernlib magma

Not an exhaustive list, but a good sample of direct dependers.

@r-burns r-burns marked this pull request as ready for review April 11, 2022 04:16
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Though I'm not familiar with the issues that motivated this PR so I'm hesitated to merge my self.

@@ -22,6 +22,8 @@ stdenv.mkDerivation rec {
"--enable-shared"
"--enable-sharedlib"
"--with-pm=${withPm}"
"FFLAGS=-fallow-argument-mismatch" # https://github.com/pmodels/mpich/issues/4300
Copy link
Member

@markuskowa markuskowa Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that this is already obsolete with the next version of mpich (which has just been released in the past week).

@markuskowa
Copy link
Member

@GrahamcOfBorg build mpich mvapich

@tpwrules
Copy link
Contributor

tpwrules commented Apr 11, 2022

I ran this command on aarch64-linux, which is the packages previous, plus the ones changed by this PR, minus the ones that are tagged as unsupported:

nix build -j4 -f . arpack python3.pkgs.{numpy,scipy} openblas suitesparse sundials liblapack fftw{,Float} mpich cholmod-extra qcdnum qrupdate netcdffortran

Unfortunately, it looks like the new flags cause mpich to break with gfortran9. Last log lines:

error: builder for '/nix/store/gimba1a9akbnm0vrpgqppinky8kf03jx-mpich-4.0.1.drv' failed with exit code 1;
checking whether Fortran 77 compiler accepts option -O2... no
checking how to get verbose linking output from gfortran... configure: WARNING: compilation failed

checking for Fortran 77 libraries of gfortran... 
checking whether gfortran accepts the FLIBS found by autoconf... no
checking for valid entries in FLIBS... 
checking whether gcc links with FLIBS found by autoconf... yes
checking whether Fortran 77 and C objects are compatible... no
checking for file... no
checking for linker for Fortran main program... configure: error: Could not determine a way to link a Fortran test program!

Can those flags be made conditional on aarch64-linux or the gfortran version? Or possibly mpich upgraded like @markuskowa suggested? Everything else builds and passes tests, though I didn't check operation of the binaries.

@r-burns
Copy link
Contributor Author

r-burns commented Apr 12, 2022

@tpwrules thanks for reporting, I've made mpich only build with -fallow-argument-mismatch for gfortran 10+. Latest mpich does seem to solve the issue but I'd rather keep this PR narrow in focus.

@ofborg ofborg bot requested review from markuskowa and doronbehar April 12, 2022 03:31
@mweinelt mweinelt merged commit ae0bb28 into NixOS:staging Apr 15, 2022
@r-burns r-burns deleted the gfortran-default-gcc branch April 15, 2022 02:57
@markuskowa
Copy link
Member

See #168772 for mpich update.

@Mindavi
Copy link
Contributor

Mindavi commented May 2, 2022

For some reason this broke cross-compilation with gfortran.

I'm building gfortran with this:

nix build .#pkgsCross.aarch64-multiplatform.buildPackages.gfortran

When I try to execute it, it says this:

./result/bin/aarch64-unknown-linux-gnu-gfortran: line 210: /nix/store/y8yb50h4vba4bngybkd64sgm5kab8sn3-gfortran-11.2.0/bin/aarch64-unknown-linux-gnu-gfortran: No such file or directory

When I look into the /nix/store/y8yb50h4vba4bngybkd64sgm5kab8sn3-gfortran-11.2.0/bin/ directory, I see this:

total 36852
lrwxrwxrwx 1 root root        3 Jan  1  1970 c++ -> g++
-r-xr-xr-x 1 root root  1226088 Jan  1  1970 cpp
-r-xr-xr-x 1 root root  1230184 Jan  1  1970 g++
-r-xr-xr-x 1 root root  1226088 Jan  1  1970 gcc
-r-xr-xr-x 1 root root    34792 Jan  1  1970 gcc-ar
-r-xr-xr-x 1 root root    34792 Jan  1  1970 gcc-nm
-r-xr-xr-x 1 root root    34792 Jan  1  1970 gcc-ranlib
-r-xr-xr-x 1 root root   715472 Jan  1  1970 gcov
-r-xr-xr-x 1 root root   563912 Jan  1  1970 gcov-dump
-r-xr-xr-x 1 root root   584392 Jan  1  1970 gcov-tool
-r-xr-xr-x 1 root root  1230184 Jan  1  1970 gfortran
-r-xr-xr-x 1 root root 29489536 Jan  1  1970 lto-dump
lrwxrwxrwx 1 root root        3 Jan  1  1970 x86_64-unknown-linux-gnu-c++ -> g++
lrwxrwxrwx 1 root root        3 Jan  1  1970 x86_64-unknown-linux-gnu-g++ -> g++
lrwxrwxrwx 1 root root        3 Jan  1  1970 x86_64-unknown-linux-gnu-gcc -> gcc
lrwxrwxrwx 1 root root        3 Jan  1  1970 x86_64-unknown-linux-gnu-gcc-11.2.0 -> gcc
-r-xr-xr-x 1 root root    34792 Jan  1  1970 x86_64-unknown-linux-gnu-gcc-ar
-r-xr-xr-x 1 root root    34792 Jan  1  1970 x86_64-unknown-linux-gnu-gcc-nm
-r-xr-xr-x 1 root root    34792 Jan  1  1970 x86_64-unknown-linux-gnu-gcc-ranlib
-r-xr-xr-x 1 root root  1230184 Jan  1  1970 x86_64-unknown-linux-gnu-gfortran

I didn't expect the cross-compiler for gfortran to refer to the native/x86_64 compiler.

Maybe related to #132651?

@r-burns
Copy link
Contributor Author

r-burns commented May 2, 2022

Ah thank you for pointing that out, I see what you mean. I think it may be a simple host/target offset issue. Instead of wrapCC (gccStdenv, I think I should have used wrapCC (targetPackages.gccStdenv. Trying that out to see if it fixes it.

Edit: yes that seems to fix it, would you mind confirming?

@Mindavi
Copy link
Contributor

Mindavi commented May 3, 2022

Will check that, let me rebuild gfortran and check if fftw now cross-compiles again.

Causes an eval error:

rick@nixos-asus:~/hobby/nixos-mobile-nixpkgs$ nix build .#pkgsCross.aarch64-multiplatform.fftw
error: attribute 'gccStdenv' missing

       at /nix/store/ajb3irf7jmjl282mx3w06sq34d3swpav-source/pkgs/top-level/all-packages.nix:12683:22:

        12682|   # Use the same GCC version as the one from stdenv by default
        12683|   gfortran = wrapCC (targetPackages.gccStdenv.cc.cc.override {
             |                      ^
        12684|     name = "gfortran";
(use '--show-trace' to show detailed location information)

The gfortran I just built with nix build .#pkgsCross.aarch64-multiplatform.buildPackages.gfortran seems to be referencing the right compiler now though:

rick@nixos-asus:~/hobby/nixos-mobile-nixpkgs$ ./result/bin/aarch64-unknown-linux-gnu-gcc 
aarch64-unknown-linux-gnu-gcc: fatal error: no input files
compilation terminated.

@NickCao NickCao mentioned this pull request May 3, 2022
13 tasks
@r-burns
Copy link
Contributor Author

r-burns commented May 3, 2022

Oh I see. How about instead of wrapCC (gccStdenv.cc.cc, simply wrapCC (gcc.cc? That seems like the most straightforward way to do it.

@Mindavi
Copy link
Contributor

Mindavi commented May 3, 2022

Sounds fine to me. Haven't tested but assuming that'll work. The splicing stuff is kind of leaky at times.

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

Successfully merging this pull request may close these issues.

9 participants