Cargo build caching by bchuo · Pull Request #719 · project-dalec/dalec

bchuo · 2025-07-24T14:45:20Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #612

Special notes for your reviewer:

Copilot

Pull Request Overview

This pull request introduces Cargo build caching functionality to improve Rust compilation performance by implementing sccache (Shared Compilation Cache) support across various targets and platforms.

Adds automatic Cargo cache injection when Rust dependencies are detected
Implements sccache-based caching for Rust/Cargo builds with cross-platform support
Integrates test coverage for the new Cargo caching functionality

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
cache.go	Defines CargoBuildCache types, constants, and sccache installation logic
generator_cargohome.go	Updates cargo home generation to support sccache binary caching
docs/spec.schema.json	Adds schema definitions for the new CargoBuildCache configuration
targets/windows/handle_zip.go	Implements Windows-specific cargo cache setup and sccache environment
targets/linux/deb/distro/pkg.go	Adds cargo cache support for Debian packaging with alternative Rust paths
targets/linux/rpm/distro/pkg.go	Implements cargo cache support for RPM packaging
packaging/linux/deb/debroot.go	Sets up sccache environment in Debian build scripts
packaging/linux/rpm/rpmbuild.go	Integrates cargo cache setup in RPM build process
test/cache_test.go	Adds comprehensive test cases for cargo build caching
test/linux_target_test.go	Adds cargo cache test integration for Linux targets
test/windows_test.go	Adds cargo cache test and package override for Windows
helpers.go	Adds HasRust helper function to detect Rust dependencies
load_test.go	Updates validation error messages to include cargobuild
targets/linux/deb/debian/bullseye.go	Updates Debian archive URL (unrelated change)

targets/windows/handle_zip.go

cache.go

targets/linux/deb/distro/pkg.go

generator_cargohome.go

cache.go

generator_cargohome.go

cpuguy83

This is also going to need changes to the website caching docs.

cpuguy83 · 2025-08-13T19:21:28Z

cache.go

 	BazelDefaultSocketID = "bazel-default" // Default ID for bazel socket
+
+	// SccacheVersion defines the version of sccache to install
+	SccacheVersion = "v0.10.0"


Do we need all these exported vars?

Can you unexport these values? It seems like they aren't used outside of this package.

cpuguy83 · 2025-08-13T19:25:05Z

cache.go

+		// Set up a simple environment variable to enable sccache
+		llb.AddEnv("RUSTC_WRAPPER", sccacheBinary).SetRunOption(ei)
+
+		// Create a symbolic link from /usr/local/bin/sccache to the actual binary


Comment about symbolic link doesn't seem correct since this is mounting the actual binary.

cpuguy83 · 2025-08-13T19:27:42Z

targets/linux/deb/distro/pkg.go

+	}
+
+	if addCargoCache {
+		spec.Build.Caches = append(spec.Build.Caches, dalec.CacheConfig{


This should only add a cargo cache if its not already there.

cpuguy83 · 2025-08-13T19:28:11Z

targets/linux/deb/distro/pkg.go

+		})
+	}
+
+	cargoBin, err := searchForAltRust(ctx, client, spec, targetKey, worker, opts...)


Was this needed for the change?
I know we have this for go because it was actually problematic (but unrelated to caching).

pmengelbert · 2025-08-25T19:24:30Z

Help your reviewers by using PR descriptions and commit messages with full descriptions of what was changed and why. (For future commits to this PR, don't bother going back and adding descriptions to your old commits).

cpuguy83 · 2025-09-05T17:15:50Z

cache.go

 	BazelDefaultSocketID = "bazel-default" // Default ID for bazel socket
+
+	// SccacheVersion defines the version of sccache to install
+	SccacheVersion = "v0.10.0"


Can you unexport these values? It seems like they aren't used outside of this package.

cache.go

packaging/linux/deb/debroot.go

packaging/linux/rpm/rpmbuild.go

targets/windows/handle_zip.go

cache.go

test/cache_test.go

cache.go

test/windows_test.go

cache.go

MorrisLaw · 2025-09-15T16:31:47Z

cache.go

+	// sccache target architectures
+	sccacheArchLinuxX64   = "x86_64-unknown-linux-musl"
+	sccacheArchLinuxArm64 = "aarch64-unknown-linux-musl"
+	sccacheArchLinuxArmV7 = "armv7-unknown-linux-musleabihf"


This seems to be a mismatch with what's on the release page.

Suggested change

sccacheArchLinuxArmV7 = "armv7-unknown-linux-musleabihf"

sccacheArchLinuxArmV7 = "armv7-unknown-linux-musleabi"

cache.go

helpers.go

cache.go

cpuguy83 · 2025-09-16T23:13:46Z

test/linux_target_test.go

 		testAutoGobuildCache(ctx, t, testConfig.Target)
 	})

+	t.Run("auto cargobuild cache", func(t *testing.T) {


Test naming is no longer quite right.
Also rust cache

cpuguy83 · 2025-09-16T23:13:56Z

test/cache_test.go

 	})
 }
+
+func testAutoCargobuildCache(ctx context.Context, t *testing.T, cfg targetConfig) {


Same, re: test name.

cpuguy83 · 2025-09-16T23:15:05Z

test/cache_test.go

+	}
+
+	testEnv.RunTest(ctx, t, func(ctx context.Context, client gwclient.Client) {
+		// Test that cargo cache is NOT automatically enabled (it's opt-in only now)


Comment is not neccessary, or could be updated to something along the lines of // test no cache

cpuguy83

Just one more set of minor things and this should be good to go!

cpuguy83 · 2025-09-29T20:46:43Z

test/windows_test.go

 		testAutoGobuildCache(ctx, t, tcfg)
 	})
