felipepessoto commented on PR #12781:
URL: https://github.com/apache/gluten/pull/12781#issuecomment-5345471025
Rebased onto `main` to pick up #12820 (Delta Spark UT caches moved to Apache
Stash), which conflicted with this PR since both touch the cache steps.
**The conflict itself was mostly a simplification.** #12820 already splits
every cache into `stash/restore` + `stash/save`, which this PR previously had
to do by hand — so the restore/save split, the `id:`s and the `cache-hit`
guards are all gone. The functional diff dropped from 58 lines to 42; what
remains on top of `main` is the `issue_comment` trigger, `DELTA_CHECKOUT_REF` +
four `ref:` pins, the gate job, and three `if:` guards.
**It also required one change beyond conflict resolution** (separate commit,
`8911f45`):
**`permissions:` must grant `actions: read`.** Declaring a `permissions:`
block at all sets every unlisted scope to `none`, and Stash's restore reads
caches through the artifacts REST API:
```
gh api repos/{repo}/actions/runs/{run_id}/artifacts
gh api repos/{repo}/actions/artifacts
gh run download
```
all of which 403 without `actions: read`. Every other Stash-using workflow
here (`velox_backend_x86.yml`, `velox_weekly.yml`, `velox_backend_arm.yml`, …)
declares no `permissions:` block and so inherits the repo default, which is why
none of them says this. Left unfixed, the Stash restores would have failed on
**every** event, not just comment runs — a cold ccache on the nightly.
**And I re-verified the guards still have a job to do under Stash.** They
do, more so than before. A stash is an artifact named
`<key>-<github.ref_name>`, restored by matching `head_branch` +
`head_repository_id` — the same branch scoping `actions/cache` has (the
action's README says so explicitly). On `issue_comment` that branch is the
**default branch**, so a save from a run executing PR code lands on `main`:
| event | `github.ref_name` | artifact written |
|---|---|---|
| `pull_request` | `my-branch` | `<key>-my-branch` |
| `schedule` (nightly) | `main` | `<key>-main` |
| `issue_comment` | `main` | **`<key>-main`** ← same name the nightly reads |
Two things make it sharper than under `actions/cache`: Stash's save defaults
to `overwrite: true`, so it *replaces* the existing entry rather than losing a
race with it; and #12820 moved the ccache onto
`ccache-centos7-release-default-${{ hashFiles('ep/build-velox/src/**') }}`,
which `velox_backend_x86.yml` also restores from — so an unguarded save would
reach beyond this pipeline. Restores stay unguarded, so a comment run still
reads `main`'s stashes and is no slower.
Verified: actionlint clean; authorization and `DELTA_CHECKOUT_REF` truth
tables re-run against the rebased file; asserted 3/3 saves guarded and 3/3
restores unguarded, and that no job overrides `permissions:` in a way that
would strip `actions: read` from a Stash step. README updated for Stash
semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
