stdenv-setup: fix substituteAll with set -eu #28799

orivej · 2017-08-31T17:42:46Z

Motivation for this change

Environment variable filter in substituteAll was not precise and produced undefined and invalid variable names. Vladimír Čunát tried to fix that in [1], but env -0 did not work during Darwin bootstrap, so [2] reverted this change and replaced an error due to invalid variables with a warning. Recently in #28057 John Ericson added set -u to setup.sh and undefined variables made the setup fail during e.g. nix-build -A gnat with setup: line 519: !varName: unbound variable.

This patch fixes the cause of such warnings as [3]:

 WARNING: substitution variables should be valid bash names,
   "ccLDFlags+" isn't and therefore was skipped; it might be caused
   by multi-line phases in variables - see #14907 for details.

[1] 62fc885
[2] 81df035
[3] #14907 (comment)

Things done

Tested building some packages (e.g. nix) on NixOS and Darwin.

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 · 2017-08-31T17:42:48Z

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

dezgeg · 2017-08-31T17:58:10Z

Ugh, overriding a builtin feels dirty. Does env -0 work on Darwin? That should allow safe parsing even if the variable values contain newlines.

orivej · 2017-08-31T18:00:34Z

This is what 9e0d042 and [1] above tried to do, but env on Darwin does not understand -0.

dezgeg · 2017-08-31T18:04:15Z

How about awk 'BEGIN { for (v in ENVIRON) print v }' then?

I checked gawk is in Darwin bootstrap tools and ENVIRON is POSIX even (for stdenvNative).

orivej · 2017-08-31T18:29:46Z

Good idea!

copumpkin · 2017-08-31T18:41:01Z

env -0 works fine on Darwin inside most Nix contexts, because we just use the standard Nix tooling (coreutils, bash) in any pure context. The only place to be worried is the occasional script that runs under /bin/sh, where we can no longer control the PATH and bash being used.

orivej · 2017-08-31T19:12:45Z

@copumpkin setup.sh is needed on Darwin before any bash or env from nixpkgs is available. See 81df035.

orivej · 2017-08-31T19:57:01Z

@dezgeg

overriding a builtin feels dirty

Actually, overriding builtins with functions is well defined:

 2. If the name does not match a function, the shell searches for it in the list of shell builtins.

https://www.gnu.org/software/bash/manual/html_node/Command-Search-and-Execution.html

Nevertheless, I'm going to continue with awk for now.

Could someone test this on hydra? I have successfully built some packages on Linux and Darwin with this pull request, but I don't know how to test properly without hydra.

copumpkin · 2017-08-31T20:51:02Z

@orivej are you sure that's still true? I thought that was only true because nix-shell used the host bash, but now that we force it to use the Nix bash (and maybe coreutils?) we might be safer. If not, can we just force setup.sh to incorporate a fixed version of coreutils?

orivej · 2017-08-31T22:02:11Z

@copumpkin You are right, since 2bc7b4e Darwin always uses Bash from nixpkgs for setup.sh. However, over programs from bootstrap-tools (including env and awk) are not in PATH during early stages (when the shell is ${bootstrapTools}/bin/bash), e.g. env or awk 'BEGIN {}' at the beginning of setup.sh will cause failure, so it is not quite safe to use them at the moment… Currently bootstrap succeeds because early stages do not call substituteAll

Ericson2314 · 2017-08-31T23:21:21Z

err in nix-shell, the impure bash will be used---I've certainly learned that! If any dependency has a setup hook or similar using substitute, it will be run when the nix-shell is entered.

Ericson2314

With or without awk seems fine to me. Thanks for fixing!

Ericson2314 · 2017-08-31T23:24:06Z

pkgs/stdenv/generic/setup.sh

