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

{cc,bintools}-wrapper: Some fixes #92089

Merged

Conversation

@Ericson2314
Copy link
Member

Ericson2314 commented Jul 2, 2020

Motivation for this change
  1. This means we can freely keep the comments up to date without the penalty of a mass rebuild.

  2. This will unbreak firefox and a few other packages which try to grab some of the libcxx flags.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built stdenv 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.
Ericson2314 added 2 commits Jun 30, 2020
This means we can freely keep the comments up to date without the
penalty of a mass rebuild.
This will unbreak firefox and a few other packages which try to grab
some of the libcxx flags.
@FRidh FRidh requested a review from vcunat Jul 2, 2020
@@ -228,58 +231,56 @@ stdenv.mkDerivation {
*) echo "Multiple dynamic linkers found for platform '${targetPlatform.config}'." >&2;;
esac
if [ -n "''${dynamicLinker:-}" ]; then
if [ -n "''${dynamicLinker-}" ]; then

This comment has been minimized.

@vcunat

vcunat Jul 2, 2020 Member

I'm mainly curious... this dash is some kind of special operator? I can't find this one in man bash.

This comment has been minimized.

@veprbl

veprbl Jul 2, 2020 Member

@vcunat See https://wiki.bash-hackers.org/syntax/pe#use_a_default_value

If you omit the : (colon), like shown in the second form, the default value is only used when the parameter was unset, not when it was empty.

This comment has been minimized.

@vcunat

vcunat Jul 5, 2020 Member

Thanks. Now I also read that man bash part again – more properly – and I see it there as well 🤦

@vcunat
vcunat approved these changes Jul 2, 2020
Copy link
Member

vcunat left a comment

Let's have this in the current staging-next iteration. The risks seem low enough and otherwise we'd have to work around those problems in some other way.

@vcunat vcunat merged commit 9dcb508 into NixOS:staging-next Jul 2, 2020
13 of 14 checks passed
13 of 14 checks passed
grahamcofborg-eval Checking original out paths
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="704daf7"; rev="704daf7a6e4e5f26c5b0ad67ed8bd1b2090aa8ed"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Ericson2314 Ericson2314 deleted the obsidiansystems:wrapper-ensure-file-exists branch Jul 2, 2020
@vcunat

This comment has been minimized.

Copy link
Member

vcunat commented on 704daf7 Jul 12, 2020

@Ericson2314: unfortunately this seems to have broken something. Regressed test case: tests.cc-multilib-clang. If I look right, the difference would be in nix-support/cc-{cflags,ldflags} retaining gcc paths before clang ones.

I don't really understand this stuff, so let me post the contents:

--- /nix/store/h8qy7qhlk52rbdymk0k0gqvdjrd5my6p-clang-wrapper-7.1.0/nix-support/cc-ldflags
+++ /nix/store/91ymyirc6hnjn8idsmsa5fflapcx6pyl-clang-wrapper-7.1.0/nix-support/cc-ldflags
@@ -1 +1 @@
- -L/nix/store/d0mgcdxjd399f2mqf0ayq7q7lajqppa0-clang-7.1.0-lib/lib 
+-L/nix/store/q5nmb2a999nfsshsnbikyqzb2hslp6cs-gcc-9.3.0/lib/gcc/x86_64-unknown-linux-gnu/9.3.0 -L/nix/store/d750xf35wvwalfji53sgyywwjjw07i22-gcc-9.3.0-lib/x86_64-unknown-linux-gnu/lib  -L/nix/store/n99zhzsy003zjl59wkz3zfdj8yncji0b-clang-7.1.0-lib/lib 
--- /nix/store/h8qy7qhlk52rbdymk0k0gqvdjrd5my6p-clang-wrapper-7.1.0/nix-support/cc-cflags
+++ /nix/store/91ymyirc6hnjn8idsmsa5fflapcx6pyl-clang-wrapper-7.1.0/nix-support/cc-cflags
@@ -1 +1 @@
- -B/nix/store/d0mgcdxjd399f2mqf0ayq7q7lajqppa0-clang-7.1.0-lib/lib 
+-target x86_64-unknown-linux-gnu -B/nix/store/q5nmb2a999nfsshsnbikyqzb2hslp6cs-gcc-9.3.0/lib/gcc/x86_64-unknown-linux-gnu/9.3.0  -B/nix/store/n99zhzsy003zjl59wkz3zfdj8yncji0b-clang-7.1.0-lib/lib 

This comment has been minimized.

Copy link
Member

vcunat replied Jul 12, 2020

If I wanted a quick fix (except rebuilding everything), I'd just revert those >> back to >; was there some reason for the change? (it can't affect existence of the files) Still, I expect the code isn't nicely written if this "mistake" was so easy to miss.

This comment has been minimized.

Copy link
Member Author

Ericson2314 replied Jul 13, 2020

Kicking off what seems like a disappointingly big build to repro the failure. The multi* compilers are quite hacky so I might just try to improve them instead of another mass rebuild. If that sounds good to you, maybe just temporarily disable the test?

This comment has been minimized.

Copy link
Member

vcunat replied Jul 13, 2020

It was already commented out for the unstable job, so it shouldn't matter as we want to fix it soon-ish anyway.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.