stdenv-setup: use `set -u` #28057

Ericson2314 · 2017-08-09T05:25:57Z

Motivation for this change

For my cross work, I need to make some fairly in-depth changes to stdenv's setup. This, and other development, is var easier with set -u as a debugging aid.

Care is taken to set +u when running hooks so as to not break existing packages. Eventually, we might try to use set -u everywhere but that is a far larger project.

Things done

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

CC @orivej

mention-bot · 2017-08-09T05:25:59Z

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

Ericson2314 · 2017-08-09T05:30:19Z

pkgs/stdenv/generic/setup.sh

@@ -1,4 +1,4 @@
-set -e
+set -eux


Just did set -x while debugging.

Ericson2314 · 2017-08-09T05:30:46Z

pkgs/stdenv/generic/setup.sh

    local hookName="$1"
    shift
    local var="$hookName"
    if [[ "$hookName" =~ Hook$ ]]; then var+=s; else var+=Hooks; fi
+    local -a "var=(\${$var[@]})"


Yes, this abomination actually works.

I believe you can use

hooksRef="${hookName%Hook}Hooks[@]" hooks=("${!hooksRef}")

I did the trailing [@] trick before, but set -u doesn't like it. Bash is such a mess.

%Hookis cool though; I'll definitely use that!

It is surprising that bash is happy with option 1:

set -eu hooksVar=abcHooks hooksRef="${hooksVar}[@]" p() { # option 1 local -a hooks="(\"\${$hooksVar[@]}\")" # option 2 hooks=("${!hooksRef+${!hooksRef}}") echo "${#hooks[@]} item(s):" for item in "${hooks[@]}"; do printf ' %q\n' "$item" done } # not empty abcHooks=('a b' 'c d') p # empty item abcHooks=('') p # empty array abcHooks=() p # undefined unset abcHooks p

Ericson2314 · 2017-08-09T05:31:06Z

pkgs/stdenv/generic/setup.sh

        _eval "$hook" "$@"
    done
+
+    eval "$oldOpts"


Want to be robust no matter who calls

Ericson2314 · 2017-08-09T19:32:17Z

Ok after much wrestling, stdenv seems to be building sucessfully now on macOS and Linux.

Ericson2314 · 2017-08-09T19:33:11Z

pkgs/stdenv/generic/setup.sh

@@ -987,3 +1041,6 @@ runHook userHook


 dumpVars
+
+# Disable nounset for nix-shell.
+set +u


We should probably make nix-shell do this, like it does for set -e.

orivej · 2017-08-09T21:11:21Z

Sorry if this observation is not well informed, but why are set +u and set -u so distant from each other? Couldn't they just wrap pieces of code that need them, such that each set +u is obviously balanced with the next set -u?

Ericson2314 · 2017-08-09T21:24:56Z

@orivej I try to do that. The worst offender is because eval "$foo" can early return, so I have to set -u in the caller to fake the RIAA/try..finally.

Ericson2314 · 2017-08-09T21:32:42Z

@orivej BTW, I'd love to get rid of the redundancy between _eval and _callImplicitHook, or even better get rid of the latter altogether, but I couldn't come up with a way after much thinking about it in the morning.

Ericson2314 · 2017-08-10T15:05:18Z

@globin Good to go now

dezgeg · 2017-08-11T05:18:12Z

Not a fan. All this set +/-u flipping around looks really brittle, plus the constant :-} is getting to line noise territory...

globin · 2017-08-11T08:33:04Z

Breakage: https://hydra.mayflower.de/eval/1006?compare=hydra-jobs-production#tabs-now-fail

Older bash version, like those in the bootstrap tools and on macOS, currently confuse variables defined as an empty array with undefined variables. `${foo+"${foo[@]}"}` will prevent `set -u` problems with empty arrays and older without making a single '' in the empty case. Care is taken to `set +u` when running hooks so as to not break existing packages.

Ericson2314 · 2017-08-23T21:26:55Z

Rebased an rebuilt. I'm trying to clean up cc-wrapper's default.nix (as opposed to the wrappers themselves, which I already did), and this impedes that too---set -u is essential to making sure that as I move things around all variables are still defined before use.

Ericson2314 · 2017-08-25T05:40:39Z

See https://github.com/NixOS/nixpkgs/projects/8 for all my PRs that (directly or indirectly) depend on this. To unblock those, I hope we can merge this, and merge it soon.

Ericson2314 · 2017-08-25T15:13:01Z

OK I'm merging this now. I just need this to have a reasonable debugging environment to finish https://github.com/NixOS/nixpkgs/projects/8. After that project, if anyone insists this is change bad, I will personally revert. (It won't commute with other changes, so it's a somewhat less trivial revert I'm promising).

Ericson2314 · 2017-08-25T15:18:29Z

N.B. Checked each failure in https://hydra.mayflower.de/jobset/mayflower/hydra-jobs-production and confirmed it was unrelated.

dezgeg · 2017-08-26T23:17:58Z