@@ -497,10 +497,8 @@ substitute() {
                shift 2
                # check if the used nix attribute name is a valid bash name
                if ! [[ "$varName" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]]; then
-                    echo "${FUNCNAME[0]}(): WARNING: substitution variables should be valid bash names," >&2


You could just hardcode substitute(): too. That is what was there before, IIRC.

I'm not familiar with FUNCNAME. Why did you use it 5d693c8?

No great reason, just in-case the code was moved elsewhere / general principle of things not knowing their own name.

If you want to replace ${FUNCNAME[0]} with substitute, I can do this in this pull request.

I don't care which, I'd just like to have something saying what the function is.

(I'll merge it after that.)

Ah, I did not get that you asked for the function name here too. Done.

Ericson2314 · 2017-09-01T01:41:06Z

pkgs/stdenv/generic/setup.sh

-                    echo "  \"$varName\" isn't and therefore was skipped; it might be caused" >&2
-                    echo "  by multi-line phases in variables - see #14907 for details." >&2
-                    continue
+                    echo "substitution variables must be valid Bash names, \"$varName\" isn't."


The >&2 should also still be there.

Thanks, didn't notice that.

NixOS#28806 (comment)

Environment variable filter in substituteAll was not precise and produced undefined and invalid variable names. Vladimír Čunát tried to fix that in [1], but `env -0` did not work during Darwin bootstrap, so [2] reverted this change and replaced an error due to invalid variables with a warning. Recently in NixOS#28057 John Ericson added `set -u` to `setup.sh` and undefined variables made the setup fail during e.g. `nix-build -A gnat` with `setup: line 519: !varName: unbound variable`. [1] NixOS@62fc885 [2] NixOS@81df035

Ericson2314 · 2017-09-03T14:26:46Z

Thanks @orivej!

orivej requested a review from Ericson2314 as a code owner August 31, 2017 17:42

orivej force-pushed the stdenv-setup branch 2 times, most recently from d0704a8 to bd51c02 Compare August 31, 2017 17:54

orivej force-pushed the stdenv-setup branch from 9b49417 to 3928ced Compare August 31, 2017 19:10

orivej force-pushed the stdenv-setup branch from 3928ced to 93a400e Compare August 31, 2017 20:04

Ericson2314 approved these changes Aug 31, 2017

View reviewed changes

Ericson2314 reviewed Sep 1, 2017

View reviewed changes

orivej force-pushed the stdenv-setup branch 2 times, most recently from f2cd568 to 16761a3 Compare September 1, 2017 09:59

orivej mentioned this pull request Sep 1, 2017

mkDerivation: fix hardening flags check #28806

Merged

orivej force-pushed the stdenv-setup branch from 67d5aa9 to 4b2b6ca Compare September 1, 2017 16:18

Ericson2314 added this to the 17.09 milestone Sep 1, 2017

Ericson2314 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 1, 2017

orivej force-pushed the stdenv-setup branch from 4b2b6ca to fecdfa0 Compare September 2, 2017 13:26

orivej added 3 commits September 3, 2017 12:57

mkDerivation: explain "all" in hardeningDisable

447240b

NixOS#28806 (comment)

stdenv-setup: list environment variables with awk

f4044c1

orivej force-pushed the stdenv-setup branch from fecdfa0 to f4044c1 Compare September 3, 2017 12:57

Ericson2314 merged commit b891d75 into NixOS:staging Sep 3, 2017

orivej deleted the stdenv-setup branch September 3, 2017 14:46

Ericson2314 added the 8.has: port to stable A PR already has a backport to the stable release. label Sep 3, 2017

jtojnar mentioned this pull request Nov 17, 2018

gnome3.gnome-desktop: fix path substitution #50507

Merged

9 tasks

samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019

orivej mentioned this pull request Oct 20, 2021

tonelib-gfx: 4.6.6 -> 4.7.0 #142259

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv-setup: fix substituteAll with set -eu #28799

stdenv-setup: fix substituteAll with set -eu #28799

orivej commented Aug 31, 2017 •

edited

Loading

mention-bot commented Aug 31, 2017

dezgeg commented Aug 31, 2017 •

edited

Loading

orivej commented Aug 31, 2017 •

edited

Loading

dezgeg commented Aug 31, 2017 •

edited

Loading

orivej commented Aug 31, 2017

copumpkin commented Aug 31, 2017

orivej commented Aug 31, 2017 •

edited

Loading

orivej commented Aug 31, 2017

copumpkin commented Aug 31, 2017

orivej commented Aug 31, 2017

Ericson2314 commented Aug 31, 2017

Ericson2314 left a comment

Ericson2314 Aug 31, 2017

orivej Aug 31, 2017

Ericson2314 Aug 31, 2017

orivej Sep 1, 2017

Ericson2314 Sep 1, 2017

Ericson2314 Sep 1, 2017

orivej Sep 1, 2017

Ericson2314 Sep 1, 2017

orivej Sep 1, 2017

Ericson2314 commented Sep 3, 2017

stdenv-setup: fix substituteAll with set -eu #28799

stdenv-setup: fix substituteAll with set -eu #28799

Conversation

orivej commented Aug 31, 2017 • edited Loading

Motivation for this change

Things done

mention-bot commented Aug 31, 2017

dezgeg commented Aug 31, 2017 • edited Loading

orivej commented Aug 31, 2017 • edited Loading

dezgeg commented Aug 31, 2017 • edited Loading

orivej commented Aug 31, 2017

copumpkin commented Aug 31, 2017

orivej commented Aug 31, 2017 • edited Loading

orivej commented Aug 31, 2017

copumpkin commented Aug 31, 2017

orivej commented Aug 31, 2017

Ericson2314 commented Aug 31, 2017

Ericson2314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Sep 3, 2017

orivej commented Aug 31, 2017 •

edited

Loading

dezgeg commented Aug 31, 2017 •

edited

Loading

orivej commented Aug 31, 2017 •

edited

Loading

dezgeg commented Aug 31, 2017 •

edited

Loading

orivej commented Aug 31, 2017 •

edited

Loading