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.

Sign up for GitHub

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

Jump to bottom

R: add libcxx to default LDFLAGS and CPPFLAGS on Darwin #34651

Merged
merged 1 commit into from Feb 7, 2018
Merged

R: add libcxx to default LDFLAGS and CPPFLAGS on Darwin #34651

merged 1 commit into from Feb 7, 2018

Conversation

mnacamura
Copy link
Contributor

Motivation for this change

I am not sure this is the best solution for #34615 but it is sufficient to change LDFLAGS and CPPFLAGS in lib/R/etc/Makeconf:

@@ -15,7 +15,7 @@
 CC = /nix/store/cslvigh4a1656g9z9j1ndvzalx7v2s4k-clang-wrapper-4.0.1/bin/cc
 CFLAGS = -g -O2 $(LTO)
 CPICFLAGS = -fPIC
-CPPFLAGS = 
+CPPFLAGS = -isystem /nix/store/942hw6bhnppjqy7szdyfrr0fiiaj7818-libc++-4.0.1/include/c++/v1
 CXX = /nix/store/cslvigh4a1656g9z9j1ndvzalx7v2s4k-clang-wrapper-4.0.1/bin/c++ 
 CXXCPP = $(CXX) -E
 CXXFLAGS = -g -O2 $(LTO)
@@ -78,7 +78,7 @@
 ## needed by R CMD config
 LIBnn = lib
 LIBTOOL = $(SHELL) "$(R_HOME)/bin/libtool"
-LDFLAGS = 
+LDFLAGS = -L/nix/store/942hw6bhnppjqy7szdyfrr0fiiaj7818-libc++-4.0.1/lib
 LTO = 
 ## needed to build applications linking to static libR
 MAIN_LD = $(CC)
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
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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.

@mnacamura mnacamura requested a review from peti as a code owner February 6, 2018 07:04
peti
peti reviewed Feb 6, 2018
View reviewed changes
pkgs/applications/science/math/R/default.nix Outdated
@@ -61,6 +61,12 @@ stdenv.mkDerivation rec {
echo >>etc/Renviron.in "TZDIR=${tzdata}/share/zoneinfo"
'';

postBuild = stdenv.lib.optionalString stdenv.isDarwin ''
Copy link
Member

Choose a reason for hiding this comment

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

Why do you make that change post build? Shouldn't that be post configure?

peti
peti reviewed Feb 6, 2018
View reviewed changes
pkgs/applications/science/math/R/default.nix Outdated
@@ -61,6 +61,12 @@ stdenv.mkDerivation rec {
echo >>etc/Renviron.in "TZDIR=${tzdata}/share/zoneinfo"
'';

postBuild = stdenv.lib.optionalString stdenv.isDarwin ''
sed -i etc/Makeconf \
Copy link
Member

Choose a reason for hiding this comment

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

Does R not have some configure flag that allows us to configure those paths? Messing with the internals of the build state feels somewhat fragile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes R does. ./configure --help says

CPPFLAGS    (Objective) C/C++ preprocessor flags, e.g. -I<include dir> if
              you have headers in a nonstandard directory <include dir>
LDFLAGS     linker flags, e.g. -L<lib dir> if you have libraries in a
              nonstandard directory <lib dir>

LnL7
LnL7 approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

Strange that this is necessary since libc++ is part of the darwin stdenv, but the fix looks good.

@LnL7
Copy link
Member

LnL7 commented Feb 6, 2018

@GrahamcOfBorg build R rPackages.rstan

@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Partial log (click to expand)

error: while evaluating the attribute 'buildInputs' of the derivation 'R-3.4.3' at /var/lib/gc-of-borg/nix-test-rs-1/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-1/pkgs/stdenv/generic/make-derivation.nix:148:11:
while evaluating the attribute 'buildInputs' of the derivation 'openjdk-8u172b02' at /var/lib/gc-of-borg/nix-test-rs-1/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-1/pkgs/stdenv/generic/make-derivation.nix:148:11:
while evaluating the attribute 'buildCommand' of the derivation 'openjdk-bootstrap' at /var/lib/gc-of-borg/nix-test-rs-1/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-1/pkgs/stdenv/generic/make-derivation.nix:148:11:
No bootstrap for system

@LnL7 LnL7 added the 6.topic: darwin Running or building packages on Darwin label Feb 6, 2018
@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Partial log (click to expand)

** testing if installed package can be loaded
* DONE (rstan)
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/mdbi0q88hcx7zwjw5sj4jgvwznmim3i6-r-rstan-2.16.2
shrinking /nix/store/mdbi0q88hcx7zwjw5sj4jgvwznmim3i6-r-rstan-2.16.2/library/rstan/libs/rstan.so
strip is /nix/store/5qj61lcvzlap87rf6blvf8p577d482bv-binutils-2.28.1/bin/strip
patching script interpreter paths in /nix/store/mdbi0q88hcx7zwjw5sj4jgvwznmim3i6-r-rstan-2.16.2
checking for references to /tmp/nix-build-r-rstan-2.16.2.drv-0 in /nix/store/mdbi0q88hcx7zwjw5sj4jgvwznmim3i6-r-rstan-2.16.2...
/nix/store/r24jkaa4vqwxhqi244110bj9yscisdbs-R-3.4.3
/nix/store/mdbi0q88hcx7zwjw5sj4jgvwznmim3i6-r-rstan-2.16.2

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Partial log (click to expand)

*** copying figures
** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (rstan)
post-installation fixup
strip is /nix/store/5a88zk3jgimdmzg8rfhvm93kxib3njf9-cctools-binutils-darwin/bin/strip
patching script interpreter paths in /nix/store/c3x239xfbsfiwjk3ajj5ffnns9qjp6qz-r-rstan-2.16.2
/nix/store/9pyb74nhgsii2z1mzicclbs15lr27nzm-R-3.4.3
/nix/store/c3x239xfbsfiwjk3ajj5ffnns9qjp6qz-r-rstan-2.16.2

@mnacamura
Copy link
Contributor Author

Is it better to add NIX_LDFLAGS to R's LDFLAGS and NIX_CFLAGS_COMPILE to R's CPPFLAGS? See #34615 (comment)

@peti
Copy link
Member

peti commented Feb 7, 2018

No, I think that would be worse. I'd try to stay away from these magic hidden variables as much as possible. If you run R's configure script with ./configure --help, you'll see a whole lot of variables that can be passed at configure time to add search paths to appropriate places. That is the mechanism we should use, IMHO.

@peti
Copy link
Member

peti commented Feb 7, 2018

Ah, I see you changed the PR already to do exactly that. Very cool, thank you.

@peti peti merged commit fa23ff4 into NixOS:master Feb 7, 2018
@mnacamura mnacamura deleted the r-darwin-libcxx branch February 7, 2018 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peti peti peti left review comments

@LnL7 LnL7 LnL7 approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mnacamura @LnL7 @GrahamcOfBorg @peti