-
Notifications
You must be signed in to change notification settings - Fork 708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
style: address shellcheck warnings #1993
base: main
Are you sure you want to change the base?
style: address shellcheck warnings #1993
Conversation
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
28e0c9e
to
896ea2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It would be great if somebody who is more familiar with shell scripting could review this. There are some things that seems strange to me. I'll point out a couple.
mk/cargo.sh
Outdated
@@ -251,16 +251,16 @@ cargo "$@" | |||
if [ -n "${RING_COVERAGE-}" ]; then | |||
# Keep in sync with check-symbol-prefixes.sh. | |||
# Use the host target-libdir, not the target target-libdir. | |||
llvm_root="$(rustc +${toolchain} --print target-libdir)/../bin" | |||
llvm_root="$(rustc +"${toolchain}" --print target-libdir)/../bin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird. I wonder if there's a better way of doing it. Maybe removing the "outermost"-looking double quotes, similar to the target_upper=...
line above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having outer quotes is fine because $() starts a new context (see SC2086) where the quoting is processed independently, so no escaping is required. However for consistency, I'll remove the outer quotes as it has not effect on the variable assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by 9a18e6f
mk/cargo.sh
Outdated
basename=$(basename "$executable") | ||
${llvm_root}/llvm-profdata merge -sparse "$coverage_dir/$basename.profraw" -o "$coverage_dir/$basename.profdata" | ||
"${llvm_root}"/llvm-profdata merge -sparse "$coverage_dir/$basename.profraw" -o "$coverage_dir/$basename.profdata" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks weird to be inconsistent with having the double quotes only around the variable usage in "${llvm_root}"/llvm-profdata
but then less tight in "$coverage_dir/$basename.profraw"
and "$coverage_dir/$basename.profdata"
. Maybe I'm missing something, but some more consistency would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by 31dc1e6.
Though I was thinking about normalizing the usage of ${example}
vs $example
using the following rules:
-
Use
$example
when the variable is not being without additional characters or variable modifiers (e.g.${example:-default_value}
value="$example" if [ "$example" = "other" ]; then echo matches; fi
-
Use
${example}
otherwise, e.g.+${example}
,my/path/${example}
to make it more (visually) obvious that a string is being built from mixing strings with variables.value="+${value}" value2="${value:-default}" "${bin}/path/myexec" run
@briansmith Are you ok with this?
Addressing the warnings raised by shellcheck.
Using shellcheck (slightly) improves the style consistency and reduces the chance of subtle errors.
There are still two shellcheck warnings for missing shebangs (in
mk/package.sh
andmk/publish.sh
, however looking at the scripts it seems these might be for windows environments running MinGW, where shebangs might be incompatible.