Skip to content

[NFCI] Make aomp_common_vars clean under shellcheck - #2578

Merged
mhalk merged 1 commit into
ROCm:aomp-devfrom
mhalk:amd/dev/mhalkenh/nfc/shellcheck-compliance-aomp-common-vars
Sep 24, 2026
Merged

mhalk merged 1 commit into
ROCm:aomp-devfrom
mhalk:amd/dev/mhalkenh/nfc/shellcheck-compliance-aomp-common-vars

Conversation

@mhalk

@mhalk mhalk commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Unrelated changes caused GH pre-merge CI to fail shellcheck check.
Modernize formatting via shfmt.

Technical Details

Record once that the file defines variables for the scripts that source it, so their uses are invisible to static analysis, and quote the two expansions flagged for word splitting. Neither has an effect.

Quoting the array element in shquot also stops an argument holding a tab or a newline from being split into several quoted words. That output is meant to be pasted back into a shell, where the split silently alters the command.

Test Plan

Use run script which sources this file as usual.

Test Result

Executed script works as expected.

Submission Checklist

Record once that the file defines variables for the scripts that source it,
so their uses are invisible to static analysis, and quote the two expansions
flagged for word splitting. Neither has an effect.

Quoting the array element in shquot also stops an argument holding a tab or
a newline from being split into several quoted words. That output is meant
to be pasted back into a shell, where the split silently alters the command.

AI-assisted.
@mhalk

mhalk commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Here's the corresponding failed workflow as reference:
https://github.com/ROCm/aomp/actions/runs/35756404838/job/106843162688

Should produce the same output as:
./bin/aomp-shellcheck ./bin/aomp_common_vars

@mhalk mhalk changed the title [NFC] Make aomp_common_vars clean under shellcheck [NFCI] Make aomp_common_vars clean under shellcheck + apply shfmt Sep 23, 2026
@mhalk

mhalk commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Maybe applying shfmt is a step too far, please let me know what you think.

@mhalk

mhalk commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

fixup: shfmt -i 2 -w ./bin/aomp_common_vars (added -i 2)

@jplehr

jplehr commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

I don't think we settled on using shfmt to format and while I am in favor, I don't think we should force that onto everybody.

@mhalk

mhalk commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

I don't think we settled on using shfmt to format and while I am in favor, I don't think we should force that onto everybody.

Check; dropping that part of the work for both affected PRs (for now).

@mhalk
mhalk force-pushed the amd/dev/mhalkenh/nfc/shellcheck-compliance-aomp-common-vars branch from df9b458 to 3611c05 Compare September 23, 2026 13:55
@jplehr jplehr changed the title [NFCI] Make aomp_common_vars clean under shellcheck + apply shfmt [NFCI] Make aomp_common_vars clean under shellcheck Sep 24, 2026
@mhalk

mhalk commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the approval and title correction! :)

@mhalk
mhalk merged commit 527f7bc into ROCm:aomp-dev Sep 24, 2026
2 checks passed
@mhalk
mhalk deleted the amd/dev/mhalkenh/nfc/shellcheck-compliance-aomp-common-vars branch September 24, 2026 11:44
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