Ability to normalize workspace member packages' versions for dependency derivation by vypxl · Pull Request #974 · ipetkov/crane

vypxl · 2026-02-09T13:20:17Z

Motivation

Resolves #445

I've come across the same problem as described in the mentioned issue, and for us, it is causing many wasted CI build minutes. That's why I set out to fix it.

This adds a new flag 'doStripVersion', which can be used to set the versions of workspace crates to a placeholder value. Doing this in the Cargo.lock and Cargo.toml's means we can reuse the dependency cache when we bump versions of our workspace crates.

Checklist

added tests to verify new behavior
added an example template or updated an existing one
updated docs/API.md (or general documentation) with changes
updated CHANGELOG.md

The source lockfile is still included if present and not overriden with either a supplied Cargo.toml or modified via doStripVersion

ipetkov · 2026-02-11T01:03:55Z

Thanks for the PR @vypxl!

The problem I ran into the last time I looked into this is the fact that cargo will refuse to reuse artifacts from two different versions of the same crate, making the version cleanup moot. With published crates this obviously makes sense (two different versions most likely have different sources) but I suspect cargo handles even workspace-members in the same way.

The real key here is that we add a compilesFresh test with this option enabled. If that passes great! But if not, the cache invalidation would make this effort moot

vypxl · 2026-02-16T10:35:52Z

I can try implementing such a test, but I'll need some more explanation. What should this test verify, specifically?

ipetkov · 2026-02-16T19:04:18Z

Search around in the test suite for existing examples of compilesFresh being used

Basically this checks that given a specific workspace and specific build flags, we can chain vendoring -> buildDepsOnly -> cargoBuild and successfully reuse the cached artifacts (namely by checking that we only rebuild the source crates themselves)

vypxl · 2026-02-17T16:46:56Z

I've looked into this now. I added two tests, one that checks this for the 'simple' project, and one for the 'strip-version-test' project. The simple test passes, so there the byteorder dependency is cached properly.

In the other one, there's a dev-dependency. The test fails here, because the check command that buildDepsOnly caches uses '--all-targets', while the compilesFresh test checks plain cargo check. If I add that flag to the test definition checks/compilesFresh.nix#L52, the test passes.

What should we do here?

P.S. In our production repo, I observed that the caching behavior with this change is.. inconsistent. It doesn't rebuild all dependencies, but a subset is rebuilt atm. Maybe a different configuration is needed there to match the cargo flags, similar to the test.

vypxl · 2026-02-20T08:30:44Z

I was able to modify our production flake to fix the weird partial caching, it was a mismatch of env variables and flags, together with missing feature unification. Now the dependencies are being reused fully, with the version stripping enabled.

ipetkov

I was able to modify our production flake to fix the weird partial caching, it was a mismatch of env variables and flags, together with missing feature unification. Now the dependencies are being reused fully, with the version stripping enabled.

Glad you sorted it out, this is exactly what I was going to suggest. Generally you want the same build flags/features/env vars to match between buildDepsOnly and any other derivations chained on top, at the very least to the extent that it affects feature unification and other cache-busting aspects

ipetkov · 2026-02-22T01:03:05Z

checks/stripVersion/sources/Cargo.toml

+serde_json = "1.0"
+
+[dev-dependencies]
+tokio-test = "0.4"