+
+	t.Run("auto rust cache", func(t *testing.T) {


Let's rename this test, it's just testing the rust caching (no auto)

cpuguy83 · 2025-09-29T20:46:51Z

test/linux_target_test.go

 		testAutoGobuildCache(ctx, t, testConfig.Target)
 	})

+	t.Run("auto rust cache", func(t *testing.T) {


Let's rename this test, it's just testing the rust caching (no auto)

cpuguy83 · 2025-09-29T20:46:56Z

test/cache_test.go

 	})
 }
+
+func testAutoRustCache(ctx context.Context, t *testing.T, cfg targetConfig) {


Let's rename this test, it's just testing the rust caching (no auto)

bchuo force-pushed the cargo-build-caching branch 3 times, most recently from 832c52d to d136b17 Compare July 28, 2025 20:16

bchuo marked this pull request as ready for review July 29, 2025 20:10

Copilot AI review requested due to automatic review settings July 29, 2025 20:10

bchuo requested a review from a team as a code owner July 29, 2025 20:10

Copilot AI reviewed Jul 29, 2025

View reviewed changes

targets/windows/handle_zip.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

targets/linux/deb/distro/pkg.go Outdated Show resolved Hide resolved

generator_cargohome.go Outdated Show resolved Hide resolved

bchuo force-pushed the cargo-build-caching branch from 4576ec2 to 385239d Compare July 30, 2025 14:19

cpuguy83 requested changes Jul 30, 2025

View reviewed changes

generator_cargohome.go Outdated Show resolved Hide resolved

bchuo force-pushed the cargo-build-caching branch 4 times, most recently from 686ea7f to bc50a0f Compare August 5, 2025 18:29

cpuguy83 requested changes Aug 6, 2025

View reviewed changes

cache.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

cache.go Outdated Show resolved Hide resolved

generator_cargohome.go Outdated Show resolved Hide resolved

bchuo added 16 commits August 6, 2025 13:59

Add caching for cargo builds

6a1aac1

Update spec schema

f44f2c2

Fix errors

738b6e6

Actually add zip cargo build caching, and fix dep resolution issue

343f8d3

Move sccache install to dep gen phase

175b908

Fix tests

e266ee5

Ensure cargo registry structure passes through

261346b

Fix cargo issues

bcd5e18

Try another fix

d6a645d

Fix bullseye issue

f4e698a

Make sccache version consistent and parameterize constants

146fd45

Add checksums for sccache downloads

3c8debb

Use helper functions for common code in pkg.go

2d1ea00

Use SourceHttp

631600a

Fix extraction

1ec6db1

Remove pipefail

7686003

bchuo added 3 commits August 6, 2025 14:02

Update docs

0e58d92

Address comments

b05c488

Fix error

706b3a8

bchuo force-pushed the cargo-build-caching branch from 366c9fb to 706b3a8 Compare August 6, 2025 18:29

Fix subpath issue

2524101

bchuo force-pushed the cargo-build-caching branch from 0e7cb18 to 2524101 Compare August 6, 2025 19:46

bchuo added 2 commits August 7, 2025 12:21

Fix tests

6856272

Change name from CargoBuildCache to CargoSCCache

3872475

cpuguy83 requested changes Aug 13, 2025

View reviewed changes

Update comment and remove unneeded block

432fee0

cpuguy83 requested changes Sep 5, 2025

View reviewed changes

bchuo added 2 commits September 9, 2025 14:25

Remove logging statements and standardize constants used

6f414a2

Fix errors

767a726

bchuo force-pushed the cargo-build-caching branch from 54571f9 to 767a726 Compare September 9, 2025 18:57

Use mirrored jaeger

d3009c5

cpuguy83 requested changes Sep 9, 2025

View reviewed changes

cache.go Outdated Show resolved Hide resolved

test/cache_test.go Outdated Show resolved Hide resolved

Remove path updating and update test to reflect

401d8a1

cpuguy83 requested changes Sep 10, 2025

View reviewed changes

MorrisLaw reviewed Sep 15, 2025

View reviewed changes

bchuo added 2 commits September 15, 2025 15:37

Fix todo and make cache opt in

a129641

Add todo

95b5b16

cpuguy83 requested changes Sep 16, 2025

View reviewed changes

Update field to be more accurate and related references

c68fdd4

bchuo force-pushed the cargo-build-caching branch from 7e78ed8 to c68fdd4 Compare September 18, 2025 18:52

cpuguy83 requested changes Sep 29, 2025

View reviewed changes

Rename tests

d6f723a

cpuguy83 approved these changes Oct 6, 2025

View reviewed changes

cpuguy83 merged commit 48ebcd7 into project-dalec:main Oct 6, 2025
22 checks passed

cpuguy83 added this to the v0.18 milestone Oct 17, 2025

	sccacheArchLinuxArmV7 = "armv7-unknown-linux-musleabihf"
	sccacheArchLinuxArmV7 = "armv7-unknown-linux-musleabi"

Conversation

bchuo commented Jul 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 left a comment

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!

pmengelbert commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

cpuguy83 left a comment

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!

Uh oh!

Reviewers

Assignees

pmengelbert commented Aug 25, 2025 •

edited

Loading