Clean up util directory#59
Conversation
|
I'll hassle everyone tomorrow about the scripts in util. I want to document those in the directory's README.md file (or delete them if they are unused). |
There was a problem hiding this comment.
Pull Request Overview
This PR adds proper variable quoting to shell scripts based on shellcheck recommendations, improving script reliability and preventing word splitting and globbing issues. The changes span four utility scripts that handle VFIO device binding, FIO benchmarking, clang formatting, and git file change detection.
Key changes:
- Added quotes around all variable expansions in file paths and command arguments
- Properly quoted command substitutions with nested variable expansions
- Improved arithmetic expression formatting by removing unnecessary
${}syntax within$(())
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| util/vfio.sh | Added quotes around $bdf, $vendor_id, $device_id, $iommu_group, and $driver variables in file paths and echo statements |
| util/iterate-fio-jobs.sh | Added quotes around variables in eval commands, echo statements, and jq operations; improved arithmetic expression formatting |
| util/full-clang-lint.sh | Added quotes around $WORKING_DIRECTORY and $file_to_lint in find and clang-format commands |
| util/files-changed.sh | Added quotes around git command arguments and pattern matching variables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FIO_PID=$! | ||
| echo "FIO_PID: ${FIO_PID}" | ||
| pidstat_output=$(pidstat -u -p ${FIO_PID} 1 $((${fio_runtime}+${fio_ramptime}))) | ||
| pidstat_output=$(pidstat -u -p ${FIO_PID} 1 $((fio_runtime + fio_ramptime))) |
There was a problem hiding this comment.
The variable ${FIO_PID} should be quoted as \"${FIO_PID}\" to be consistent with the PR's shellcheck quoting improvements and prevent potential word splitting issues.
| pidstat_output=$(pidstat -u -p ${FIO_PID} 1 $((fio_runtime + fio_ramptime))) | |
| pidstat_output=$(pidstat -u -p "${FIO_PID}" 1 $((fio_runtime + fio_ramptime))) |
| fi | ||
| done | ||
|
|
||
| printf '%s' ${FILES_CHANGED} |
There was a problem hiding this comment.
The variable ${FILES_CHANGED} should be quoted as \"${FILES_CHANGED}\" to be consistent with the PR's shellcheck quoting improvements, even though it contains only numeric values.
| printf '%s' ${FILES_CHANGED} | |
| printf '%s' "${FILES_CHANGED}" |
* Remove [INTERNAL] tag and rename H1 to hipFile * Add H2 entries for each script
522a64b to
243369c
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
| done | ||
|
|
||
| printf '%s' ${FILES_CHANGED} |
There was a problem hiding this comment.
The variable FILES_CHANGED should be quoted here to follow the same shellcheck improvements applied to other variables in this file. This ensures consistent behavior when the variable is empty or contains special characters.
Suggested change:
printf '%s' "${FILES_CHANGED}"| printf '%s' ${FILES_CHANGED} | |
| printf '%s' "${FILES_CHANGED}" |
|
|
||
| ## llvm-coverage.sh | ||
|
|
||
| Code coverage script used by CI. |
There was a problem hiding this comment.
The README.md documents all shell scripts but doesn't mention the docker/ subdirectory which contains three DOCKERFILE files (ais_ci_rocky, ais_ci_suse, ais_ci_ubuntu). Consider adding a section documenting the docker subdirectory and its purpose for completeness.
Example:
## docker/
Contains Dockerfiles for CI environments on different Linux distributions (Rocky, SUSE, Ubuntu).| Code coverage script used by CI. | |
| Code coverage script used by CI. | |
| ## docker/ | |
| Contains Dockerfiles for CI environments on different Linux distributions (Rocky, SUSE, Ubuntu). |
[INTERNAL]tag