[AOMP] Shellcheck fixes #1252
Merged
[AOMP] Shellcheck fixes #1252
Conversation
This file contains hidden or 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
jtb20
requested review from
estewart08,
gregrodgers and
ronlieb
as code owners
jtb20
force-pushed
the
shellcheck-fixes-8
branch
3 times, most recently
from
3dac11f to
8a77cce
Compare
Contributor
Author
|
I've fixed various issues with these patches, and the result now completes a "build_aomp.sh" run. |
jtb20
force-pushed
the
shellcheck-fixes-8
branch
2 times, most recently
from
0d3409a to
a2c7ecc
Compare
Contributor
Author
|
This version's rebased, and has had a couple of minor tweaks (e.g. around argument quoting). It still mostly covers only the top-level build*.sh scripts. |
estewart08
approved these changes
May 16, 2025
Contributor
estewart08
left a comment
estewart08
left a comment
There was a problem hiding this comment.
Do you plan on squashing this?
dpalermo
requested changes
May 16, 2025
Contributor
dpalermo
left a comment
dpalermo
left a comment
There was a problem hiding this comment.
Don't land this today (Friday). There are a lot of scripts changed here that will affect things beyond build_aomp.sh (e.g. nightly CI). I would also like to try this out a bit in a sandbox before letting the nightlies run with these changes. I will remove the change request and give approval when I have a few tests run with this (and it isn't Friday ;-).
Contributor
|
dont land this till after 21.0-1 is released and we move development to 21.0-2. I will land this myself after I study this some more. |
jtb20
force-pushed
the
shellcheck-fixes-8
branch
2 times, most recently
from
3a3871f to
9f6d775
Compare
Member
|
Any plans to add a .shellcheckrc file also? |
dpalermo
requested changes
Jun 2, 2025
Contributor
dpalermo
left a comment
dpalermo
left a comment
There was a problem hiding this comment.
Patch does not currently apply cleanly to amd-staging. Please rebase and I will retry these changes with the nightly build.
Contributor
|
[AMD Official Use Only - AMD Internal Distribution Only]
Branch name should be aomp-dev
From: dpalermo ***@***.***>
Sent: Monday, June 2, 2025 11:51 AM
To: ROCm/aomp ***@***.***>
Cc: Lieberman, Ron ***@***.***>; Review requested ***@***.***>
Subject: Re: [ROCm/aomp] [AOMP] Shellcheck fixes (PR #1252)
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
@dpalermo requested changes on this pull request.
Patch does not currently apply cleanly to amd-staging. Please rebase and I will retry these changes with the nightly build.
—
Reply to this email directly, view it on GitHub<#1252 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD3EYZ2VIB6YZ2NZRYSLSC33BRXH3AVCNFSM6AAAAABXOZXOEWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQOBZGAZTSMRYHA>.
You are receiving this because your review was requested.Message ID: ***@***.******@***.***>>
|
Contributor
|
Sorry wrong branch in previous comment. Build was amd-staging, but yes aomp was aomp-dev. Still need this patch rebased before I can retest. |
Factor out helper function to check if installdir is writable.
Add a helper functions to quote a list of arguments so it can be safely passed to cmake as a single string argument (cmquot), or to be reinterpreted by the shell as a string (shquot).
Use the new writable_install_dir function instead of duplicating the check in each script. Further factoring is possible here for the symlink check, but that's not done yet.
This patch uses $(...) syntax instead of `...` syntax, which is considered easier to read and can be nested properly. Some additional quotation is also introduced here (to prevent word splitting and globbing).
Parsing 'ls' output is considered fragile, and should be avoided in shellscripts. This patch adds a function to find the most recent version of FileCheck in a particular directory, and uses it in rocm_quick_check.sh.
This patch fixes a typo and uses correct quotation for the AOMP_NINJA_BIN variable in aomp_common_vars.
This patch fixes a typo in the shebang line for build-deb-aomp.sh.
This patch removes some variables which are set but not used in various build scripts.
This patch uses "test -n" in several places instead of "test ! -z".
This patch introduces the "cd ... || exit" (and pushd/popd, etc.) idiom, to avoid situations where changing directory may fail but the script carries on executing regardless.
Bare shell variable references are subject to word splitting and globbing, which can have undesirable side effects. This patch adds quotation in "simple" cases throughout the build scripts.
The 'find | xargs' invocation in build_aomp.sh is unsafe in some degenerate cases. This patch uses find's '-exec' option instead.
This patch changes quoting for *_RPATH variables to be consistent with the quoting above. This also stops shellcheck complaining about non-expansion of variables in single quotes.
This patch avoids use of $? in many places throughout the build scripts, preferring 'if cmd' syntax. See https://www.shellcheck.net/wiki/SC2181 for rationale.
This patch uses arrays instead of strings for "cmake" options (and a few other things) throughout the build scripts. This makes quotation easier when options are themselves lists of options (e.g. CFLAGS), though necessitates the use of bash's slightly unwieldy array syntax in a few places.
This patch groups echos in build-deb-aomp.sh, to avoid repeatedly appending to files.
This patch exports the 'log' variable in run_omptests.sh so it can be seen by check_omptests.sh. Otherwise the latter cannot access the variable, and sees it as uninitialised.
This patch avoids parsing 'ls' output in build_supp.sh, check_omptests.sh, create_release_tarball.sh and get_trunk_pr_since.
This patch fixes several cases where 'cat' is used unnecessarily, rather than using an input redirection.
This patch moves some variable initialisations higher in build-rpm.sh to avoid uninitialised variable references in the intervening code.
Several build scripts set LDFLAGS but do not export it, so the new value will not be seen by spawned processes. It turns out that exporting LDFLAGS causes build failures, so this patch comments out that code for now, pending further investigation.
This patch fixes edit_installed_hip_file to use a function argument instead of a global to pass its parameter.
This patch uses regexps instead of 'cut' and 'eval' in the clone_test.sh, clone_aomp.sh and create_release_tarball.sh scripts.
This patch fixes some quotation issues in get_trunk_pr_since.
This patch uses [ ... ] && [ ... ] instead of [ ... && ... ], since the latter does not appear to be valid syntax.
The use of 'p2rereq_array' appears to be a typo.
The 'llvm_dylib' variable doesn't seem to be set anywhere, so the code in question looks dead. This patch comments it out. (FIXME: Intent?)
This patch adds shellcheck 'disable' annotations in several places that aren't wrong, but show up as false positives with the tool.
If running these scripts under osh (https://oils.pub), stricter semantics (improved error checking, etc.) can be enabled using this patch. It should be a no-op under bash.
If "$(which getconf)" fails, running it inside the condition of an 'if' statement will mask the error. This patch breaks out the substitution into a separate line.
This patch initialises the 'list' variable in build_aomp.sh before appending to it. This fixes an error running under osh.
This patch simplifies the quoting for SED_INSTALL_DIR in build_extras.sh.
dpalermo
approved these changes
Jun 2, 2025
Contributor
dpalermo
left a comment
dpalermo
left a comment
There was a problem hiding this comment.
Works fine with the nightly build scripts. Did another pass through the patch and all changes look good to me. Thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
These patches constitute the result of running Shellcheck (https://www.shellcheck.net/) on the AOMP scripts, mostly build*.sh so far but a few other bits crept in too. With this, "shellcheck -x build*.sh" runs clean, and the resulting patches run "build_aomp.sh" and "run_epsdb_aomp_test.sh" successfully. (I will probably need help with further testing, since there are likely lots of use cases I'm not very familiar with). These patches are mostly quite mechanical, but touch a lot of code.
Some of these bits are less controversial than others. Shellcheck is a little opinionated (e.g. regarding backtick syntax), so we don't necessarily need to do all this stuff, but some things in here are definitely real bugs it found, too.