Skip to content

cc-wrapper: fix wrapper tests and building for darwin on staging-next #107336

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
Closed
Emantor wants to merge 3 commits into from
Closed

cc-wrapper: fix wrapper tests and building for darwin on staging-next #107336

Emantor wants to merge 3 commits into from

Conversation

@Emantor
Copy link
Member

@Emantor Emantor commented Dec 21, 2020 •
edited
Loading

Motivation for this change

Fix darwin builds and the cc-wrapper-libcxx tests for staging-next. Opening as draft since I am not sure the first commit is the correct fix for this.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Emantor
cc-wrapper: disable dynamic linker for darwin
f6584a3 
Disable the dynamic linker if the platform is darwin. On darwin the
dynamic linker cannot be passed using the -dynamic-linker argument, but
needs to be explicitly set via environment variables. Passing the linker
via -dynamic-linker results in an error:

  ld: unknown option: -dynamic-linker=/usr/lib/dyld

Pass a isdarwin variable to the build script and disable the linker on
darwin systems.
@Emantor
cc-wrapper: useGccForLibs for libcxx == null
708cb80 
Since commit 11302dc ("clang, cc-wrapper: Move `--gcc-toolchain`
logic into CC wrapper") --gcc-toolchain was only passed i useGccForLibs
is true. However useGccForLibs is disabled if libcxx is null, which
results in the libcxxStdenv failing, since the linker can't find libgcc
while linking. Remove the condition check for libcxx == null, to let the
wrapper tests pass again.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 21, 2020
@Emantor Emantor changed the base branch from master to staging-next December 21, 2020 16:50
@Emantor Emantor requested a review from Ericson2314 December 21, 2020 16:50
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 8.has: module (update) This PR changes an existing module in `nixos/` and removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 21, 2020
@Emantor Emantor mentioned this pull request Dec 21, 2020
Merged
10 tasks
@vcunat
Copy link
Member

vcunat commented Dec 21, 2020

/cc @NixOS/darwin-maintainers I guess?

Emantor
Emantor commented Dec 21, 2020
View reviewed changes
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 21, 2020
@ofborg ofborg bot requested a review from peti December 21, 2020 18:46
LnL7
LnL7 reviewed Dec 21, 2020
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.

Polluting the wrappers with this type of logic doesn't seem ideal. Allowing this to be set in the environment and inheriting that from the stdenv seems like a nicer approach. Some similar things are done like that here.

nixpkgs/pkgs/stdenv/darwin/default.nix

Lines 36 to 38 in 8946ff8

export NIX_ENFORCE_NO_NATIVE=''${NIX_ENFORCE_NO_NATIVE-1}
export NIX_ENFORCE_PURITY=''${NIX_ENFORCE_PURITY-1}
export NIX_IGNORE_LD_THROUGH_GCC=1

@Emantor
Copy link
Member Author

Emantor commented Dec 21, 2020 •
edited
Loading

Polluting the wrappers with this type of logic doesn't seem ideal. Allowing this to be set in the environment and inheriting that from the stdenv seems like a nicer approach. Some similar things are done like that here.

nixpkgs/pkgs/stdenv/darwin/default.nix

Lines 36 to 38 in 8946ff8

export NIX_ENFORCE_NO_NATIVE=''${NIX_ENFORCE_NO_NATIVE-1}
export NIX_ENFORCE_PURITY=''${NIX_ENFORCE_PURITY-1}
export NIX_IGNORE_LD_THROUGH_GCC=1

I strongly agree, that is why I opened it as a draft. What do you think is the correct approach here, setting NIX_DYNAMIC_LINKER to darwin and checking for that? Or do we need another variable for clarity?

Edit: pushed a fixup commit which implements the special handling for darwin in NIX_DYNAMIC_LINKER.

@Ericson2314
Copy link
Member

Ericson2314 commented Dec 21, 2020

CC @Mic92. I'm curious what exactly want wrong with #91293/#92089/#94582 and #106908.

#106925 in particular was supposed to help with this exact issue?

@LnL7
Copy link
Member

LnL7 commented Dec 21, 2020

I would change setDynamicLinker=1 to NIX_SET_DYNAMIC_LINKER=${NIX_SET_DYNAMIC_LINKER:-1}.

@Ericson2314 That's just the test, the stdenv is still broken.

@ofborg ofborg bot added the 6.topic: stdenv Standard environment label Dec 21, 2020
@Emantor Emantor force-pushed the topic/fix_cc_wrapper branch from beb710a to 7b4f7da Compare December 21, 2020 20:36
@Emantor
Copy link
Member Author

Emantor commented Dec 21, 2020

I would change setDynamicLinker=1 to NIX_SET_DYNAMIC_LINKER=${NIX_SET_DYNAMIC_LINKER:-1}.

Okay, I just realized that my first fixup was also broken… it's late here so I'll be going to bed, if this hasn't been resolved by tomorrow I'll work on this some more.

@Emantor
Copy link
Member Author

Emantor commented Dec 22, 2020

Closing this, since the two changes have been reverted in staging-next.

@Emantor Emantor closed this Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@davidtwco davidtwco Awaiting requested review from davidtwco

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@veprbl veprbl Awaiting requested review from veprbl

@Lucus16 Lucus16 Awaiting requested review from Lucus16

@peti peti Awaiting requested review from peti

1 more reviewer

@LnL7 LnL7 LnL7 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: stdenv Standard environment 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Emantor @vcunat @Ericson2314 @LnL7