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

Fix shellcheck failures #70

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

pniedzielski
Copy link
Collaborator

@pniedzielski pniedzielski commented Aug 4, 2023

Issue number of the reported bug or feature request: #66

Closes #66.

Describe your changes
The incorporation of ShellCheck linting into our CI found some quoting bugs in our shell scripts. This patchset fixes these issues.

Testing performed
Both build-darwin.sh and bmqbrkr/run were tested. I am unable to test build-ubuntu.sh without access to an Ubuntu machine.

Additional context
This PR is a draft, because it will change after the incorporation of #65 and #67.

@pniedzielski pniedzielski requested review from jll63 and kaikulimu August 4, 2023 18:52
kaikulimu
kaikulimu previously approved these changes Aug 4, 2023
jll63
jll63 previously approved these changes Aug 15, 2023
@hallfox hallfox marked this pull request as ready for review August 17, 2023 19:56
@hallfox hallfox requested a review from a team as a code owner August 17, 2023 19:56
This patch fixes the warnings that our ShellCheck linter catches on
the `build-darwin.sh` file.  Most of these changes are about quoting
expanded variables, which in our case is generally good: for example,
we want to correctly handle the case of a user’s build directory
having a space in its full path.  In the case of `CMAKE_OPTIONS`, we
do this by changing the string variable into an array of strings we
can correctly quote, both when creating the array and when expanding
it as arguments.

Signed-off-by: Patrick M. Niedzielski <patrick@pniedzielski.net>
This patch fixes the warnings that our ShellCheck linter catches on
the `build-ubuntu.sh` file.  As with the previous patch, the majority
of these changes are about quoting expanded variables.  Here
additionally, we are using a HereDoc as multiline comment for the user
to copy and paste, so we disable a linter warning `SC2188` about
[redirection missing a command][sc2188] for this line.

[sc2188]: https://www.shellcheck.net/wiki/SC2188

Signed-off-by: Patrick M. Niedzielski <patrick@pniedzielski.net>
@pniedzielski pniedzielski dismissed stale reviews from jll63 and kaikulimu via 16bcff6 August 17, 2023 20:12
@pniedzielski pniedzielski force-pushed the fix-shellcheck-failures branch from cba4448 to 16bcff6 Compare August 17, 2023 20:12
@pniedzielski pniedzielski merged commit f4a4f55 into bloomberg:main Aug 17, 2023
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.

Fix ShellCheck failures
5 participants