Lint GitHub Actions and Dependabot #126002
Conversation
| - name: Setup directory envs for out-of-tree builds | ||
| run: | | ||
| echo "CPYTHON_BUILDDIR=$(realpath -m ${GITHUB_WORKSPACE}/../cpython-builddir)" >> $GITHUB_ENV | ||
| echo "CPYTHON_BUILDDIR=$(realpath -m "${GITHUB_WORKSPACE}"/../cpython-builddir)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Do you need different quotes here for the nested strings?
| echo "CPYTHON_BUILDDIR=$(realpath -m "${GITHUB_WORKSPACE}"/../cpython-builddir)" >> "$GITHUB_ENV" | |
| echo 'CPYTHON_BUILDDIR=$(realpath -m "${GITHUB_WORKSPACE}"/../cpython-builddir)' >> "$GITHUB_ENV" |
Or just skip this one, since it's already inside double quotes?
| echo "CPYTHON_BUILDDIR=$(realpath -m "${GITHUB_WORKSPACE}"/../cpython-builddir)" >> "$GITHUB_ENV" | |
| echo "CPYTHON_BUILDDIR=$(realpath -m ${GITHUB_WORKSPACE}/../cpython-builddir)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
It's this warning:
367 | run: |
| ^~~~
.github/workflows/build.yml:367:7: shellcheck reported issue in this script: SC2086:info:1:83: Double quote to prevent globbing and word splitting [shellcheck]
Looking up the code: https://www.shellcheck.net/wiki/SC2086
This applies:
Note that
$( )starts a new context, and variables in it have to be quoted independently:echo "This $variable is quoted $(but this $variable is not)" echo "This $variable is quoted $(and now this "$variable" is too)"
Or just skip this one, since it's already inside double quotes?
That would trigger this:
.github/workflows/build.yml:367:7: shellcheck reported issue in this script: SC2016:info:1:6: Expressions don't expand in single quotes, use double quotes for that [shellcheck]
|
367 | run: |
| ^~~~
There was a problem hiding this comment.
Huh. It looks strange to me, but I'll trust you that this is the right thing to do!
| - name: Configure ccache action | ||
| uses: hendrikmuhs/[email protected] | ||
| with: | ||
| save: ${{ github.event_name == 'push' }} | ||
| max-size: "200M" | ||
| - name: Setup directory envs for out-of-tree builds | ||
| run: | | ||
| echo "CPYTHON_RO_SRCDIR=$(realpath -m ${GITHUB_WORKSPACE}/../cpython-ro-srcdir)" >> $GITHUB_ENV | ||
| echo "CPYTHON_BUILDDIR=$(realpath -m ${GITHUB_WORKSPACE}/../cpython-builddir)" >> $GITHUB_ENV | ||
| echo "CPYTHON_RO_SRCDIR=$(realpath -m "${GITHUB_WORKSPACE}"/../cpython-ro-srcdir)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Same question as above for this and the following line
| timeout-minutes: 60 | ||
| needs: check_source | ||
| if: needs.check_source.outputs.run_tests == 'true' | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-22.04] |
There was a problem hiding this comment.
The change itself looks ok, but I was wondering if it was just for consistency or if there is another reason.
There was a problem hiding this comment.
I think it's because on main the matrix.os variable here is currently undefined:
cpython/.github/workflows/build.yml
Line 449 in c5b99f5
There was a problem hiding this comment.
That's right.
This was added in #124403, where we replaced ${{ runner.os }} (which evaluates to Linux) with ${{ matrix.os }} (which evaluates to ubuntu-22.04), so the cache would be cleared when we update to 24.04.
In that PR, you can see we added os: to the top one, but forgot for the bottom one (and we didn't have a linter).
See also #123699 (comment) for more background, cc @zware.
Yeah, I'm more than happy to remove that commit. I guess it's only technically correct for this stuff:
And if we don't have stuff like that we don't need to quote. We don't necessarily need a lint rule that requires all being quoted, if the cost is too nitpicky or noisy.
You're welcome, I also missed it in review :) |
Extra linting
Add extra linting of GitHub Actions and Dependabot config files.
check-dependabotandcheck-github-workflowsdon't currently have any warnings.actionlinthas lots. Here are all the warnings without fixes or ignores:Details
Fix warnings
This PR fixes
.github/workflows/build.yml:449:18: property "os" is not defined in object type {} [expression], which is a bug, and would have helped for #125786.This also fixes
SC2086: Double quote to prevent globbing and word splitting, but this could potentially be ignored too.I ignored all the rest, these can be tackled at another time if someone is motivated and they look worthwhile:
Details
Also let's bump the other pre-commit hooks whilst we're here.