Like, WTF is this new apparent "policy" that feedback from others in multiple pull requests can just be ignored just on the basis on "I am in a hurry" or "I need this"? We are using git after all....

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

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`. [1] 62fc885 [2] 81df035 (cherry picked from commit a09d9e7)

Ericson2314 · 2017-09-13T14:54:48Z

I am no longer trying to squeeze my cross work into 17.03, so there is no rush. I will continue to need this for cross work on master for 18.03, but I did merge it over other's objections. If someone wants to revert it until there is more consensus, I cannot complain.

Ericson2314 added 2.status: work-in-progress 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 9.needs: community feedback labels Aug 9, 2017

Ericson2314 requested review from cstrahan, dezgeg, globin and vcunat August 9, 2017 05:25

Ericson2314 removed the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Aug 9, 2017

Ericson2314 commented Aug 9, 2017

View reviewed changes

Ericson2314 changed the title ~~stdenv-setup: WIP use set -u~~ [WIP] stdenv-setup: use set -u Aug 9, 2017

Ericson2314 force-pushed the stdenv-set-u branch 5 times, most recently from 52566b7 to ab628e7 Compare August 9, 2017 19:18

Ericson2314 commented Aug 9, 2017

View reviewed changes

Ericson2314 force-pushed the stdenv-set-u branch 3 times, most recently from ab563c4 to 9fc7726 Compare August 9, 2017 22:32

Ericson2314 removed the 2.status: work-in-progress label Aug 10, 2017

Ericson2314 changed the title ~~[WIP] stdenv-setup: use set -u~~ stdenv-setup: use set -u Aug 10, 2017

globin added a commit to mayflower/hydra-jobs that referenced this pull request Aug 10, 2017

add temp jobset for NixOS/nixpkgs#28057

f2caa66

Ericson2314 added this to Needed by the big PR in Cross compilation Aug 16, 2017

Ericson2314 modified the milestone: 17.09 Aug 16, 2017

Ericson2314 force-pushed the stdenv-set-u branch from 8148360 to 81194ee Compare August 23, 2017 19:58

Ericson2314 moved this from Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top to Needed for binutils-wrapper in Cross compilation Aug 25, 2017

Ericson2314 mentioned this pull request Aug 25, 2017

cc-wrapper: Cleanup of Nix #28556

Merged

8 tasks

Ericson2314 requested a review from fpletz August 25, 2017 05:41

Ericson2314 merged commit 2e7a390 into NixOS:staging Aug 25, 2017

Ericson2314 deleted the stdenv-set-u branch August 25, 2017 15:20

orivej mentioned this pull request Aug 31, 2017

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

Merged

8 tasks

Ericson2314 mentioned this pull request Sep 13, 2017

Setting -u in stdenv to catch invalid variables #29296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv-setup: use `set -u` #28057

stdenv-setup: use `set -u` #28057

Ericson2314 commented Aug 9, 2017 •

edited

mention-bot commented Aug 9, 2017

Ericson2314 Aug 9, 2017

Ericson2314 Aug 9, 2017

orivej Aug 9, 2017

Ericson2314 Aug 9, 2017

orivej Aug 9, 2017

Ericson2314 Aug 9, 2017

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 Aug 9, 2017

orivej commented Aug 9, 2017

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 10, 2017

dezgeg commented Aug 11, 2017

globin commented Aug 11, 2017

Ericson2314 commented Aug 23, 2017 •

edited

Ericson2314 commented Aug 25, 2017 •

edited

Ericson2314 commented Aug 25, 2017 •

edited

Ericson2314 commented Aug 25, 2017

dezgeg commented Aug 26, 2017

Ericson2314 commented Sep 13, 2017

stdenv-setup: use set -u #28057

stdenv-setup: use set -u #28057

Conversation

Ericson2314 commented Aug 9, 2017 • edited

Motivation for this change

Things done

mention-bot commented Aug 9, 2017

Ericson2314 Aug 9, 2017

Choose a reason for hiding this comment

Ericson2314 Aug 9, 2017

Choose a reason for hiding this comment

orivej Aug 9, 2017

Choose a reason for hiding this comment

Ericson2314 Aug 9, 2017

Choose a reason for hiding this comment

orivej Aug 9, 2017

Choose a reason for hiding this comment

Ericson2314 Aug 9, 2017

Choose a reason for hiding this comment

Ericson2314 commented Aug 9, 2017 • edited

Ericson2314 Aug 9, 2017

Choose a reason for hiding this comment

orivej commented Aug 9, 2017

Ericson2314 commented Aug 9, 2017 • edited

Ericson2314 commented Aug 9, 2017 • edited

Ericson2314 commented Aug 10, 2017

dezgeg commented Aug 11, 2017

globin commented Aug 11, 2017

Ericson2314 commented Aug 23, 2017 • edited

Ericson2314 commented Aug 25, 2017 • edited

Ericson2314 commented Aug 25, 2017 • edited

Ericson2314 commented Aug 25, 2017

dezgeg commented Aug 26, 2017

Ericson2314 commented Sep 13, 2017

stdenv-setup: use `set -u` #28057

stdenv-setup: use `set -u` #28057

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 9, 2017 •

edited

Ericson2314 commented Aug 23, 2017 •

edited

Ericson2314 commented Aug 25, 2017 •

edited

Ericson2314 commented Aug 25, 2017 •

edited