This doesn't actually seem to be used, can we replace it with a "lighter" dependency? E.g. a lot of the test cases use crates like libc and byteorder since they have no other transitive deps. If we just need to check that dev-dependencies are still being cached it shouldn't matter what they are (though the smaller we keep these the ligher our test suite is to run. We're already hitting some disk limits in the github actions runners)

ipetkov · 2026-02-22T01:06:40Z

lib/buildPackage.nix

  cargoExtraArgs ? "--locked",
  cargoTestCommand ? "cargoWithProfile test",
  cargoTestExtraArgs ? "",
+  doStripVersion ? false,


In Nix adding a foo ? default binding doesn't actually "insert" it in the args binding, its only visible in the current scope. Meaning this doesn't actually do anything as currently defined since we don't access doStripVersion anywhere in this file

Though if this is being handled by other helpers that's totally fine, we can omit it here

ipetkov · 2026-02-22T01:10:03Z

lib/buildDepsOnly.nix

  crateName = crateNameFromCargoToml argsMaybeDummySrcOverride;
+
+  # Use placeholder version when stripping versions to improve cache hit rates
+  versionPlaceholder = "0.0.0";


Thoughts on making the placeholder string something explicit like "version-placeholder" or even just a blank string?

It might be confusing to folks seeing logs and derivations with version 0.0.0 instead of their actual version, but seeing something named like *placeholder* might be a better clue

As currently written this functionality is opt-in (so using 0.0.0 is reasonable!) but if we ever decided to make this on-by-default the placeholder may matter slightly more

ipetkov · 2026-02-22T01:11:05Z

lib/buildDepsOnly.nix

+  effectiveVersion =
+    if doStripVersion then versionPlaceholder else (args.version or crateName.version);


Instead of explicitly handling this here (and in all other callers which set version), thoughts about extending crateNameFromCargoToml to understand if args.doStripVersion is set and have it inject the placeholder? Might simplify things so callers don't need to juggle it themselves

ipetkov · 2026-02-22T01:12:06Z

lib/buildTrunkPackage.nix

        // {
          CARGO_BUILD_TARGET = args.CARGO_BUILD_TARGET or "wasm32-unknown-unknown";
          doCheck = args.doCheck or false;
+          inherit doStripVersion;


We probably don't need to forward this here explicitly, we're already passing args along, so if doStripVersion is set, we'll observe it at the lower levels (i.e. we can probably revert all the changes in this file)

ipetkov · 2026-02-22T01:23:55Z

checks/default.nix

              { inherit cargoLockContents; }
              { inherit cargoLockParsed; }


Both of these specific cases now fail to evaluate with error: cannot coerce null to a string: null

ipetkov · 2026-02-22T01:26:03Z

lib/cleanCargoToml.nix

+{
+  cargoToml ? throw "either cargoToml or cargoTomlContents must be specified",
+  cargoTomlContents ? builtins.readFile cargoToml,
+  # ([String] -> Boolean)
+  cleanCargoTomlFilter ? filters.cargoTomlDefault,
+  doStripVersion ? false,
+  versionPlaceholder ? "0.0.0",
+}:


Just curious, why did this get moved? It was intentionally kept lower down the file as everything up to the filterData declaration does not depend on the actual inputs to cleanCargoToml (i.e. Nix can optimize not needing to re-evaluate all that on each invocation)

if we need a new let ... in ... binding for cleaned we can easily add it in below!

ipetkov · 2026-02-22T01:28:41Z

lib/cleanCargoToml.nix

+if doStripVersion && builtins.hasAttr "package" cleaned then
+  cleaned
+  // {
+    package = cleaned.package // {
+      version = versionPlaceholder;
+    };
+  }
+else
+  cleaned


This can probably be simplified with a mix of lib.recursiveUpdate and lib.optionalAttrs. Something like:

lib.recursiveUpdate cleaned (lib.optionalAttrs (doStripVersion && cleaned ? package) { package.version = versionPlaceholder; })

ipetkov · 2026-02-22T01:31:21Z

lib/mkDummySrc.nix

+        cleanedCargoLock = writeTOML "Cargo.lock" (
+          if doStripVersion then cleanWorkspacePackageVersions cargoLockContents else cargoLockContents
+        );


If doStripVersion is disabled this will still end up writing a Cargo.lock to the Nix store and copying from it. Can we optimize this to fall back to whatever the original value was

ipetkov · 2026-02-22T01:39:07Z

lib/mkDummySrc.nix

-  copyCargoLock = if cargoLock == null then "" else "cp ${cargoLock} $out/Cargo.lock";
+  copyCargoLock =
+    if cargoLock == null && !builtins.pathExists (origSrc + "/Cargo.lock") then
+      null


This is probably what's causing the evaluation failure above. If there's no cargoLock file we should be interpolating an empty string (i.e. nothing to do) rather than a null here.

In general you can run the test suite via nix flake check ./test# (tests are defined in a sub flake for reasons beyond this PR), or you can use ./test.sh which will run the full, comprehensive test suite

vypxl force-pushed the ignore-version-in-deps branch from 50c71f5 to e2ed5c4 Compare February 9, 2026 16:10

vypxl added 6 commits February 9, 2026 17:14

Support stripping package version from Cargo.toml files

a00d231

Support stripping workspace package versions from Cargo.lock

454bf63

fix(mkDummySrc): do not always include unmodified Cargo.lock

dffbd03

The source lockfile is still included if present and not overriden with either a supplied Cargo.toml or modified via doStripVersion

fix(mkDummySrc): do not depend on source cargo.lock

5ed2957

test: add checks for doStripVersion

874ce00

docs: docs and changelog for doStripVersion

03d96f9

vypxl force-pushed the ignore-version-in-deps branch from e2ed5c4 to 03d96f9 Compare February 9, 2026 16:26

vypxl marked this pull request as ready for review February 9, 2026 16:28

test: add compilesFresh tests for doStripVersion

b8063b1

ipetkov reviewed Feb 22, 2026

View reviewed changes

		effectiveVersion =
		if doStripVersion then versionPlaceholder else (args.version or crateName.version);

Conversation

vypxl commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Checklist

Uh oh!

ipetkov commented Feb 11, 2026

Uh oh!

vypxl commented Feb 16, 2026

Uh oh!

ipetkov commented Feb 16, 2026

Uh oh!

vypxl commented Feb 17, 2026

Uh oh!

vypxl commented Feb 20, 2026

Uh oh!

ipetkov left a comment

Choose a reason for hiding this comment

Uh oh!

ipetkov Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vypxl commented Feb 9, 2026 •

edited

Loading

ipetkov Feb 22, 2026 •

edited

Loading