[MINOR] Persist the Maven build cache between CI runs - #882
Conversation
5b1c12d to
1d7434f
Compare
.mvn/extensions.xml enables maven-build-cache-extension, but the cache never survives a CI run: actions/setup-java's `cache: maven` persists ~/.m2/repository only, while the extension writes to ~/.m2/build-cache. The effect is visible in any recent build log - ten modules, ten "Local build was not found by checksum" lines, zero restores - so every run rebuilds and retests all ten modules from scratch, including for pull requests that touch only the website or a single module. Restore ~/.m2/build-cache before the build and save it afterwards. Only main writes an entry; pull requests restore from main's rather than each branch consuming the repository-wide 10 GB cache budget. Source Build Check is deliberately left alone - it passes -Dmaven.build.cache.enabled=false and skips `cache: maven` on purpose, to prove the source release builds without prebuilt artifacts.
Persisting ~/.m2/build-cache as the previous commit does is only useful if the entry is small enough to live alongside the ~/.m2/repository caches in the repository-wide 10 GB budget. Measured, it is not: one generation is 1.2 GB, and 1.17 GB of that is three bundled jars - xtable-utilities (1.0 GB), xtable-hive-metastore (125 MB) and xtable-aws (42 MB). Those three modules are worth about 3 minutes of a 27 minute build. xtable-core and xtable-service are 84% of the time and about 1 MB of cache between them. Caching is worth it in inverse proportion to artifact size here, so opt the bundle producers out with the extension's per-project property, which disables both restore and save for that module. A cold build then writes 12 MB instead of 1.2 GB while still caching everything expensive; measured locally, a fully warm run restores 7 of 7 remaining modules. Also in this commit: - Add .mvn/maven-build-cache-config.xml pinning maxBuildsCached to 1, disabling the remote cache explicitly, and reconciling the surefire skip flags. Command line flags are not part of a module's checksum, so without reconciliation a build that skipped tests and one that ran them are indistinguishable to the cache. The flags are tracked without a skipValue attribute deliberately: skipValue relaxes the comparison rather than tightening it. - Bump maven-build-cache-extension 1.1.0 -> 1.2.3. Two invalidation fixes land in 1.2.1: MBUILDCACHE-87, plugin dependencies missing from the checksum, and MBUILDCACHE-99, moved files not always detected. Both affect whether a restore is correct, so they matter more once the cache actually persists. - Guard the save step on the cache directory staying under 100 MB, and warn when it does not, so a future module that starts caching large artifacts is visible rather than silently churning the budget. - Pass -Dmaven.build.cache.enabled=false when publishing a release. versions:set rewrites the version immediately beforehand, while cache restoration is version-agnostic. That runner keeps no cache directory today, so this is a guard against it gaining one, and it matches what source-build-check.yml already does deliberately. - Add the JDK to the Actions cache key, since the extension does not hash the JDK, and raise the job timeout from 30 to 40 minutes - a cold build is already 27 minutes against it, independent of caching. - Teach the spotless license-header delimiter about <cache, the root element of the new config file, alongside the <extensions entry that is already there for .mvn/extensions.xml.
…aved `mvn install` puts xtable's own artifacts into ~/.m2/repository, and that includes the bundled jars - about 1.2 GB, of which xtable-utilities alone is roughly 1 GB. actions/setup-java caches that directory wholesale and offers no way to exclude a path, so remove them in a final step, which runs before its post-save hook. They are rebuilt from source on every run, so they can never produce a useful cache hit, and a stale SNAPSHOT sitting in the local repository can shadow a reactor build. This is a guard rather than a fix. License Check currently reserves the shared cache key first - it downloads the whole dependency tree to inspect each jar's license, which is the useful half - and because it only runs `package`, the saved entry has never contained this project's artifacts. That is why this job logs "Unable to reserve cache with key" at the end of every run. The arrangement is the one we want, but it holds by ordering rather than by construction: if License Check ever fails or is cancelled before its post step, this job would win the key and save roughly 2.15 GB into a repository-wide 10 GB budget.
Both jobs used setup-java's `cache: maven`, which has two problems here. It restores on an exact key match only. setup-java sets no restore-keys deliberately, "to start with a clear cache after dependency update" (actions/setup-java#269), so any change to a pom or to extensions.xml made every job re-download the entire dependency tree from Central. Keying the same way but adding a prefix fallback warms from the previous entry and downloads only the delta. This commit changes both files, so it pays that cost once itself. It also gave the two jobs the same key, and cache entries are immutable, so whichever finished first reserved it and the other's save failed. License Check finishes in about two minutes and the full build takes 27, so in practice License Check always won and Maven CI Build logged "Unable to reserve cache with key" on every run. That outcome was the right one, which is why this makes it explicit rather than trying to win the race. License Check has to resolve every dependency in order to inspect each jar's license, so it populates exactly what the other jobs need, and it stops at `package`, so its entry never contains this project's own artifacts. It is now the sole writer; Maven CI Build restores the same key read-only and no longer attempts a save. The wrapper distribution was cached separately by setup-java, keyed on the wrapper properties so it survives pom changes. That is reproduced here rather than lost. Both jobs also clear ~/.m2/repository/org/apache/xtable before the post-job save, as Source Build Check already does. On the writer that guards the invariant above; on the reader it is belt and braces.
c4290e2 to
4972ec8
Compare
The entry measures 981 MB against a repository-wide 10 GB budget, so a handful of concurrent pull requests with differing pom hashes would crowd out the entry they all restore from. Split restore and save so only main writes, matching what this branch already does for the build cache. Pull requests warm from main through the prefix fallback and re-download only their own delta. Saving is also conditional on the exact key having missed. Cache entries are immutable, so writing over an existing key just fails; that check is what actions/cache performs internally when restore and save are not split. The wrapper cache is left unsplit. At ~9 MB it is not worth gating, and it is keyed on the wrapper properties, which rarely change.
| run is the one skipping, which is exactly the exemption we do not | ||
| want on a release build. | ||
| --> | ||
| <plugin artifactId="maven-surefire-plugin" goal="test"> |
There was a problem hiding this comment.
Do we need a similar config for the failsafe plugin?
There was a problem hiding this comment.
Yes — and it turned out to matter more than the question implied. Two things came out of checking, both pushed in 3414da6.
Failsafe itself. It is bound in the parent POM to integration-test and verify with <skip>${skipITs}</skip>, and CI runs install, so integration tests do run on every build and a restored module skips them exactly as it skips unit tests. Added, one entry per goal — reconciles are matched per goal, and the two goals do not expose the same parameters (testFailureIgnore is on verify only; naming a parameter a goal does not have earns a warning on every build, which I checked against help:describe for failsafe 3.3.0 rather than guessing).
The one that actually bites. Writing the failsafe entries made me test the surefire block properly instead of assuming it, and it does not cover -Dmaven.test.skip=true. The parent POM pins <skip>${skipUTs}</skip>, and explicit plugin configuration wins over the user property, so surefire's skip parameter reads identically whether the flag is there or not — and the module checksum comes out byte-identical. What the flag actually suppresses is test compilation.
On xtable-api, Temurin 11, before the fix:
$ ./mvnw clean install -pl xtable-api -Dmaven.test.skip=true # 0 tests run, entry saved
$ ./mvnw clean install -pl xtable-api
[INFO] Found cached build, restoring org.apache.xtable:xtable-api from cache by checksum 70d4788128782ec7
A plain install restoring a build whose tests never ran, reporting success. After adding a maven-compiler-plugin:testCompile reconcile on skip:
[INFO] Plugin parameter mismatch found. Parameter: skip, expected: true, actual: false
[INFO] Mojo cached parameters mismatch with actual, forcing full project build. Mojo: compiler:testCompile
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 - in org.apache.xtable.spi.extractor.TestExtractFromSource
...
Plain-to-plain still restores, so the guard costs nothing in CI. This is a local-developer hazard rather than a CI one today — CI writes the cache only from the plain install on main, and the release job disables the cache outright — but it is precisely the failure mode the reconcile block exists to prevent, and it was open.
buildinfo.xml now records all nine failsafe parameters as tracked="true" with no Cannot find a Mojo parameter warnings.
Failsafe is bound in the parent POM to integration-test and verify, and
CI runs `install`, so integration tests run on every build and a
restored module skips them exactly as it skips unit tests. Reconciles
are matched per goal, and the two goals expose different parameters:
testFailureIgnore is on verify only, and naming a parameter a goal does
not have earns a warning on every build.
The compiler entry closes a hole the surefire block does not reach.
-Dmaven.test.skip=true never gets to surefire here, because the parent
POM pins <skip>${skipUTs}</skip> and explicit plugin configuration wins
over the user property, so surefire's skip parameter reads identically
either way and the module checksum is byte-identical across the two.
What the flag actually suppresses is test compilation.
Verified on xtable-api, Temurin 11: seeding the cache with
-Dmaven.test.skip=true and then running a plain `clean install`
restored the untested build before this commit, and after it logs
"Plugin parameter mismatch found. Parameter: skip, expected: true,
actual: false" and runs the tests. A plain build still restores a plain
build, and buildinfo.xml records all nine failsafe parameters as
tracked="true" with no "Cannot find a Mojo parameter" warnings.
The JDK is in these keys because the build cache extension does not hash it, and the two workflows have to agree on the key or the reader stops finding what the writer saved. Four literal "jdk11" strings across two files could not express that; bumping actions/setup-java's java-version would have left them asserting a JDK that is no longer in use.
"License Check has to resolve every dependency in order to inspect each jar's license" is not true, and the previous commit made it load- bearing. apache-rat:check declares no dependency resolution; dependency:tree collects poms rather than jars; and the shaded-bundle step names three modules and reaches only their -am closure. -am makes dependencies, not dependents, so the two leaves are outside it: nothing in the job resolves xtable-service's Quarkus stack, xtable-utilities' bundled dependencies, or maven-install-plugin, which only an `install` pulls. Maven CI Build is restore-only by construction, so it cannot fill that gap itself - it would re-download the delta from Central on every run, permanently. dependency:go-offline over the full reactor covers what the license steps do not. It runs only on main, where the save happens, and its failure must not fail a license check: an incomplete entry costs download time on the next run and nothing else. Verified: `./mvnw -B -ntp -q dependency:go-offline -Dmaven.build.cache.enabled=false` exits 0 across the reactor.
The save gated on `size_mb < 100` while the warning fired above 100, so
a directory of exactly 100 MB - reachable, du -sm rounds up - was
skipped silently, the one outcome the guard exists to make visible. It
also treated the `${size_mb:-0}` fallback for a missing directory as a
pass, handing actions/cache/save a path that is not there.
Saving is now also conditional on the exact key having missed on
restore. Cache entries are immutable, so a re-run of a main commit would
try to write over its own entry and log "Unable to reserve cache with
key" - the message the earlier commits removed by giving the two jobs
separate keys. The repository cache save already carries this check.
|
Pushed four commits after a self-review pass. One of them corrects a claim I made in this PR's own description, so flagging it rather than burying it in a force-push.
Verification, all on Temurin 11: the before/after and negative control for the reconcile are in the thread; |
3 participants
What is the purpose of the pull request
.mvn/extensions.xmlenablesmaven-build-cache-extension, but the cache never survives a CI run, so it currently costs us the bookkeeping and buys nothing. Fixing that turned up two further problems with how CI caches Maven state, both addressed here.Reviewing this commit-by-commit is easier than as a single diff.
Brief change log
1. Persist the build cache —
actions/setup-java'scache: mavencovers~/.m2/repository; the extension writes to~/.m2/build-cache, which nothing preserved. Every run loggedLocal build was not found by checksumfor all ten modules and rebuilt everything, including for PRs touching only the website or a single leaf module. Onlymainwrites an entry; PRs restore from it.2. Keep the bundled jars out of that cache — measured, one generation is 1.2 GB, and 1.17 GB of it is three bundled jars:
(times from run 31226195551 on
main, 27:11 total)The three shade modules are 97% of the size and 12% of the time; core and service are 84% of the time and about 1 MB. Saving 1.2 GB per merge into a repository-wide 10 GB budget would evict the very caches it sits beside. They opt out with the extension's per-project property, which disables both restore and save for that module.
Also in this commit:
.mvn/maven-build-cache-config.xml(maxBuildsCached=1, remote cache off, strict surefire reconciliation — command-line flags are not part of a module's checksum, so without it a build that skipped tests and one that ran them are indistinguishable); extension1.1.0→1.2.3for two invalidation fixes, MBUILDCACHE-87 and MBUILDCACHE-99; a guard that skips the save and warns above 100 MB;-Dmaven.build.cache.enabled=falseon the release publish, sinceversions:setruns immediately beforehand and cache restoration is version-agnostic; the JDK in the cache key, as the extension does not hash it; andtimeout-minutes30 → 40, since a cold build is already 27:11 against it.3 and 4. One writer for
~/.m2/repository— both jobs usedcache: maven, which restores on an exact key match only; setup-java sets no restore-keys deliberately (#269), so any pom change made every job re-download the whole dependency tree. Same key in both jobs also meant whichever finished first reserved it and the other's save failed — License Check takes ~2 min against the build's 27, so the build loggedUnable to reserve cache with keyon every run.That outcome is the right one, so this makes it explicit rather than racing for it: License Check must resolve every dependency to inspect each jar's license, and it stops at
package, so it populates exactly what the other jobs need without ever putting this project's own artifacts in the entry. It is now the sole writer; Maven CI Build restores read-only. A prefix fallback warms from the previous entry after a pom change, and both jobs clear~/.m2/repository/org/apache/xtablebefore the post-job save, as Source Build Check already does.Verify this pull request
Build/CI infrastructure only; no production code touched.
Verified locally on Temurin 11 that the build-cache change does what it claims:
BUILD SUCCESSBUILD SUCCESS~/.m2/build-cacheCache is explicitly disabled on project levelbuildinfo.xmlconfirms the four surefire flags recorded astracked="true".Two notes for reviewers. The extension bump renames the cache directory
v1→v1.1, and this PR touches poms andextensions.xml, which rotates the repository cache key — so the first run after merge is cold on both counts, once. And a cache-restoredxtable-servicedoes not reproducetarget/quarkus-app; nothing consumes that directory today, and there is a comment in the workflow saying so.The saving depends on what a PR touches: a change to
xtable-corestill rebuilds and retests core and everything downstream, while a website, workflow, or docs-only change should hit on everything cacheable.