cc-wrapper: Cleanup of Nix #28556
cc-wrapper: Cleanup of Nix #28556
Conversation
Ericson2314
requested review from
edolstra,
LnL7,
copumpkin,
fpletz and
globin
No hashes were changed
Ericson2314
force-pushed
the
cc-wrapper-nix-cleanup
branch
from
706e832 to
47a5dab
Compare
There was a problem hiding this comment.
Here's 3 more interesting changes. Two of which were needed for set -u compliance for reasons I don't fully understand.
@fpletz the first comment, marked outdated, concerns arguing. Pinging you here as a ping from a hidden message probably looks like a GitHub bug :).
| @@ -320,7 +322,7 @@ stdenv.mkDerivation { | |||
| rm $out/nix-support/setup-hook.tmp | |||
|
|
|||
| # some linkers on some platforms don't support specific -z flags | |||
| hardening_unsupported_flags="" | |||
| export hardening_unsupported_flags="" | |||
There was a problem hiding this comment.
Without this export, substitute failed. but more more broadly I'm surprised it previously worked at all: substituteAll only crawls through the output of env, not set, yet it's important this variable be substituted. Maybe it was only on some branches exported?
CC @fpletz
There was a problem hiding this comment.
I checked in both builds and @hardening_unsupported_flags@ was substituted.
|
|
||
| if [[ -z ''${dynamicLinker+x} ]]; then | ||
| echo "Don't know the name of the dynamic linker for platform '${targetPlatform.config}', so guessing instead." >&2 | ||
| local dynamicLinker="${libc_lib}/lib/ld*.so.?" |
There was a problem hiding this comment.
Added local to avoid it lasting to the next phase.
There was a problem hiding this comment.
Ensured in both builds this made it the right file.
| echo ${libc_lib}/lib/32/ld-linux.so.2 > $out/nix-support/dynamic-linker-m32 | ||
| fi | ||
|
|
||
| local ldflagsBefore=(-dynamic-linker "$dynamicLinker") |
There was a problem hiding this comment.
Added local to avoid it lasting to the next phase, and because it weirdly caused substituteAll problems if I didn't.
There was a problem hiding this comment.
Ensured in finished Linux build that these were still written to the right file.
Ericson2314
force-pushed
the
cc-wrapper-nix-cleanup
branch
from
47a5dab to
df7c305
Compare
|
Fixed Linux's |
| @@ -320,15 +234,129 @@ stdenv.mkDerivation { | |||
|
|
|||
| + optionalString cc.langVhdl or false '' | |||
| ln -s $ccPath/${prefix}ghdl $out/bin/${prefix}ghdl | |||
| ''; | |||
|
|
|||
| propagatedBuildInputs = extraPackages; | |||
There was a problem hiding this comment.
This was tantamount propagatedNativeBuildInputs before, but it doesn't (currently) make a difference with native, and this is more correct with cross.
|
I'm interested in merging this within the next 24 hours---I've gone any manual checked everything I was concerned about, as documented in the comments above. |
Motivation for this change
I cleaned up the installation script a bit. This is a prereq for me splitting out the ld wrappers into a
binutils-wrapper.One of the changes was using
set -ubut that will be simpler and cleaner if #28057 is accepted. This also depends on #28556This is most easily reviewed commit-by-commit.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)