From ad800ef0815ea37ce1c7b2fd6484d679b361d20e Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 15 Oct 2024 06:32:52 -0700 Subject: [PATCH] Target specific scripts in "Check Scripts" template Background ---------- Previously, the approach taken shell script tasks was to recursively search the entire repository for shell scripts. There were several problems with that system: The `-regextype` flag used in the `find` commands of the `shell:check` and `shell:check-mode` tasks is not supported by the BSD/macOS version of `find`, meaning the tasks would fail if a contributor using a macOS machine tried to run them: ``` find: -regextype: unknown primary or operator ``` The `shell:check` and `shell:check-mode` tasks only provided coverage for files with `.sh` and `.bash` file extensions, whereas the practice of omitting a file extension on shell scripts is unfortunately quite common. There was no obvious way to exclude paths of externally maintained files from coverage by the `shell:format` task. Alternative Solutions --------------------- The first of the problems listed above could be overcome by configuring the tasks to adjust the `find` commands to use different flags depending on the operating system of the machine. The second could be overcome by using `shfmt --files` (which searches recursively for shell scripts by checking for the presence of a shebang inside files and outputs a list of the paths) as a replacement for `find`. The third could be overcome by documenting the usage of shfmt's poorly advertised feature of ignoring the paths assigned an `ignore` attribute in the .editorconfig file. Chosen Solution --------------- After careful consideration, the decision was made to abandon the previous approach of attempting to automatically discover script files and instead instead use the strategy of configuring the template with the specific paths of the scripts to be checked for each project it is installed into. Although such an approach would not be appropriate in the case of a check on files that tend to be present in greater abundance and regularly added and moved in a project, this is not the case for shell scripts in Arduino projects. A project is more likely to contain a few scripts at most and their paths tend to be reasonably stable. Code Duplication in Workflow ---------------------------- In order to make it easier to interpret results, navigate logs, and reduce duration of workflow runs, a separate workflow job is used for each of the distinct checks. Unfortunately it is necessary for the project maintainer to configure the script paths redundantly in the matrix of each of the three jobs. Intuitively we would expect this could be avoided by defining the paths at workflow scope via the env key. However, this is not feasible due to the env context not being available for use in the jobs..strategy.matrix key. It is technically possible to accomplish this by adding a job that converts the data from the `env` context into a job output (the `needs` context is available for use in the `matrix` key). However the significant increase in complexity this would bring to the workflow outweighs the benefit of avoiding duplication. --- .github/workflows/check-shell-task.yml | 37 +++++++++++++-- Taskfile.yml | 66 +++++++++++--------------- 2 files changed, 60 insertions(+), 43 deletions(-) diff --git a/.github/workflows/check-shell-task.yml b/.github/workflows/check-shell-task.yml index 8df29ca9f..39dbd9577 100644 --- a/.github/workflows/check-shell-task.yml +++ b/.github/workflows/check-shell-task.yml @@ -50,7 +50,7 @@ jobs: echo "result=$RESULT" >> $GITHUB_OUTPUT lint: - name: ${{ matrix.configuration.name }} + name: ${{ matrix.configuration.name }} (${{ matrix.script }}) needs: run-determination if: needs.run-determination.outputs.result == 'true' runs-on: ubuntu-latest @@ -75,6 +75,8 @@ jobs: # ShellCheck's "tty" output format is most suitable for humans reading the log. format: tty continue-on-error: false + script: + - etc/install.sh steps: - name: Set environment variables @@ -114,15 +116,26 @@ jobs: continue-on-error: ${{ matrix.configuration.continue-on-error }} with: linters: gcc - run: task --silent shell:check SHELLCHECK_FORMAT=${{ matrix.configuration.format }} + # Due to a quirk of the "liskin/gh-problem-matcher-wrap" action, the entire command must be on a single line + # (instead of being broken into multiple lines for readability). + run: | + task --silent shell:check SCRIPT_PATH="${{ matrix.script }}" SHELLCHECK_FORMAT=${{ matrix.configuration.format }} formatting: + name: formatting (${{ matrix.script }}) needs: run-determination if: needs.run-determination.outputs.result == 'true' runs-on: ubuntu-latest permissions: contents: read + strategy: + fail-fast: false + + matrix: + script: + - etc/install.sh + steps: - name: Set environment variables run: | @@ -158,18 +171,30 @@ jobs: echo "${{ env.SHFMT_INSTALL_PATH }}" >> "$GITHUB_PATH" - name: Format shell scripts - run: task --silent shell:format + run: | + task \ + --silent \ + shell:format \ + SCRIPT_PATH="${{ matrix.script }}" - name: Check formatting run: git diff --color --exit-code executable: + name: executable (${{ matrix.script }}) needs: run-determination if: needs.run-determination.outputs.result == 'true' runs-on: ubuntu-latest permissions: contents: read + strategy: + fail-fast: false + + matrix: + script: + - etc/install.sh + steps: - name: Checkout repository uses: actions/checkout@v4 @@ -181,4 +206,8 @@ jobs: version: 3.x - name: Check for non-executable scripts - run: task --silent shell:check-mode + run: | + task \ + --silent \ + shell:check-mode \ + SCRIPT_PATH="${{ matrix.script }}" diff --git a/Taskfile.yml b/Taskfile.yml index 107f29359..7a247ba70 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -396,73 +396,61 @@ tasks: cmds: - poetry run flake8 --show-source + # Parameter variables: + # - SCRIPT_PATH: path of the script to be checked. # Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml shell:check: desc: Check for problems with shell scripts cmds: + - | + if [[ "{{.SCRIPT_PATH}}" == "" ]]; then + echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable." + echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage" + exit 1 + fi - | if ! which shellcheck &>/dev/null; then echo "shellcheck not installed or not in PATH. Please install: https://github.com/koalaman/shellcheck#installing" exit 1 fi - | - # There is something odd about shellcheck that causes the task to always exit on the first fail, despite any - # measures that would prevent this with any other command. So it's necessary to call shellcheck only once with - # the list of script paths as an argument. This could lead to exceeding the maximum command length on Windows if - # the repository contained a large number of scripts, but it's unlikely to happen in reality. shellcheck \ --format={{default "tty" .SHELLCHECK_FORMAT}} \ - $( - # The odd method for escaping . in the regex is required for windows compatibility because mvdan.cc/sh gives - # \ characters special treatment on Windows in an attempt to support them as path separators. - find . \ - -path ".git" -prune -or \ - \( \ - -regextype posix-extended \ - -regex '.*[.](bash|sh)' -and \ - -type f \ - \) - ) + "{{.SCRIPT_PATH}}" + # Parameter variables: + # - SCRIPT_PATH: path of the script to be checked. # Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml shell:check-mode: desc: Check for non-executable shell scripts cmds: - | - EXIT_STATUS=0 - while read -r nonExecutableScriptPath; do - # The while loop always runs once, even if no file was found - if [[ "$nonExecutableScriptPath" == "" ]]; then - continue - fi - - echo "::error file=${nonExecutableScriptPath}::non-executable script file: $nonExecutableScriptPath"; - EXIT_STATUS=1 - done <<<"$( - # The odd approach to escaping `.` in the regex is required for windows compatibility because mvdan.cc/sh - # gives `\` characters special treatment on Windows in an attempt to support them as path separators. - find . \ - -path ".git" -prune -or \ - \( \ - -regextype posix-extended \ - -regex '.*[.](bash|sh)' -and \ - -type f -and \ - -not -executable \ - -print \ - \) - )" - exit $EXIT_STATUS + if [[ "{{.SCRIPT_PATH}}" == "" ]]; then + echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable." + echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage" + exit 1 + fi + - | + test -x "{{.SCRIPT_PATH}}" + # Parameter variables: + # - SCRIPT_PATH: path of the script to be formatted. # Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml shell:format: desc: Format shell script files cmds: + - | + if [[ "{{.SCRIPT_PATH}}" == "" ]]; then + echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable." + echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage" + exit 1 + fi - | if ! which shfmt &>/dev/null; then echo "shfmt not installed or not in PATH. Please install: https://github.com/mvdan/sh#shfmt" exit 1 fi - - shfmt -w . + - shfmt -w "{{.SCRIPT_PATH}}" # Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-mkdocs-task/Taskfile.yml website:check: