ci: refactor test_helper.sh#4209
Conversation
Signed-off-by: Vitor Bandeira <vvbandeira@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors test_helper.sh to introduce named command-line arguments, a usage menu, and more robust make command execution using Bash arrays. Review feedback highlights a potential crash in the new argument parsing logic if an option is provided without a value and suggests using here-strings instead of process substitution for better Bash idiomaticity.
| --design) DESIGN_NAME=$2; shift 2 ;; | ||
| --platform) PLATFORM=$2; shift 2 ;; | ||
| --config) CONFIG_MK=$2; shift 2 ;; | ||
| --design-path) DESIGN_PATH=$2; shift 2 ;; | ||
| --variant) FLOW_VARIANT=$2; shift 2 ;; | ||
| --target) TARGET=$2; shift 2 ;; | ||
| --work-home) WORK_HOME=$2; shift 2 ;; | ||
| --private-dir) PRIVATE_DIR=$2; shift 2 ;; |
There was a problem hiding this comment.
The current flag parsing logic uses shift 2 for options that require an argument (e.g., --design, --platform). If an option is provided as the last argument without a value, shift 2 will fail because the shift count exceeds the number of remaining arguments. Since set -e is active, this will cause the script to crash with a shift count out of range error. Additionally, if another flag follows an option that expects a value (e.g., --design --platform), the second flag will be incorrectly assigned as the value for the first option. Consider adding a check to ensure an argument is present before shifting.
| TARGETS=$($__make -np | grep -e '^[^ ]*:') | ||
| if [ $ret -eq 0 ] && grep -q 'simulate:' <(echo $TARGETS); then | ||
| TARGETS=$("${__make[@]}" -np | grep -e '^[^ ]*:') | ||
| if [ $ret -eq 0 ] && grep -q 'simulate:' <(echo "$TARGETS"); then |
There was a problem hiding this comment.
For consistency with the use of here-strings elsewhere in the script (e.g., line 92), you can use <<< "$TARGETS" instead of <(echo "$TARGETS"). This is more idiomatic in Bash and avoids spawning a subshell for echo.
| if [ $ret -eq 0 ] && grep -q 'simulate:' <(echo "$TARGETS"); then | |
| if [ $ret -eq 0 ] && grep -q 'simulate:' <<< "$TARGETS"; then |
2 participants
No description provided.