Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

reubenmiller
Copy link

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 and mk/publish.sh, however looking at the scripts it seems these might be for windows environments running MinGW, where shebangs might be incompatible.

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.
@reubenmiller reubenmiller force-pushed the style-resolve-shellcheck-warnings branch from 28e0c9e to 896ea2f Compare March 13, 2024 22:08
Copy link
Owner

@briansmith briansmith left a 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"
Copy link
Owner

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?

Copy link
Author

@reubenmiller reubenmiller May 14, 2024

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.

Copy link
Author

@reubenmiller reubenmiller May 14, 2024

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"
Copy link
Owner

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.

Copy link
Author

@reubenmiller reubenmiller May 14, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants