Skip to content

Commit

Permalink
internal/ci: fix up modified times for all directories
Browse files Browse the repository at this point in the history
Our recent change in https://cuelang.org/cl/551227 did indeed cause more
packages to start hitting the test cache properly, but some of the
heavier ones like ./cmd/cue/cmd were still causing misses.

After some debugging, it turns out that git-restore-mtime does not
properly update the modified time for directories which themselves only
contain other directories. This happens with cmd/cue/cmd/testdata, for
example, which explains the case above.

Since Go currently hashes the modified time as well as the entire
content for every file that is opened, including directories,
we don't actually need the modified timestamps to be perfect;
we just need them to be stable enough to allow test cache hits.

For that reason, reset all directory timestamps to a dummy static
timestamp before we run git-restore-mtime, to act as a fallback for
those directories affected by the bug.

We could use a static timestamp for all files and directories,
removing git-restore-mtime from the equation entirely,
but that could potentially lead to false test cache hits if any of our
caching does rely only on modified times.
For that reason, be conservative and keep both.

Fixing up the modified times now uses two steps,
so refactor #checkoutCode into a list of three steps to be embedded,
per Paul's suggestion to keep the config easy to compose.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I69aba918965ce31fb27fd95ef0bc13c742735e57
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/551230
Reviewed-by: Paul Jolly <paul@myitcv.io>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
  • Loading branch information
mvdan committed Mar 19, 2023
1 parent daa4022 commit e31b8f7
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 29 deletions.
5 changes: 5 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ jobs:
- name: Checkout code
uses: actions/checkout@v3
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0
- name: Reset git directory modification times
run: find . -not -path '*/.*' -type d -exec touch -t 202211302355 {} ';'
- name: Restore git file modification times
uses: chetan/git-restore-mtime-action@075f9bc9d159805603419d50f794bd9f33252ebe
- name: Install Go
uses: actions/setup-go@v3
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/trybot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ jobs:
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0
- name: Reset git directory modification times
run: find . -not -path '*/.*' -type d -exec touch -t 202211302355 {} ';'
- name: Restore git file modification times
uses: chetan/git-restore-mtime-action@075f9bc9d159805603419d50f794bd9f33252ebe
- name: Install Go
Expand Down
47 changes: 33 additions & 14 deletions internal/ci/base/base.cue
Original file line number Diff line number Diff line change
Expand Up @@ -48,20 +48,39 @@ import (
}
}

#checkoutCode: json.#step & {
name: "Checkout code"
uses: "actions/checkout@v3"
}

// #restoreGitModTimes works around test cache misses due to https://go.dev/issues/58571.
// Note that this action requires actions/checkout to use a fetch-depth of 0.
// Since this is a third-party action which runs arbitrary code,
// we pin a commit hash for v2 to be in control of code updates.
// TODO(mvdan): May be unnecessary once the Go bug above is fixed.
#restoreGitModTimes: json.#step & {
name: "Restore git file modification times"
uses: "chetan/git-restore-mtime-action@075f9bc9d159805603419d50f794bd9f33252ebe"
}
#checkoutCode: [
json.#step & {
name: "Checkout code"
uses: "actions/checkout@v3"

// "pull_request" builds will by default use a merge commit,
// testing the PR's HEAD merged on top of the master branch.
// For consistency with Gerrit, avoid that merge commit entirely.
// This doesn't affect builds by other events like "push",
// since github.event.pull_request is unset so ref remains empty.
with: {
ref: "${{ github.event.pull_request.head.sha }}"
"fetch-depth": 0 // see the docs below
}
},
// Restore modified times to work around https://go.dev/issues/58571,
// as otherwise we would get lots of unnecessary Go test cache misses.
// Note that this action requires actions/checkout to use a fetch-depth of 0.
// Since this is a third-party action which runs arbitrary code,
// we pin a commit hash for v2 to be in control of code updates.
// Also note that git-restore-mtime does not update all directories,
// per the bug report at https://github.com/MestreLion/git-tools/issues/47,
// so we first reset all directory timestamps to a static time as a fallback.
// TODO(mvdan): May be unnecessary once the Go bug above is fixed.
json.#step & {
name: "Reset git directory modification times"
run: "find . -not -path '*/.*' -type d -exec touch -t 202211302355 {} ';'"
},
json.#step & {
name: "Restore git file modification times"
uses: "chetan/git-restore-mtime-action@075f9bc9d159805603419d50f794bd9f33252ebe"
},
]

#earlyChecks: json.#step & {
name: "Early git and code sanity checks"
Expand Down
4 changes: 1 addition & 3 deletions internal/ci/github/release.cue
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ release: _base.#bashWorkflow & {
"runs-on": _#linuxMachine
if: "${{github.repository == '\(core.#githubRepositoryPath)'}}"
steps: [
_base.#checkoutCode & {
with: "fetch-depth": 0
},
for v in _base.#checkoutCode {v},
_base.#installGo & {
with: "go-version": core.#pinnedReleaseGo
},
Expand Down
14 changes: 2 additions & 12 deletions internal/ci/github/trybot.cue
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,7 @@ trybot: _base.#bashWorkflow & {
strategy: _#testStrategy
"runs-on": "${{ matrix.os }}"
steps: [
_base.#checkoutCode & {
// "pull_request" builds will by default use a merge commit,
// testing the PR's HEAD merged on top of the master branch.
// For consistency with Gerrit, avoid that merge commit entirely.
// This doesn't affect "push" builds, which never used merge commits.
with: {
ref: "${{ github.event.pull_request.head.sha }}"
"fetch-depth": 0 // see #restoreGitModTimes's doc
}
},
_base.#restoreGitModTimes,
for v in _base.#checkoutCode {v},
_base.#installGo,

// cachePre must come after installing Node and Go, because the cache locations
Expand All @@ -69,7 +59,7 @@ trybot: _base.#bashWorkflow & {
if: _#isLatestLinux
},
json.#step & {
if: "\(_#isProtectedBranch) || \(_#isLatestLinux)"
if: "\(_#isProtectedBranch) || \(_#isLatestLinux)"
run: "echo CUE_LONG=true >> $GITHUB_ENV"
},
_#goGenerate,
Expand Down

0 comments on commit e31b8f7

Please sign in to comment.