ci: cache NuGet packages and npm downloads#4525
Conversation
Add cache: true + cache-dependency-path to all setup-dotnet steps to cache ~/.nuget/packages across job runs. Add cache: npm to all setup-node steps to cache the npm download cache. This eliminates redundant package downloads on every CI run. The NuGet cache key is keyed on the hash of all .fsproj files, so it invalidates automatically when dependencies change. Closes #4486 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-de479ba7cc732133
Note on NuGet caching correctnessThe For correct caching, the intended setup is:
Reference: https://www.damirscorner.com/blog/posts/20240726-CachingNuGetPackagesInGitHubActions.html This also has the benefit of reproducible builds beyond just caching. Worth considering whether we want to adopt lock files across the repo before merging this. |
7ceced2 to
c85af32
Compare
Hashing only **/*.fsproj misses version numbers defined outside project files. FSharp.Compiler.Service.fsproj interpolates $(SystemCollectionsImmutableVersion), $(FSharpCoreShippedPackageVersionValue) and six more, which are defined in .props. Bumping one of those changes the resolved package graph without touching any .fsproj, so the cache key would not move. Also resolve the merge with main, keeping setup-node@v7 alongside the new `cache: npm`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Marking ready. Expanding on why this should work, since the lock-file question in the earlier review is the main thing standing between this and merge. The mechanismThe concern was that a So the sequence on a cache hit is:
Step 2 is unconditional. A package can only be used if restore independently resolved to that exact version — at which point it's the correct version, regardless of what the key hashed. The cache is a download accelerator sitting behind resolution, not a substitute for it. That makes a mis-keyed cache a hit-rate problem, not a correctness one. Worst case is extra downloads, and it self-corrects on the next run. This is the difference from ecosystems where the lockfile is the resolution (npm, uv) — there a stale cache genuinely can change what you build. NuGet's global packages folder is content-addressed by What lock files would actually addReproducibility, not cache safety: a guarantee that this commit resolves to the same transitive graph in six months, with The one real gap, now fixedThe original key was under-specified, just not dangerously. Hashing only How to confirm it's workingCache behaviour is observable, so this doesn't have to be taken on argument. In the Actions logs:
If the second run doesn't show restores, the key is wrong and I'd rather find that here than after merge. |
The 11 setup-dotnet blocks were byte-identical, comment included. Move them into .github/actions/setup-dotnet so the cache key policy lives in one place instead of eleven copies that drift apart on the next change. Also exclude obj/ from the NuGet key and node_modules/ from the npm key. Restore generates *.nuget.g.props/.targets under obj/, so including them would make the key depend on whether the workspace had been built before — a no-op on a clean CI job, but not if a job ever restores before setup. Widen the npm key to **/package-lock.json: build.sh runs `npm install` in src/fable-standalone and src/fable-compiler-js as well, each with its own committed lockfile, so the root-only default key left those tarballs out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Caches
~/.nuget/packagesand the npm download cache across CI job runs. This eliminates redundant package downloads on every run — the repo pulls 40 distinct NuGet packages across 69 project files, re-downloaded per job today.All 11
setup-dotnetsteps now go through a composite action at.github/actions/setup-dotnet, and all 5setup-nodesteps getcache: npmwith a key covering every committed lockfile.Composite action
The 11
setup-dotnetblocks were byte-identical, comment included, so they live in one file rather than eleven copies that drift apart on the next change:This adds an ordering constraint: the composite action requires
actions/checkoutto have run first. Every job already checks out as its first step, but a future job that sets up .NET before checking out would fail with a confusing "action not found".NuGet cache key
The key hashes:
.propsand.targetsare included deliberately, not just.fsproj. Version numbers are not all declared in project files:FSharp.Compiler.Service.fsprojinterpolates$(SystemCollectionsImmutableVersion),$(FSharpCoreShippedPackageVersionValue)and six others that are defined in.props. Bumping one of those changes the resolved package graph without touching any.fsproj, so a.fsproj-only key would not move.obj/is excluded because restore generates*.nuget.g.props/.targetsthere. Those don't exist atsetup-dotnettime in a clean CI job, so this is a no-op today — but without it the key depends on whether the workspace had been built before, which stops being hypothetical if a job ever gains a restore step ahead of setup, or on a self-hosted runner with a reused workspace.npm cache key
setup-node's default hashes only the rootpackage-lock.json, butbuild.shrunsnpm installinsrc/fable-standaloneandsrc/fable-compiler-jsas well (Standalone.fs:112,CompilerJs.fs:33), each with its own committed lockfile. The root-only key left those tarballs out of the cache entirely — most visibly in thebuild-standalonejob. The key is**/package-lock.json, excludingnode_modulesfor the same workspace-cleanliness reason asobj/.On NuGet lock files
An earlier review raised that this style of key can't capture transitive version drift, and that the correct setup is
RestorePackagesWithLockFile+ committedpackages.lock.json+--locked-mode.That's a fair description of what lock files give you, but it isn't a prerequisite for this change, because the cache cannot produce a wrong build:
setup-dotnet'scache: truerestores~/.nuget/packagesbeforedotnet restoreruns — it does not short-circuit restore. Restore still resolves the graph against the feed, and for each resolved package either finds it already present locally or downloads it. For a cached package to be used, restore must have independently resolved to that exact version, in which case it is the correct one by construction.So a mis-keyed cache costs hit rate, not correctness. The failure mode is extra downloads, and it is self-correcting. That is why the keys here are widened rather than the change being blocked.
Lock files buy something real but orthogonal — reproducibility, i.e. a guarantee that this commit resolves to the same transitive graph months from now, with
--locked-modefailing loudly if it wouldn't. That's worth doing on its own merits, but it costs 69 committedpackages.lock.jsonfiles plus churn on every dependency bump, and it shouldn't gate a change whose entire benefit is not re-downloading packages. Better filed separately.Notes
!exclusions are belt-and-braces, not load-bearing.@actions/glob'shashFilessupports negation, but even if it didn't, an unsupported pattern matches nothing and contributes nothing to the hash — identical behavior either way, sinceobj/andnode_modules/are absent at that point regardless.main; the fivesetup-nodeconflicts were resolved keeping main's@v7.Closes #4486
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com