Conversation
When sccache-dist restarts, the in-memory toolchain_dir_map is wiped but the on-disk toolchain directories survive. On the next build, prepare_overlay_dirs hits the else branch (toolchain not in the map) and calls fs::create_dir on the existing directory, which returns an AlreadyExists error and fails the build. Guard create_dir with an exists() check. The archive_id is a content hash, so a pre-existing directory for the same id already holds the correct unpacked toolchain and is safe to reuse. Signed-off-by: KK <pandeykamal13526@gmail.com>
On distros where rustc --print sysroot reports /usr (e.g. Void
Linux), the ToolchainPackager joins LIBS_DIR ("lib") to the
sysroot and packages the entire /usr/lib tree. This drags the full
system library set (several GiB) into every distributed toolchain
archive, blowing past the toolchain cache size and making toolchain
packaging prohibitively slow.
Package the rustlib/ subtree instead, which is the actual Rust
standard library. Fall back to the old behavior (whole lib dir) if
rustlib/ does not exist, preserving compatibility with non-standard
sysroot layouts.
Signed-off-by: KK <pandeykamal13526@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Missing regression tests and unresolved overlay-directory persistence prevent approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reduces Rust toolchain archive sizes and handles pre-existing overlay toolchain directories.
Changes:
- Packages
lib/rustlibwith a compatibility fallback. - Avoids errors when toolchain directories already exist.
File summaries
| File | Summary |
|---|---|
src/compiler/rust.rs |
Narrows Rust packaging to rustlib; regression tests for selection and fallback are needed. |
src/bin/sccache-dist/build.rs |
Guards existing directories; persistence behavior remains unresolved because cleanup removes them, and regression coverage is needed. |
Review details
Suppressed comments (2)
src/bin/sccache-dist/build.rs:197
- This branch is not reached for the described server-restart scenario:
OverlayBuilder::newcallscleanup()before creating these directories, andcleanup()recursively removesself.dir. A normal restart therefore deletestoolchains/<archive_id>beforeprepare_overlay_dirsruns, so this change does not provide the claimed restart persistence. Preserve the toolchain directory while cleaning only transient build data, or otherwise add the actual persistence path before relying on this guard.
if !toolchain_dir.exists() {
fs::create_dir(&toolchain_dir)?;
src/compiler/rust.rs:2296
- Because this changes the bytes produced for an unchanged compiler, the toolchain key also needs invalidation.
RustHasherbuildsweak_toolchain_keyfromCACHE_VERSIONand direct sysroot shared-library digests, whileClientToolchains::put_toolchainreuses the persistentweak_mapwithout callingwrite_pkg; existing clients will therefore keep selecting the old multi-GiB archive after upgrading. Bump the Rust cache/package version or add a packaging-version component to the weak key so this branch runs for existing caches.
let rustlib_path = libs_path.join("rustlib");
if rustlib_path.is_dir() {
package_builder.add_dir_contents(&rustlib_path)?;
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+196
to
+197
| if !toolchain_dir.exists() { | ||
| fs::create_dir(&toolchain_dir)?; |
Comment on lines
+2294
to
+2298
| let rustlib_path = libs_path.join("rustlib"); | ||
| if rustlib_path.is_dir() { | ||
| package_builder.add_dir_contents(&rustlib_path)?; | ||
| } else { | ||
| package_builder.add_dir_contents(&libs_path)?; |
Author
|
Superseded by #2850 which consolidates all fixes onto a single branch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent fixes for sccache-dist that affect Void Linux (and potentially other distros with non-standard sysroot layouts).
Fix 1: OverlayBuilder crash on persistent toolchain dirs
When sccache-dist restarts, the in-memory
toolchain_dir_mapis wiped, but the on-disk toolchain directories survive. On the next build,prepare_overlay_dirshits theelsebranch (toolchain not in the map) and callsfs::create_diron the existing directory, which returns anAlreadyExistserror and fails the build.Guard
create_dirwith anexists()check. Thearchive_idis a content hash, so a pre-existing directory for the same id already holds the correct unpacked toolchain and is safe to reuse.Fix 2: Package rustlib/ instead of whole sysroot lib dir
On distros where
rustc --print sysrootreports/usr(e.g. Void Linux), theToolchainPackagerjoinsLIBS_DIR("lib") to the sysroot and packages the entire/usr/libtree. This drags the full system library set (several GiB) into every distributed toolchain archive, blowing past the toolchain cache size and making toolchain packaging prohibitively slow.Package the
rustlib/subtree instead, which is the actual Rust standard library. Fall back to the old behavior (whole lib dir) ifrustlib/does not exist, preserving compatibility with non-standard sysroot layouts.Testing