Source (.) init-tools.sh rather than executing it #240
Conversation
build.sh
Outdated
| "$SCRIPT_ROOT/init-tools.sh" | ||
| ( | ||
| # init-tools.sh isn't compatible with these flags. Unset them in this subshell. | ||
| set +euo pipefail |
There was a problem hiding this comment.
CI failed and I had to add this workaround, still look ok @weshaggard?
There was a problem hiding this comment.
Can we fix init-tools so it works with this option? What was the failure?
There was a problem hiding this comment.
The first error is:
init-tools.sh: line 9: __BUILDTOOLS_SOURCE: unbound variable
I'll give fixing it a try (waiting for a different build to finish).
There was a problem hiding this comment.
I wonder if we should just remove the "u" option in the root script.
There was a problem hiding this comment.
Personally, I really like how -u forces scripts to be clear if they expect the variable to be set at that point in time, and I think it's worth keeping. I haven't done a ton of work with it yet, though.
build.sh
Outdated
| export NUGET_PACKAGES="$SCRIPT_ROOT/packages/" | ||
|
|
||
| "$SCRIPT_ROOT/init-tools.sh" | ||
| . "$SCRIPT_ROOT/init-tools.sh" |
There was a problem hiding this comment.
Does it work to use the actual source "$DIR/common/_prettyprint.sh" command? That seems more obvious to me when reading the code than trying to see that little ..
There was a problem hiding this comment.
source isn't part of POSIX, so . is in theory more portable.
There was a problem hiding this comment.
We have #!/bin/bash at the top...
There was a problem hiding this comment.
Hmm, more real than I thought: dash apparently doesn't support source. https://stackoverflow.com/a/13702876 https://wiki.ubuntu.com/DashAsBinSh#source
There was a problem hiding this comment.
(Whoops, didn't refresh and missed your comment.)
You're right that we do have that direct dependency on bash right now, and it would be ok to use source. A while back someone pointed out some bashisms in BuildTools and I think CoreFX. I've pointed out obvious ones like [[ since then (in PRs), but I don't know if there's a higher-level drive to avoid a bash dependency.
There was a problem hiding this comment.
Right now we are definitely dependent on bash.
There was a problem hiding this comment.
I'll go ahead and change it to source then--the readability concern also came up in the CoreFX PR. It's not like this would be hard to spot/fix if we do want to work on non-bash.
3 participants
Fixes #239