dev.sh fixups #2114
Merged
dev.sh fixups #2114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of switching between windows and non-windows to determine how to handle slashes for msbuild, use dashes instead of slashes to simplify the calling.
Stop on errors, instead of continuing. This prevents us from failing to move through the directory space with `cd` / `pushd` / `popd` but still running commands. This is particularly dangerous when running commands like `rm`.
Since directories may have a space in them, quote them to treat them as a single entity instead of wordsplitting on a space. Otherwise, if `FOO="a b c"` then `rm -rf $FOO` will remove files or folders named `a`, b`, and `c` instead of removing the single entity named `a b c`.
When expanding variables to pass to `rm`, make sure that the path variable is set and fail if it is not. This prevents an `rm` from accidentally expanding to `rm ""/*` when the variable is unset: ValveSoftware/steam-for-linux#3671 Using `${FOO:?}` syntax will fail when `FOO` is unset.
`$LastExitCode` is not a bash variable; to pass that string along to PowerShell, it needs to be quoted.
The $(cmd) execution syntax is preferred over the legacy backtick syntax.
Quote the variables for the directories so that we can properly work with directories with spaces in their names.
|
@ethomson Thanks! 🥇 |
|
Awesome, thanks @TingluoHuang! 😀 |
gavtroy
reviewed
Mar 6, 2019
| find "${LAYOUT_DIR}/bin" -type f -name '*.pdb' -delete | ||
|
|
||
| mkdir -p "$PACKAGE_DIR" | ||
| rm -Rf "${PACKAGE_DIR:?}/*" |
There was a problem hiding this comment.
👍 good catch, you want to create a PR? :)
There was a problem hiding this comment.
Thanks for the encouragement, but I'm afraid I care only enough to drop a drive-by comment. You can decide if it's actually worth fixing. :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I cloned the
azure-pipelines-agentto a directory name with a space in it and after doing so,dev.shcommands did not work; the bulk of these commits deal with that issue.In particular, this starts quoting all variable strings to ensure that they're not wordsplit on spaces. Otherwise:
will remove both
/home/ethomson/fooandbar asdf. Quoting asrm -rf "$DIRECTORY"will treat the entire path (with a space in it) as a single string, as expected.This also adds some safety around
rms to avoid accidentally coping with empty strings, similar to the problem that happened in ValveSoftware/steam-for-linux#3671. Otherwise, acdinto a directory followed by anrmbecomes dangerous if thecdfailed.As a result, this enables
set -eto avoid continuing when acdorpushd(or any command) fails. (As would happen with an unquoted space in the path, or to a directory that doesn't exist.)Similarly, using
${FOO:?}/*will ensure that ifFOOis unset that the command fails instead of trying torm /*.After that I just ran shellcheck against the scripts and fixed up the remainder of warnings.