cc-wrapper: Hygiene and set -u

[Ericson2314]

Motivation for this change

For cross compilation, we need to have multiple cc-wrappers in use at once (usually so we can generate built-tools on the fly along with building the derivation itself). But cc-wrapper leaks a bunch of environment variables in its setup hook to be picked up by the wrapper scripts, so multiple cc-wrappers would stomp all over each other. The hack solution to that alone is simple---make sure the two cc-wrapper don't use the same identifiers, which ${infixSalt} accomplishes.

The catch is that many derivations try to mess with this process by modifying those variables. I specially modified the setup hook to pick up "unsalted" environment variables from derivation itself (or an earlier setup hook...yikes), but derivations (like glibc; see the last commit) also inject bash to modify these too. git grep 'export NIX_' can give one a scale of this. I do not yet have a general solution for how not to break these packages...

...Now I do. I do the conversion in add-flags instead. Also, normal host-platform-targetting env vars keep their existing notation, so this (at least by the standards of my recent PRs :D) is highly backwards compatible. The only semantics change I remember off-hand is that the START_HOOKs are now evaluated after add-flags.sh, but those are probably dead code---unused in nixpkgs at least.

This is best reviewed commit-by-commit, as some commits are basically seddish and bump up the diff count. Also start with #27879

[Longer term, instead of an unconditional envHook, I'd like to use pkg-config. By default, cc-wrapper will inject the appropriate pkg-config invocations, but this can be disabled per-derivation. Derivations like glibc that want to be tricky instead call it or do a deferred injection themselves.]

Things done

Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.

• Tested using sandboxing
  (nix.useSandbox on NixOS,
  or option build-use-sandbox in nix.conf
  on non-NixOS)
• Built stdenv on platform(s)
  • NixOS
  • macOS
  • Linux
• Evaluated all of nixpkgs
• 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.

---

CC @orivej, as you have been working on this a fair amount lately.

[cstrahan]

@Ericson2314 If I'm reading this correctly, won't this cause problems where we use e.g. NIX_LDFLAGS=$NIX_LDFLAGS:$etc_etc_etc within a phase, and ditto for within a nix-shell?

Your slurpUnsalted would, I think, only catch stuff that was defined within the derivation's environment (e.g. set directly as one of the attrs within a call to mkDerivation).

[Ericson2314]

@cstrahan Indeed that is exactly the flaw I'm worried about. I'm not sure what to do about that, considering the order of flags does matter.

[LnL7]

I'm probably missing something. But wouldn't the issues with setting NIX_*FLAGS during a build be solved by having the wrappers look at both the salted and unsalted versions of the environment variables?

[Ericson2314]

@LnL7 Just saw. Well non-nativeBuildInput (which is where stdenv.cc is injected) cc-wrapper should not ignore those flags. But I think I have a solution.

1. The setup hook will keep a "set" off the rolls the specific cc-wrapper is used in. Something like:

   if isPreNativeBuildInput; NIX_CC_WRAPPER_${salt}_ROLE+=("BUILD_"); fi
   if isNativeBuildInput; NIX_CC_WRAPPER_${salt}_ROLE+=(""); fi
   if isBuildInput; NIX_CC_WRAPPER_${salt}_ROLE+=("TARGET_"); fi
   

   N.B. These conditions will be faked with crossConfig like this PR until Make cross compilation elegant #26805

2. The env hook (for-each-ed on each dep) will append to the role-specific, not salt-specific names (e.g. NIX_TARGET_CFLAGS_COMPILE). Since the setup hook is run for each included cc-wrapper dep, it still only works on one sort of dependency at a time (not all roles).

3. add-flags.sh will iterate through NIX_CC_WRAPPER_${salt}_ROLE and agglomerate in non-exported camel-case names.

4. The wrapper scripts will only use the camel-case names.

[Ericson2314]

OK! Rewrote this sort of as described. Probably best to review commit-by-commit as some commits are noisy but borring. set -u compliance helps demonstrate correctness (e.g. won't be forgetting to convert a env var so easily).

[grahamc]

Why SC2016?

[Ericson2314]

Good question....

[Ericson2314]

Ah it used to say $hardeningDisabled or whatever

[grahamc]

Nicely palindromic PR #

Generally looks fine, but I admit to not being really familiar with stdenv and the various things we do with it.

[grahamc]

can you add some more info to this comment as to what it means, for future archeologists?

[grahamc]

hmm you sure?

[grahamc@Morbo:~]$ declare -i n=0

[grahamc@Morbo:~]$ i+=1

[grahamc@Morbo:~]$ echo $i
1

[grahamc@Morbo:~]$ i+=1

[grahamc@Morbo:~]$ echo $i
11

[grahamc]

same "are you sure?"

[grahamc]

nice rename

[grahamc]

Hmm not sure why we're keeping this

[Ericson2314]

I don't know anything about gnat --- I'd figure I'd at least keep the comment up to date?

[grahamc]

typo

[Ericson2314]

Rebased just to get rid of merge conflict with my macOS Sierra linking hack---just a nix conflict no hashes were affected so my old builds are still valid.

[FRidh]

This broke postgresql https://hydra.nixos.org/build/58192157.

[orivej]

Postgresql ./configure is broken because /nix/store/…-glibc-2.25/lib/librt.so.1 has /nix/store/…-bootstrap-glibc/lib in its RUNPATH; previously it had no RUNPATH. This may happen if NIX_DONT_SET_RPATH is not respected…

[Ericson2314]

Very weird---https://github.com/NixOS/nixpkgs/pull/27672/files#diff-ae6d7fc8f1059168bd391972ef177c31R70 should do the trick. When developing this, a lack of respecting that up caused the Linux stdenv to fail to build at all, so the current issue must be especially subtle.

Also, I'm fine reverting this, especially as the next cross PR won't be ready for a while.

[FRidh]

Some fragments from config.log.

configure:13164: result: failed
configure:13166: error:
Could not execute a simple test program.  This may be a problem
related to locating shared libraries.  Check the file 'config.log'
for the exact reason.

configure:3847: $? = 0
configure:3836: gcc -v >&5
Using built-in specs.
COLLECT_GCC=/nix/store/3vicjdaxfvcfz3nvc8wghp31n22rflq1-gcc-5.4.0/bin/gcc
COLLECT_LTO_WRAPPER=/nix/store/3vicjdaxfvcfz3nvc8wghp31n22rflq1-gcc-5.4.0/libexec/gcc/x86_64-unknown-linux-gnu/5.4.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with:
Thread model: posix
gcc version 5.4.0 (GCC)
configure:3847: $? = 0
configure:3836: gcc -V >&5
gcc: error: unrecognized command line option '-V'
gcc: fatal error: no input files
compilation terminated.
configure:3847: $? = 1
configure:3836: gcc -qversion >&5
gcc: error: unrecognized command line option '-qversion'
gcc: fatal error: no input files

That reminds me of #27940 where export LD=$CXX became necessary. I don't know what caused that.

[FRidh]

@Ericson2314 if you don't think you will be able to fix this before the next staging evaluation, then I recommend reverting these changes.

[Ericson2314]

@FRidh Found it. The problem is very silly; I slurped NIX_DONT_SET_RPATH twice so it gets defined as 1 1.

(For cross I'll eventually need to have a different monoid for these numeric env vars, but for native it suffices to just stop slurping twice.)

[Ericson2314]

Fixed in 810fb0c

[orivej]

@Ericson2314 It seems that this pull request broke standalone gcc, ld:

$ /nix/store/…-gcc-wrapper-5.4.0/bin/gcc
/nix/store/…-gcc-wrapper-5.4.0/nix-support/add-flags.sh: line 75: NIX_x86_64_unknown_linux_gnu_CFLAGS_COMPILE: unbound variable

[Ericson2314]

Ah, that's unfortunate. It's because in the case that no "role" is defined, none of those variables get set. We should come up with some sane defaults for that case (all empty would probably be backwards compatible).

@globin With the LLVM test failure you found, it's possible the environment has been cleaned somehow, leading to the roles being undefined.

[orivej]

It appears that LLVM tests sanitize the environment for programs they run during the test. (llvm/test/tools/llvm-symbolizer/print_context.c attempts to run

// RUN: %host_cc -O0 -g %s -o %t 2>&1
// RUN: %t 2>&1 | llvm-symbolizer -print-source-context-lines=5 -obj=%t | FileCheck %s

where %host_cc expands to /nix/store/…-gcc-wrapper-5.4.0/bin/gcc.)
Restoring support for standalone gcc will fix that.

[globin]

Yep, working on it, give me a bit

[dezgeg]

Um, why is this infixSalt stuff needed at all, can't cc-wrapper.sh just directly use the 'role' variables like NIX_BUILD_CFLAGS_COMPILE?

[Ericson2314]

I tried to describe it in the code comment: then we can't get away with platform-tool and need something nonstandard like role-tool instead.

[dezgeg]

Sure, the names of the wrapped tools being after the platform triplet is fine, I just don't understand the need for the @infixSalt@ environment variables. Doesn't it just add possibilities for getting things mixed up if you happen to be cross compiling to something that is different from the native platform but maps to the same platform triplet? (E.g. ARM with some different FPU/ABI configurations or maybe baremetal stuff like http://wiki.osdev.org/Why_do_I_need_a_Cross_Compiler%3F someone was asking on IRC)

[Ericson2314]

@dezgeg We could use a hash of targetPlatform, or a hash of the entire cc-wrapper, instead. But unless we also likewise change the binary prefix, those two cc-wrappers will clobber each other on the PATH anyways.

I'm open to doing that or similar, though. What would you prefer?

[eddyb]

This appears to be setting the variables unconditionally, which broke #6688 (standalone clang++). I'm not sure what the exact interactions here are but nix run nixpkgs.clang can't find the C++ standard library headers and I've tracked it down to this variable assignment.

[eddyb]

Probably the right fix is to use ${NIX_CXXSTDLIB_COMPILE:-@default_cxx_stdlib_compile@} instead of ${NIX_CXXSTDLIB_COMPILE-@default_cxx_stdlib_compile@}, in cc-wrapper.sh to treat the empty variable the same way as if it was uninitialized, IIRC.

EDIT: opened as #44221.