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

llvm-stdenv: fix default stdlib #28870

Closed
wants to merge 1 commit into from
Closed

llvm-stdenv: fix default stdlib #28870

wants to merge 1 commit into from

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Sep 1, 2017

Motivation for this change

Fixes #28223

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

@mention-bot
Copy link

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

Ericson2314
Ericson2314 requested changes Sep 1, 2017
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment •
edited

Choose a reason for hiding this comment

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

I think https://github.com/NixOS/nixpkgs/pull/28870/files#diff-ae6d7fc8f1059168bd391972ef177c31R43 will unfortunately ensure that's defined no matter what. We'll have to special case that var taking it out of the loop.

@LnL7
Copy link
Member Author

LnL7 commented Sep 1, 2017 •
edited

It works fine, NIX_CXXSTDLIB_COMPILE will be either empty or the default at that point.

$ nix-shell -E 'with import ./. {}; llvmPackages.stdenv.mkDerivation { name = "foo"; }' --run 'NIX_DEBUG=1 $CXX'
HARDENING: disabled flags: IGNORED_KEY
HARDENING: Is active (not completely disabled with "all" flag)
HARDENING: enabling fortify
HARDENING: enabling stackprotector
HARDENING: enabling pic
HARDENING: enabling strictoverflow
HARDENING: enabling format
HARDENING: enabling relro
HARDENING: enabling bindnow
extra flags before to /nix/store/gmb9n24q2brhd3x2z1c5zv3psy1yv4jk-clang-4.0.1/bin/clang++:
  ''
original flags to /nix/store/gmb9n24q2brhd3x2z1c5zv3psy1yv4jk-clang-4.0.1/bin/clang++:
  ''
extra flags after to /nix/store/gmb9n24q2brhd3x2z1c5zv3psy1yv4jk-clang-4.0.1/bin/clang++:
  -B/nix/store/gmb9n24q2brhd3x2z1c5zv3psy1yv4jk-clang-4.0.1/lib
  -B/nix/store/yz5vwqycca5jrgcdigxyj5776jalbvsh-glibc-2.25/lib/
  -idirafter
  /nix/store/6jyqh11wgbam661h9sqfpydan6jhsh30-glibc-2.25-dev/include
  -idirafter
  -B/nix/store/i3knpsfj710dxmpx73vbh16mhfpv4jnh-clang-wrapper-4.0.1/bin/
  -isystem
  /nix/store/dj2w6h9vyma0bvrchb8npwk6s1m62j10-gcc-6.4.0/include/c++/6.4.0
  -isystem
  /nix/store/dj2w6h9vyma0bvrchb8npwk6s1m62j10-gcc-6.4.0/include/c++/6.4.0/x86_64-unknown-linux-gnu
  -O2
  -D_FORTIFY_SOURCE=2
  -fstack-protector-strong
  --param
  ssp-buffer-size=4
  -fPIC
  -fno-strict-overflow
  -Wformat
  -Wformat-security
  -Werror=format-security
clang-4.0: error: no input files

@LnL7
Copy link
Member Author

LnL7 commented Sep 1, 2017

@Ericson2314 just finished both afl and python27Packages.vowpalwabbit, they are both ok.

@Ericson2314
Copy link
Member

Oh my bad, I thought the export line used the salted variable. I see how it works now, but unfortunately I don't think this will work in the cross case, where that variable won't be inspected if the "host role" is not in use. For that reason we do need to stuff it directly into the salted var, but only if none of the activities input roll-based ones are defined.

@LnL7
Copy link
Member Author

LnL7 commented Sep 2, 2017 •
edited

Yeah, I thought so. Would doing the same for the build/target variants (role infix) work?

@Ericson2314
Copy link
Member

@LnL7 I suppose, though the flags will be repeated when more the cc-wrapper takes on multiple roles. I think going straight to defining the salted var like I described is cleanest for now---I'll just look forward to the day when we can remove all CXXSTDLIB_*.

@Ericson2314 Ericson2314 added this to the 17.09 milestone Sep 3, 2017
@Ericson2314 Ericson2314 added 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 9.needs: port to stable A PR needs a backport to the stable release. labels Sep 3, 2017
@LnL7 LnL7 requested a review from FRidh as a code owner September 4, 2017 19:27
@LnL7 LnL7 mentioned this pull request Sep 4, 2017
Merged
8 tasks
@Ericson2314 Ericson2314 added this to After big PR in Cross compilation Sep 5, 2017
@LnL7 LnL7 closed this Sep 5, 2017
@LnL7 LnL7 deleted the stdenv-default-stdlib branch September 5, 2017 19:35
@LnL7 LnL7 removed the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 5, 2017
@Ericson2314 Ericson2314 removed this from After big PR in Cross compilation Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: cross-compilation Building packages on a different sort platform than than they will be run on
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LnL7 @mention-bot @Ericson2314