Skip to content

Commit

Permalink
setup.sh: shellcheck 2206 + 2207
Browse files Browse the repository at this point in the history 
https://github.com/koalaman/shellcheck/wiki/SC2206
https://github.com/koalaman/shellcheck/wiki/SC2207

admittedly this one is a lot less pretty
  • Loading branch information
@happysalada
happysalada committed Jul 6, 2021
1 parent 430fdb7 commit c335a18
Showing 1 changed file with 50 additions and 38 deletions.
88 changes: 50 additions & 38 deletions pkgs/stdenv/generic/setup.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,14 @@ exitHandler() {

if [ -n "${showBuildStats:-}" ]; then
times > "$NIX_BUILD_TOP/.times"
local -a times=($(cat "$NIX_BUILD_TOP/.times"))
local -a buildTimesArray
IFS=" " read -r -a buildTimesArray <<< "$(cat "$NIX_BUILD_TOP/.times")"
# Print the following statistics:
# - user time for the shell
# - system time for the shell
# - user time for all child processes
# - system time for all child processes
echo "build time elapsed: " "${times[@]}"
echo "build time elapsed: " "${buildTimesArray[@]}"
fi

if (( "$exitCode" != 0 )); then
Expand Down Expand Up @@ -1002,10 +1003,10 @@ configurePhase() {
fi

if [ -n "$configureScript" ]; then
# Old bash empty array hack
# shellcheck disable=SC2086
local flagsArray=(
$configureFlags ${configureFlagsArray+"${configureFlagsArray[@]}"}
IFS=" " read -r -a configureFlagsTemp <<< "$configureFlags"
local -a flagsArray=(
"${configureFlagsTemp[@]}"
${configureFlagsArray+"${configureFlagsArray[@]}"}
)
echoCmd 'configure flags' "${flagsArray[@]}"
# shellcheck disable=SC2086
Expand All @@ -1029,14 +1030,15 @@ buildPhase() {
echo "no Makefile, doing nothing"
else
foundMakefile=1

# Old bash empty array hack
# shellcheck disable=SC2086
local flagsArray=(
IFS=" " read -r -a makeFlagsTemp <<< "$makeFlags"
IFS=" " read -r -a buildFlagsTemp <<< "$buildFlags"
local -a flagsArray=(
${enableParallelBuilding:+-j${NIX_BUILD_CORES} -l${NIX_BUILD_CORES}}
SHELL=$SHELL
$makeFlags ${makeFlagsArray+"${makeFlagsArray[@]}"}
$buildFlags ${buildFlagsArray+"${buildFlagsArray[@]}"}
SHELL="$SHELL"
"${makeFlagsTemp[@]}"
${makeFlagsArray+"${makeFlagsArray[@]}"}
"${buildFlagsTemp[@]}"
${buildFlagsArray+"${buildFlagsArray[@]}"}
)

echoCmd 'build flags' "${flagsArray[@]}"
Expand Down Expand Up @@ -1069,14 +1071,17 @@ checkPhase() {
if [[ -z "${checkTarget:-}" ]]; then
echo "no check/test target in ${makefile:-Makefile}, doing nothing"
else
# Old bash empty array hack
# shellcheck disable=SC2086
local flagsArray=(
IFS=" " read -r -a makeFlagsTemp <<< "$makeFlags"
IFS=" " read -r -a checkFlagsTemp <<< "${checkFlags:-VERBOSE=y}"
IFS=" " read -r -a checkTargetTemp <<< "${checkTarget}"
local -a flagsArray=(
${enableParallelChecking:+-j${NIX_BUILD_CORES} -l${NIX_BUILD_CORES}}
SHELL=$SHELL
$makeFlags ${makeFlagsArray+"${makeFlagsArray[@]}"}
${checkFlags:-VERBOSE=y} ${checkFlagsArray+"${checkFlagsArray[@]}"}
${checkTarget}
SHELL="$SHELL"
"${makeFlagsTemp[@]}"
${makeFlagsArray+"${makeFlagsArray[@]}"}
"${checkFlagsTemp[@]}"
${checkFlagsArray+"${checkFlagsArray[@]}"}
"${checkTargetTemp[@]}"
)

echoCmd 'check flags' "${flagsArray[@]}"
Expand All @@ -1095,14 +1100,16 @@ installPhase() {
if [ -n "$prefix" ]; then
mkdir -p "$prefix"
fi

# Old bash empty array hack
# shellcheck disable=SC2086
local flagsArray=(
SHELL=$SHELL
$makeFlags ${makeFlagsArray+"${makeFlagsArray[@]}"}
$installFlags ${installFlagsArray+"${installFlagsArray[@]}"}
${installTargets:-install}
IFS=" " read -r -a makeFlagsTemp <<< "$makeFlags"
IFS=" " read -r -a installFlagsTemp <<< "$installFlags"
IFS=" " read -r -a installTargetsTemp <<< "${installTargets:-install}"
local -a flagsArray=(
SHELL="$SHELL"
"${makeFlagsTemp[@]}"
${makeFlagsArray+"${makeFlagsArray[@]}"}
"${installFlagsTemp[@]}"
${installFlagsArray+"${installFlagsArray[@]}"}
"${installTargetsTemp[@]}"
)

echoCmd 'install flags' "${flagsArray[@]}"
Expand Down Expand Up @@ -1203,14 +1210,17 @@ installCheckPhase() {
&& ! make -n ${makefile:+-f $makefile} ${installCheckTarget:-installcheck} >/dev/null 2>&1; then
echo "no installcheck target in ${makefile:-Makefile}, doing nothing"
else
# Old bash empty array hack
# shellcheck disable=SC2086
local flagsArray=(
IFS=" " read -r -a makeFlagsTemp <<< "$makeFlags"
IFS=" " read -r -a installCheckFlagsTemp <<< "$installCheckFlags"
IFS=" " read -r -a installCheckTargetTemp <<< "${installCheckTarget:-installcheck}"
local -a flagsArray=(
${enableParallelChecking:+-j${NIX_BUILD_CORES} -l${NIX_BUILD_CORES}}
SHELL=$SHELL
$makeFlags ${makeFlagsArray+"${makeFlagsArray[@]}"}
$installCheckFlags ${installCheckFlagsArray+"${installCheckFlagsArray[@]}"}
${installCheckTarget:-installcheck}
SHELL="$SHELL"
"${makeFlagsTemp[@]}"
${makeFlagsArray+"${makeFlagsArray[@]}"}
"${installCheckFlagsTemp[@]}"
${installCheckFlagsArray+"${installCheckFlagsArray[@]}"}
"${installCheckTargetTemp[@]}"
)

echoCmd 'installcheck flags' "${flagsArray[@]}"
Expand All @@ -1225,10 +1235,12 @@ installCheckPhase() {
distPhase() {
runHook preDist

# Old bash empty array hack
# shellcheck disable=SC2086
IFS=" " read -r -a distFlagsTemp <<< "$distFlags"

local flagsArray=(
$distFlags ${distFlagsArray+"${distFlagsArray[@]}"} ${distTarget:-dist}
"${distFlagsTemp[@]}"
${distFlagsArray+"${distFlagsArray[@]}"}
${distTarget:-dist}
)

echo 'dist flags: %q' "${flagsArray[@]}"
Expand Down

4 comments on commit c335a18

@vcunat
Copy link
Member

@vcunat vcunat commented on c335a18 Jul 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a practical problem (bisected). Currently some packages use *Flags with newlines inside them and those got silently broken by this commit. Example: 6a10c72. I think nowadays we can migrate packages to syntax like d7427b0, but

  1. I still don't really like the breakage, and
  2. the silent failure is bad; it should either work or complain that *Flags were set incorrectly.

@happysalada
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for going through this.
So if I understand correctly, there should be an additional check in the flags array to see if they include a newline and then print an error message and an exit.
Got it.
Since making a PR with several commits generated quite some complications, I'll try to make a PR with just one commit for this issue.
I'll add it on the list of things to do.

@vcunat
Copy link
Member

@vcunat vcunat commented on c335a18 Jul 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I'm not sure if there's some good use case to have the newlines in there. It's hard to see all cases, because there were many breaking changes in this staging iteration and there's always lots of "random failures" mixed in as well (e.g. flaky tests or builder getting overloaded and timing out).

@happysalada
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't imagine newlines at the time.
I'll remake a PR and put this in a separate commit. I can do a non-breaking change by making the split on space and newline explicit.
Then perhaps later we can try removing the newline and see which packages break.
Thank you for the insights and sorry for the troubles!

Please sign in to comment.