[CI] Unify handling of scripted PR workflows#199127
Conversation
A few workflows already force the use of baseline scripting when testing PRs. This PR pulls that out into a separate action that we can just reuse in other workflows as well.
|
@llvm/pr-subscribers-github-workflow Author: Oliver Hunt (ojhunt) ChangesA few workflows already force the use of baseline scripting when testing PRs. This PR pulls that out into a separate action that we can just reuse in other workflows as well. Full diff: https://github.com/llvm/llvm-project/pull/199127.diff 3 Files Affected:
diff --git a/.github/actions/checkout-baseline-tooling/action.yml b/.github/actions/checkout-baseline-tooling/action.yml
new file mode 100644
index 0000000000000..c92d724394967
--- /dev/null
+++ b/.github/actions/checkout-baseline-tooling/action.yml
@@ -0,0 +1,17 @@
+name: Checkout Baseline Tooling
+description: >-
+ Checkout the baseline tooling scripts prior to running any pre-commit hooks
+ or other GH action or workflow scripts.
+
+runs:
+ using: composite
+ steps:
+ - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+ with:
+ persist-credentials: false
+ sparse-checkout: |
+ llvm/utils/git/
+ .ci/
+ .github/
+ ref: main
+ path: baseline-scripts
diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index 9841301f32a2d..ef6fac649e181 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -26,6 +26,9 @@ jobs:
persist-credentials: false
fetch-depth: 2
+ - name: Checkout Baseline Tooling
+ uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main
+
- name: Get changed files
id: changed-files
uses: tj-actions/changed-files@9426d40962ed5378910ee2e21d5f8c6fcbf2dd96 # v47.0.6
@@ -49,10 +52,10 @@ jobs:
# Create an empty comments file so the pr-write job doesn't fail.
run: |
echo "[]" > comments &&
- python ./llvm/utils/git/code-format-helper.py \
+ python ./baseline-scripts/llvm/utils/git/code-format-helper.py \
--write-comment-to-file \
--token ${{ secrets.GITHUB_TOKEN }} \
- --issue-number $GITHUB_PR_NUMBER \
+ --issue-number "$GITHUB_PR_NUMBER" \
--start-rev HEAD~1 \
--end-rev HEAD \
--changed-files "$CHANGED_FILES"
diff --git a/.github/workflows/pr-code-lint.yml b/.github/workflows/pr-code-lint.yml
index 5101db3ca7612..f37ddb69b0080 100644
--- a/.github/workflows/pr-code-lint.yml
+++ b/.github/workflows/pr-code-lint.yml
@@ -33,6 +33,9 @@ jobs:
persist-credentials: false
fetch-depth: 2
+ - name: Checkout Baseline Tooling
+ uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main
+
- name: Get changed files
id: changed-files
uses: tj-actions/changed-files@9426d40962ed5378910ee2e21d5f8c6fcbf2dd96 # v47.0.6
@@ -55,7 +58,7 @@ jobs:
run: |
git config --global --add safe.directory '*'
- . <(git diff --name-only HEAD~1...HEAD | python3 .ci/compute_projects.py)
+ . <(git diff --name-only HEAD~1...HEAD | python3 ./baseline-scripts/.ci/compute_projects.py)
if [[ "${projects_to_build}" == "" ]]; then
echo "No projects to analyze"
@@ -91,7 +94,7 @@ jobs:
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
echo "[]" > comments &&
- python3 llvm/utils/git/code-lint-helper.py \
+ python3 ./baseline-scripts/llvm/utils/git/code-lint-helper.py \
--token ${{ secrets.GITHUB_TOKEN }} \
--issue-number $GITHUB_PR_NUMBER \
--start-rev HEAD~1 \
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| fetch-depth: 2 | ||
|
|
||
| - name: Checkout Baseline Tooling | ||
| uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
| fetch-depth: 2 | ||
|
|
||
| - name: Checkout Baseline Tooling | ||
| uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
| fetch-depth: 2 | ||
|
|
||
| - name: Checkout Baseline Tooling | ||
| uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
| fetch-depth: 2 | ||
|
|
||
| - name: Checkout Baseline Tooling | ||
| uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
| runs: | ||
| using: composite | ||
| steps: | ||
| - uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
|
That commit actually does the "right" thing from the PoV of what I was trying to do, but of course it (in principle) still breaks the CI because of course these scripts already clobber the directory with the new actions. @tstellar Behold my skill :D |
|
Awww, I thought my clever "llvm/main" would work, but having read more I think it's a broken and/or dumb idea :( I guess the real path is land change then update hashes :-/ |
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
Can you link actions that are already doing this? I'm skeptical a new composite action actually makes things simpler.
I'm also not sure how much this actually helps. Premerge is going to have way more problems than code formatting/linting since it just runs the build, and has the same exposure as formatting/linting.
| runs: | ||
| using: composite | ||
| steps: | ||
| - uses: llvm/llvm-project/.github/actions/checkout-baseline-tooling@main |
There was a problem hiding this comment.
Also - I think? even if this could work I need a repository parameter as well?
There was a problem hiding this comment.
This is just referencing itself currently which doesn't make a lot of sense?
There was a problem hiding this comment.
@boomanaiden154 I was trying to make it just pull from the main llvm repository on the assumption that would always be trusted and then avoid a specific SHA check :D
it looks like the actual correct way to do this would be land, and then enable with a pin, but I was so hopeful :D
There was a problem hiding this comment.
If that's the intent, you need to SHA pin in the user workflows.
A recursive self reference I don't think makes sense in any case.
| llvm/utils/git/ | ||
| .ci/ | ||
| .github/ |
There was a problem hiding this comment.
If this were meant to be a general action maybe it should cover all ci scripting directories? (e.g. libcxx, compiler-rt, etc?)
A consistent tool for all workflows seems like the sensible course of action
There was a problem hiding this comment.
Most of the scripts are in llvm/utils. I'm not sure a single tool for all workflows makes a lot of sense as they will all want to check out different things. An additional checkout step I think is also about the same number of lines of code...
There was a problem hiding this comment.
@boomanaiden154 my thinking is just having a baseline of "all the scripts should be the baseline versions" and a single action that we could have a trivial lint for seems like it would be a good idea? I also realize I did mean to include a pile of the libcxx testing utilities in here as well, and compiler-rt, and any others that just seemed used everywhere
There was a problem hiding this comment.
Lumping a bunch of things makes the sparse checkout less effective. I don't think utilities from compiler-rt are used inside any of these sorts of workflows. All the libcxx utilities would be specific to libcxx workflows.
If we actually want to make this a policy, I think we're better off documenting it in https://llvm.org/docs/CIBestPractices.html and trying to enforce it during review. We would have to do that anyways to ensure that someone is using utilities from this action rather than elsewhere.
do you mean the "Oliver read about composites and tried to write one?" or the hackery around getting the baseline? the latter was based on google and trying to get past the endless llm generated garbage that now fills the world. I wasn't sure if it was correct to do the sparse checkout things like this, because they all seemed to occur in gated workflows. - name: Checkout LLVM
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
persist-credentials: false
sparse-checkout: |
.github/workflows/containers/github-action-ci-tooling/
...The need for a separate checkout seemed plausible, but there's also a reason that I mass added CI folk to this PR :D |
What do you mean by gated workflows? |
Only automatically run by members of the project/people who can write to the repository |
|
Also, both of these actions are triggered by |
3 participants
A few workflows already force the use of baseline scripting when testing PRs. This PR pulls that out into a separate action that we can just reuse in other workflows as well.