gcc: include dylibs for darwin build #24637

LnL7 · 2017-04-04T20:46:54Z

Motivation for this change

WIP: libgfortran is currently not found correctly when using the wrapper, but csdp etc does work when using gfortran.cc.lib directly.

Fixes #24197

/cc @copumpkin @vcunat

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-04-04T20:46:56Z

@LnL7, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peti, @edolstra and @vcunat to be potential reviewers.

copumpkin · 2017-04-04T21:18:28Z

pkgs/development/compilers/gcc/5/default.nix

@@ -237,10 +237,16 @@ stdenv.mkDerivation ({

  # This should kill all the stdinc frameworks that gcc and friends like to
  # insert into default search paths.
-  prePatch = if stdenv.isDarwin then ''
+  prePatch = stdenv.lib.optionalString stdenv.isDarwin ''


By making it into optionalString instead of if ... then ... else null, you actually change the hash on Linux 😄

Yes, but the builder also changed so I think it doesn't really matter.

But I guess if we merge this to staging (since we'll already make changes to Linux hash) then we can get rid of the null, which would be nice...

copumpkin · 2017-04-04T21:19:56Z

pkgs/development/compilers/gcc/builder.sh

@@ -251,6 +253,16 @@ postInstall() {
        done
    fi

+    if type "install_name_tool"; then


Is this still necessary if you get the install name right the first time above?

Have not really looked at it, but I still get a cycle when removing the install_name_tool -id commands.

copumpkin · 2017-04-05T01:21:54Z

@LnL7 adding this to csdp made it work:

diff --git a/pkgs/applications/science/math/csdp/default.nix b/pkgs/applications/science/math/csdp/default.nix
index 7aafe9da41..353607a0fc 100644
--- a/pkgs/applications/science/math/csdp/default.nix
+++ b/pkgs/applications/science/math/csdp/default.nix
@@ -14,6 +14,15 @@ stdenv.mkDerivation {
     substituteInPlace Makefile --replace /usr/local/bin $out/bin
   '';
 
+  # If we don't set this here (just setting it as a derivation variable gets overridden by something)
+  # something earlier in the build sets CC=clang (on Darwin) which skips the cc provided by the
+  # gfortran wrapper, which then doesn't include sensible library search paths. This should be done
+  # more cleanly, by making the fortran wrapper propagate the underlying libraries more cleanly, but
+  # I'm scared of changing the wrapper right now so for now this should work.
+  preBuild = ''
+    export CC=cc
+  '';
+
   preInstall = ''
     rm -f INSTALL
     mkdir -p $out/bin

So basically, it works on Linux because they look for gcc by default, and adding gfortran to buildInputs puts gfortran/bin/gcc sooner in the $PATH, which then means that you use the gfortran wrapper's gcc which adds the proper library search paths.

So in essence, it sort of feels like it works by accident on Linux. The proper behavior IMO is to use the stdenv C compiler properly, and gfortran's wrapper should provide some lib directory forwarding. Otherwise my patch above will force us to build csdp with gcc (instead of clang), but I don't think it makes sense to have to switch compiler just to get some fortran libraries.

Another simple fix would be to use gfortran.cc in the buildInputs, but then that barfs up a bunch of symbol errors when building with clang:

...
clang -O3 -ansi -Wall -DNOSHORTS -DUSEGETTIME -I../include   -c -o sortentries.o sortentries.c
ar cr libsdp.a  readprob.o sdp.o op_o.o psd_feas.o op_a.o op_at.o  Fnorm.o calc_pobj.o calc_dobj.o trace_prod.o zero_mat.o mat_mult.o sym_mat.o copy_mat.o addscaledmat.o  user_exit.o make_i.o allocmat.o initsoln.o initparams.o add_mat.o writesol.o readsol.o easysdp.o writeprob.o solvesys.o makefill.o mat_multsp.o norms.o linesearch.o matvec.o chol.o qreig.o tweakgap.o freeprob.o packed.o sortentries.o
make[1]: Leaving directory '/private/var/folders/vv/17b_hycx691699h20v458fnh0000gn/T/nix-build-csdp-6.1.1.drv-0/Csdp-6.1.1/lib'
cd solver; make csdp
make[1]: Entering directory '/private/var/folders/vv/17b_hycx691699h20v458fnh0000gn/T/nix-build-csdp-6.1.1.drv-0/Csdp-6.1.1/solver'
clang -O3 -ansi -Wall -DNOSHORTS -DUSEGETTIME -I../include    -c -o csdp.o csdp.c
clang -O3 -ansi -Wall -DNOSHORTS -DUSEGETTIME -I../include  csdp.o -L../lib -lsdp -llapack -lblas -lgfortran -lm -o csdp
clang-3.7: warning: argument unused during compilation: '-ansi'
Undefined symbols for architecture x86_64:
  "___addtf3", referenced from:
      __gfortran_random_r16 in libgfortran.a(random.o)
      __gfortran_arandom_r16 in libgfortran.a(random.o)
  "___divtf3", referenced from:
      _write_float in libgfortran.a(write.o)
  "___eqtf2", referenced from:
      _write_float in libgfortran.a(write.o)
  "___floatunditf", referenced from:
      __gfortran_random_r16 in libgfortran.a(random.o)
      __gfortran_arandom_r16 in libgfortran.a(random.o)
  "___getf2", referenced from:
      _write_float in libgfortran.a(write.o)
  "___gttf2", referenced from:
      _write_float in libgfortran.a(write.o)
  "___lttf2", referenced from:
      _write_float in libgfortran.a(write.o)
  "___multf3", referenced from:
      _write_float in libgfortran.a(write.o)
      __gfortran_random_r16 in libgfortran.a(random.o)
      __gfortran_arandom_r16 in libgfortran.a(random.o)
  "___subtf3", referenced from:
      _write_float in libgfortran.a(write.o)
  "_finiteq", referenced from:
      _write_float in libgfortran.a(write.o)
  "_isnanq", referenced from:
      _write_float in libgfortran.a(write.o)
  "_quadmath_snprintf", referenced from:
      _write_float in libgfortran.a(write.o)
  "_signbitq", referenced from:
      _write_float in libgfortran.a(write.o)
  "_strtoflt128", referenced from:
      __gfortrani_convert_real in libgfortran.a(read.o)
      __gfortrani_convert_infnan in libgfortran.a(read.o)
ld: symbol(s) not found for architecture x86_64
clang-3.7: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [Makefile:126: csdp] Error 1
make[1]: Leaving directory '/private/var/folders/vv/17b_hycx691699h20v458fnh0000gn/T/nix-build-csdp-6.1.1.drv-0/Csdp-6.1.1/solver'
make: *** [Makefile:12: all] Error 2
builder for ‘/nix/store/qw868zzbhczwdnaqx29nznpp5mab9i4b-csdp-6.1.1.drv’ failed with exit code 2
error: build of ‘/nix/store/qw868zzbhczwdnaqx29nznpp5mab9i4b-csdp-6.1.1.drv’ failed

I assume those missing symbols get magically passed in by gcc somehow. If I manually go add a couple more things, it works, but I'm not sure what the right way to do it is:

diff --git a/pkgs/applications/science/math/csdp/default.nix b/pkgs/applications/science/math/csdp/default.nix
index 7aafe9da41..9205f3e5d3 100644
--- a/pkgs/applications/science/math/csdp/default.nix
+++ b/pkgs/applications/science/math/csdp/default.nix
@@ -8,12 +8,14 @@ stdenv.mkDerivation {
     sha256 = "1f9ql6cjy2gwiyc51ylfan24v1ca9sjajxkbhszlds1lqmma8n05";
   };
 
-  buildInputs = [ blas gfortran liblapack ];
+  buildInputs = [ blas gfortran.cc liblapack ];
 
   postPatch = ''
     substituteInPlace Makefile --replace /usr/local/bin $out/bin
   '';
 
+  NIX_LDFLAGS = "-lgcc_ext.10.5 -lquadmath";
+
   preInstall = ''
     rm -f INSTALL
     mkdir -p $out/bin

Perhaps when building libgfortran.dylib on darwin, we manually add library dependencies on those two, so downstream dependencies don't need to care?

@vcunat @LnL7 thoughts welcome!

vcunat

At least on Linux side this seems OK (but I haven't tried it yet).

matthewbauer · 2017-04-09T06:45:50Z

😓 Just found this! I've made many of the same changes in a working branch and it seems to work well. It also seems like a lot of packages that fail on Hydra are built on top of Fortran so hopefully this fixes that !

LnL7 · 2017-04-12T19:45:26Z

Unless somebody an idea on how to improve this I would propose to merge and fix csdp separately for now.

matthewbauer · 2017-04-13T22:35:33Z

The *.dylib changes are definitely good! All of the install_name_tool makes me nervous though because it doesn't seem like gcc should be needing it. There are still lots of times with go packages that this same kind of cycle-avoidance needs to be done. I start to wonder if something is off with our linker/compiler system that the Linux stdenv doesn't have. I probably shouldn't be complaining because I was the one that originally put the darwin.usr-include in here which is super ugly but I still can't find a way around it.

vcunat · 2017-04-14T08:46:33Z

Do the **/5/default.nix changes need to be ported to other versions as well? (At least 6?)

LnL7 added 1.severity: mass-rebuild 2.status: work-in-progress 6.topic: darwin Running or building packages on Darwin labels Apr 4, 2017

copumpkin reviewed Apr 4, 2017

View reviewed changes

LnL7 changed the base branch from master to staging April 4, 2017 21:45

gcc: include dylibs for darwin build

9896cf1

LnL7 force-pushed the darwin-gcc branch from ca44559 to 9896cf1 Compare April 4, 2017 21:54

vcunat reviewed Apr 5, 2017

View reviewed changes

LnL7 changed the title ~~[WIP] gcc: include dylibs for darwin build~~ gcc: include dylibs for darwin build Apr 12, 2017

LnL7 removed the 2.status: work-in-progress label Apr 12, 2017

matthewbauer approved these changes Apr 13, 2017

View reviewed changes

vcunat merged commit 9896cf1 into NixOS:staging Apr 14, 2017

vcunat added a commit that referenced this pull request Apr 14, 2017

Merge #24637: gcc: include dylibs for darwin build

1a1f226

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcc: include dylibs for darwin build #24637

gcc: include dylibs for darwin build #24637

LnL7 commented Apr 4, 2017 •

edited

mention-bot commented Apr 4, 2017

copumpkin Apr 4, 2017

LnL7 Apr 4, 2017 •

edited

copumpkin Apr 4, 2017

copumpkin Apr 4, 2017

LnL7 Apr 6, 2017

copumpkin commented Apr 5, 2017

vcunat left a comment

matthewbauer commented Apr 9, 2017

LnL7 commented Apr 12, 2017 •

edited

matthewbauer commented Apr 13, 2017 •

edited

vcunat commented Apr 14, 2017

gcc: include dylibs for darwin build #24637

gcc: include dylibs for darwin build #24637

Conversation

LnL7 commented Apr 4, 2017 • edited

Motivation for this change

Things done

mention-bot commented Apr 4, 2017

copumpkin Apr 4, 2017

Choose a reason for hiding this comment

LnL7 Apr 4, 2017 • edited

Choose a reason for hiding this comment

copumpkin Apr 4, 2017

Choose a reason for hiding this comment

copumpkin Apr 4, 2017

Choose a reason for hiding this comment

LnL7 Apr 6, 2017

Choose a reason for hiding this comment

copumpkin commented Apr 5, 2017

vcunat left a comment

Choose a reason for hiding this comment

matthewbauer commented Apr 9, 2017

LnL7 commented Apr 12, 2017 • edited

matthewbauer commented Apr 13, 2017 • edited

vcunat commented Apr 14, 2017

LnL7 commented Apr 4, 2017 •

edited

LnL7 Apr 4, 2017 •

edited

LnL7 commented Apr 12, 2017 •

edited

matthewbauer commented Apr 13, 2017 •

edited