Stdenv setup misc cleanups #27336

Ericson2314 · 2017-07-12T18:30:42Z

Motivation for this change

@grahamc showed me shellcheck, which I am now quite fond of. In order to tightening things up so #26805 is easier to debug, I did these clean ups.

I left the # shellcheck disable=... in there in hopes we can eventually make it a test. A lot of this quoting doesn't by us much since other things block store paths with whitespace, but shellcheck can also make switching toset -u feasible, which is definitely worthy of static analysis.

This is easiest to review commit-by-commit:

Use associative arrays to accumulate deps. Then instead of doing huge pattern match just check the key, and do get out deps iterator through keys.
Shell check
Few style + error handling things on substitue

Things done

Rebuilt Linux and Darwin stdenvs.

copumpkin · 2017-07-12T18:39:44Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -211,7 +211,7 @@ stdenv.mkDerivation ({
    configureFlags="${concatStringsSep " " defaultConfigureFlags} $configureFlags"

    # nativePkgs defined in stdenv/setup.hs
-    for p in $nativePkgs; do
+    for p in "${!nativePkgs[@]}"; do


Does this work? Isn't the ${} escaping into nix?

~~Weird you would think, but I did build it.~~ ah building nevermind building stdenv wouldn't catch that. Thanks!

…d of strings This is generally cleaner: less eval, less worrying about separators, and probably also faster. I got the idea from that python wrapper script.

@grahamc

I took some liberties with the flags-echoing code to make it more concise and correct. Also, a few warnings in findInputs and friends I skipped because I am going to rewrite those anyways. Thanks @grahamc for telling me about this great linter!

joachifm · 2017-07-12T19:38:13Z

pkgs/stdenv/generic/setup.sh

-    local input="$1"
-    local output="$2"
+    local input=$1; shift
+    local output=$1; shift


shift also takes a numeric argument, so you can do e.g., local foo=$1 bar=$2; shift 2 to achieve the same effect.

I short of like the "one variable, one shift" correspondence, but I'd be happy to change it.

Looks a bit odd to me, but it's entirely superficial :)

Ah I can avoid binding p doing this. Swayed :).

It now blows up on null byte in file (rather than silently truncating), and invalid arguments (rather than silently skipping).

edolstra · 2017-07-13T18:35:55Z

pkgs/stdenv/generic/setup.sh

-    local input="$1"
-    local output="$2"
+    local input=$1
+    local output=$2


Why no quotes here?

Bash parses it the same either way! I learned this and was quite surprised.

I could leave as is, but I figured given it didn't matter I should pick one and be consistent. I could normalize the other way if you like.

Copying from the bash because it's really quite unbelievable for me still:

$ set -x; x='asdf asdf'; y=$x; z="$x" + set -x + x='asdf asdf' + y='asdf asdf' + z='asdf asdf'

This is covered under PARAMETERS in the bash manual. Word splitting & pathname expansion is not performed on assignment.

I don't find this an improvement, it's just forces people to learn another special case in bash syntax instead of using the obviously safe variant.

Agree with @dezgeg here. Shell quoting is such a pain and source of subtle bugs already; although it may be technically correct, why not be explicit when we can do so trivially? This particular gun might have a fancy high tech anti-foot-shot protection device in it, but I'd rather just point it away from my feet.

Uh, I hate how the bash language is so complex.

I basically got bash Stockholm syndrome writing this stuff. Happy to quote everything for consistency.

edolstra · 2017-07-13T18:40:09Z

pkgs/stdenv/generic/setup.sh

@@ -727,17 +742,23 @@ configurePhase() {
 buildPhase() {
    runHook preBuild

-    if [ -z "$makeFlags" ] && ! [ -n "$makefile" -o -e "Makefile" -o -e "makefile" -o -e "GNUmakefile" ]; then
+    if [ -z "$makeFlags" ] && ! [[ -n "$makefile" || -e "Makefile" || -e "makefile" || -e "GNUmakefile" ]]; then


Can't this be written as a single [[...]] test? Something like

if [[ -z $makeFlags && ! ( -n $makefile || ... ) ]]; then

Note that quotes are not necessary inside [[...]].

Yes it can. Will do.

edolstra · 2017-07-13T18:43:09Z

Looks good to me!

edolstra · 2017-07-13T18:48:09Z

pkgs/stdenv/generic/setup.sh

-    eval $var="'${!var} $pkg '"
+    # Stop if we've already added this one
+    [[ -z "${varDeref["$pkg"]}" ]] || return 0
+    varDeref["$pkg"]=1


This is great because it fixes the O(n^2) performance setting up the environment!

Also remove useless quotes on same line

foo=$1 surprisingly doesn't need quotes in Bash. Word splits are only syntactic in string variable (not array var!) assignments.

Ericson2314 · 2017-07-13T19:03:40Z

Doing another build with those changes, and then will merge.

dezgeg · 2017-07-13T19:34:00Z

pkgs/stdenv/generic/setup.sh

-        $installFlags "${installFlagsArray[@]}"
+        $installFlags "${installFlagsArray[@]}")
+
+    printf 'install flags:'


Why is this multiple commands?

@dezgeg it hash to be:

print the name of the flags

repeat the format string (unquoted space + quoted arg) for each argument

close the line

printf repeats the format string until all the args are gone, so it is essential the repeated format string contain one a single substitution.

I could add a helper function and comments for this :).

dezgeg · 2017-07-13T20:01:18Z

pkgs/stdenv/generic/setup.sh

+    local content
+    # read returns non-0 on EOF, so we want read to fail
+    if IFS='' read -r -N 0 content < "$input"; then
+        echo "${FUNCNAME[0]}(): ERROR: File \"$input\" has null bytes, won't process" >&2


substitute is documented to work on binaries in the manual:

It supports performing substitutions on binary files (such as executables), though there you’ll probably want to make sure that the replacement string is as long as the replaced string.

@dezgeg Woah. Well the manual is definitely wrong about that. We can use mapfile and carefully printf the missing null bytes, but I think that should be a followup PR.

@dezgeg

@dezgeg made the good point that the reasons for doing this were not at all intuitive.

Ericson2314 requested review from grahamc, dezgeg and edolstra July 12, 2017 18:30

copumpkin reviewed Jul 12, 2017

View reviewed changes

Ericson2314 added 2 commits July 12, 2017 15:30

stdenv-setup: Make the package accumulators associative arrays instea…

8d76eff

…d of strings This is generally cleaner: less eval, less worrying about separators, and probably also faster. I got the idea from that python wrapper script.

Ericson2314 force-pushed the stdenv-setup-cleanup branch from 02ff8c1 to 2087c3e Compare July 12, 2017 19:31

joachifm reviewed Jul 12, 2017

View reviewed changes

Ericson2314 force-pushed the stdenv-setup-cleanup branch from 2087c3e to 959801d Compare July 12, 2017 19:50

stdenv-setup: Clean up 'substitute()' for style and error handling

5d693c8

It now blows up on null byte in file (rather than silently truncating), and invalid arguments (rather than silently skipping).

Ericson2314 force-pushed the stdenv-setup-cleanup branch from 959801d to 5d693c8 Compare July 12, 2017 21:49

edolstra reviewed Jul 13, 2017

View reviewed changes

Ericson2314 added 2 commits July 13, 2017 14:57

stdenv-setup: Combine [[ .. ]] && [[ .. ]] into one [[ .. && .. ]]

273a4c1

Also remove useless quotes on same line

stdenv-setup: Remove useless quotes

2743078

foo=$1 surprisingly doesn't need quotes in Bash. Word splits are only syntactic in string variable (not array var!) assignments.

dezgeg reviewed Jul 13, 2017

View reviewed changes

stdenv-setup: Pull out and explain 3-part printing of commands

30a1420

@dezgeg made the good point that the reasons for doing this were not at all intuitive.

Ericson2314 merged commit ea9af6e into NixOS:staging Jul 13, 2017

Ericson2314 deleted the stdenv-setup-cleanup branch July 13, 2017 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stdenv setup misc cleanups #27336

Stdenv setup misc cleanups #27336

Ericson2314 commented Jul 12, 2017 •

edited

Loading

copumpkin Jul 12, 2017

Ericson2314 Jul 12, 2017 •

edited

Loading

joachifm Jul 12, 2017

Ericson2314 Jul 12, 2017

joachifm Jul 12, 2017

Ericson2314 Jul 12, 2017

edolstra Jul 13, 2017

Ericson2314 Jul 13, 2017 •

edited

Loading

Ericson2314 Jul 13, 2017

Ericson2314 Jul 13, 2017 •

edited

Loading

joachifm Jul 13, 2017

dezgeg Jul 13, 2017

copumpkin Jul 13, 2017

vcunat Jul 14, 2017

Ericson2314 Jul 15, 2017

edolstra Jul 13, 2017

Ericson2314 Jul 13, 2017

edolstra commented Jul 13, 2017

edolstra Jul 13, 2017

Ericson2314 commented Jul 13, 2017

dezgeg Jul 13, 2017

Ericson2314 Jul 13, 2017

Ericson2314 Jul 13, 2017

dezgeg Jul 13, 2017

dezgeg Jul 13, 2017

Ericson2314 Jul 13, 2017

Stdenv setup misc cleanups #27336

Stdenv setup misc cleanups #27336

Conversation

Ericson2314 commented Jul 12, 2017 • edited Loading

Motivation for this change

Things done

Choose a reason for hiding this comment

Ericson2314 Jul 12, 2017 • edited Loading

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 Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Jul 13, 2017 • edited Loading

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

edolstra commented Jul 13, 2017

Choose a reason for hiding this comment

Ericson2314 commented Jul 13, 2017

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 Jul 12, 2017 •

edited

Loading

Ericson2314 Jul 12, 2017 •

edited

Loading

Ericson2314 Jul 13, 2017 •

edited

Loading

Ericson2314 Jul 13, 2017 •

edited

Loading