Skip to content

Build-std: Add builtin dependencies - #16675

Open
adamgemmell wants to merge 21 commits into
rust-lang:masterfrom
adamgemmell:dev/adagem01/opaque
Open

Build-std: Add builtin dependencies#16675
adamgemmell wants to merge 21 commits into
rust-lang:masterfrom
adamgemmell:dev/adagem01/opaque

Conversation

@adamgemmell

@adamgemmell adamgemmell commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

View all comments

What does this PR try to resolve?

This PR internally introduces:

  • The builtin source, a way for Cargo to discover builtin packages
  • Opaque dependencies, the idea that a dependency shouldn't be handled by the main resolve but rather a separate resolve in order to allow things like multiple packages in the same resolve with differing minor versions. The concept has been proposed as a first-class cargo feature, but is only introduced partially here.
  • (Implicit) builtin dependencies, which are dependencies on builtin packages injected in order to allow packages to migrate to explicit builtin dependencies in the future.

It moves the -Zbuild-std implementation to use them as part of rust-lang/rfcs#3875. While not strictly required for build-std=always which is our immediate goal, I wanted to ensure the implementation moving forward had explicit dependencies in mind.

The main behaviour change in this PR is that host dependencies like build-scripts and proc_macros no longer use build-std. If this is desired we can re-introduce the behaviour in an unstable manner, but it never worked for -Zbuild-std in cross-compile mode.

This PR implements this as a whole by:

  • Registering the builtin source (lazily, when a dependency is discovered), which discovers packages with a RecursivePathSource.
  • Creating a default set of opaque implicit builtin dependencies the set of roots defined in std_crates, and injects them to every eligible crate during the resolve.
  • The builtin source, when queried for a package satisfying one of these dependencies, returns a builtin Summary which has been modified compared to the one discovered by RecursivePathSource. This package has no dependencies, because the dependency queried is opaque. This allows the resolve to succeed as normal without modifying the resolver itself. The intended behaviour, though yet untested, is to make use of all the patching/replacement logic already in the registry query.
  • Instead of the current -Zbuild-std unit generation step which attaches UnitDeps to each root of the std resolve, the main unit generation loop replaces any builtin Summarys with roots from the separate std resolve. Dependencies of the roots are then added afterwards.

This PR has a very large surface area and will inevitably change some behaviour of -Zbuild-std. In particular we need to be careful about where we expose builtin SourceIds and package specs. The intent here is to avoid disruption as build-std is a very commonly used unstable feature, but some is to be expected and will be fixed in future PRs according to the agreed RFC. The non-build-std route is well tested already and I do not expect to find regressions there as the majority of this code is gated on -Zbuild-std.

How to test and review this PR?

The PR is best read commit-by-commit - all commits are atomic and try to introduce features piecemeal where possible, though inevitably there's a big "waterfall" enabling commit in the middle. The behaviour can be manually tested locally by providing -Zbuild-std as an argument.

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-crate-dependencies Area: [dependencies] of any kind A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues A-registries Area: registries Command-fix Command-metadata Command-package Command-tree Command-update labels Feb 25, 2026
@davidtwco

davidtwco commented Feb 26, 2026

Copy link
Copy Markdown
Member

We aren't expecting that this will land prior to those RFCs being approved (unless Cargo want it to), the intent with this is just to get some feedback on the general direction for the implementation, we're happy to adjust as much or as little as required :) @adamgemmell will also be away for a couple of weeks starting next week, so we might not respond to feedback immediately.

/// A directory-based registry.
Directory,
/// Package sources distributed with the rust toolchain
Builtin,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will impact the unique identifier for the packages from this source in cargo's json output when compiling, cargo metadata, cargo <cmd> -p, etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll modify there too, and add a note to check the stdout in various use cases. The RFCs often make notes on what the output of various commands will be. Note that builtin doesn't actually appear in Units - they're all Path dependencies by that point.

An interesting point on cargo metadata is that we decided that we have an unresolved question regarding if deps of builtins should be shown on output, which will be a little hard here as they're not attached until unit generation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opted to implement pkg spec in a later PR. I've added tests for common output commands like metadata/tree. json output (from build --message-format=json) isn't impacted as builtins do not exist in the unit graph, the risk is only with commands that operate on the resolve.

Comment thread src/cargo/core/source_id.rs Outdated
.url
.to_file_path()
.expect("builtin sources should not be remote");
Ok(Box::new(PathSource::new(&path, self, gctx)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will using a PathSsource directly like this work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particularly could have interesting design questions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to a builtin source

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A builtin source now wraps a RecursivePathSource, and there's only one source for all builtin packages.

Comment thread src/cargo/core/resolver/dep_cache.rs Outdated
None
} else {
Some(builtins.iter())
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is for implicit builtins. Is there a reason you chose to do this here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source replacements happen in dep_cache (see query) so it seemed simpler just to put this here for now. After digging a little more, it might just be the deprecated [replace] section though, in which case I can probably lift this out of the cache to a more appropriate place

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in build_deps() now, which is a bit more appropriate. It could be moved up to the resolver activate function, but that's already massive, and moving it to PackageRegistry::query would involve modifying Summarys which are intended to be immutable.

Comment thread src/cargo/core/dependency.rs Outdated
}
}

pub fn new_injected_builtin(name: InternedString) -> Dependency {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you see as the role of this compared to the other news?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that opaque is true (making a SourceId was also more complicated in a previous iteration of this patch). public should also be true now that I look closer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked slightly in the new push

Comment thread src/cargo/core/registry.rs Outdated
)
})?;

if dep.is_opaque() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to find a way to ask the source for the opaque variant of the summary. The tricky thing will be that we need to work with both variants.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern works well, see the new Summary::to_opaque_builtin_summary()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked slightly again, let me know what you think

Comment thread src/cargo/core/source_id.rs Outdated
// Injecting builtins earlier (somewhere with access to RustcTargetData) is needed instead of this
let home = std::env::var("HOME").expect("HOME is set");
let path = format!(
"file://{home}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL in source ids is public facing. I think we'll need something more generic and then a new Source

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely tried something more generic but found that it had to be valid at various points. Will experiment more with this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to fix the output to remove the true file path. See https://rust-lang.github.io/rfcs/3875-build-std-explicit-dependencies.html#cargo-subcommands and cargo pkgid

Comment thread src/cargo/core/source_id.rs Outdated
Comment thread src/cargo/ops/fix/mod.rs Outdated
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
dry_run,
true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really a fan of bool constants being used in parameter lists. Makes it a lot harder to figure what what is going on here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this specific bool, but there's probably scope to improve the passing around of the builtins path and dependencies set

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked how this works signifcantly.

Comment thread src/cargo/ops/resolve.rs Outdated
keep_previous: Option<Keep<'_>>,
specs: &[PackageIdSpec],
register_patches: bool,
inject_builtins: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to tunnel this through? Not thrilled with this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bool or the required logic behind it could probably be packaged into the workspace or it's gctx

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked how this works signifcantly, see ResolverOpts

@rustbot

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Mar 6, 2026
@adamgemmell
adamgemmell force-pushed the dev/adagem01/opaque branch from 16dc1ab to ab7d8b0 Compare March 19, 2026 14:20

@adamgemmell adamgemmell left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for leaving feedback. My latest set of changes does a bit of tidying and works towards getting the tests passing, but doesn't address these comments yet or otherwise make progress towards removing hacks.

View changes since this review

Comment thread src/cargo/core/resolver/dep_cache.rs Outdated
None
} else {
Some(builtins.iter())
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source replacements happen in dep_cache (see query) so it seemed simpler just to put this here for now. After digging a little more, it might just be the deprecated [replace] section though, in which case I can probably lift this out of the cache to a more appropriate place

Comment thread src/cargo/core/dependency.rs Outdated
}
}

pub fn new_injected_builtin(name: InternedString) -> Dependency {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that opaque is true (making a SourceId was also more complicated in a previous iteration of this patch). public should also be true now that I look closer.

Comment thread src/cargo/core/source_id.rs Outdated
// Injecting builtins earlier (somewhere with access to RustcTargetData) is needed instead of this
let home = std::env::var("HOME").expect("HOME is set");
let path = format!(
"file://{home}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely tried something more generic but found that it had to be valid at various points. Will experiment more with this.

Comment thread src/cargo/core/source_id.rs Outdated
Comment thread src/cargo/ops/resolve.rs Outdated
keep_previous: Option<Keep<'_>>,
specs: &[PackageIdSpec],
register_patches: bool,
inject_builtins: bool,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bool or the required logic behind it could probably be packaged into the workspace or it's gctx

@rustbot

This comment has been minimized.

@davidtwco davidtwco mentioned this pull request Apr 14, 2026
10 tasks
@adamgemmell
adamgemmell force-pushed the dev/adagem01/opaque branch from ab7d8b0 to 8607d8c Compare May 7, 2026 15:58
@rustbot rustbot added A-directory-source Area: directory sources (vendoring) A-workspaces Area: workspaces labels May 7, 2026
pull Bot pushed a commit to Jankyboy/cargo that referenced this pull request Aug 3, 2026
### What does this PR try to resolve?

I split this out from a refactor of
rust-lang#16675, as part of work on
[build-std=always](rust-lang/rust#155363). In
this PR, and in other build-std work I have in progress, it's useful to
know the sysroot path earlier than the `compiler/` stage of Cargo.
Because there's one rustc instance per Cargo invocation, and because the
sysroot path remains fixed per rustc and isn't dependent on the target,
this should be safe to move out of TargetInfo.

### How to test and review this PR?

The first commit shows the logic being moved, and the second commit
shows updating all the users of this logic.

The third removes the actual `--print` lookup in `TargetInfo`, which
necessitated some test changes and a refactor to ensure the rustc info
cache can be written to.

Getting the sysroot is already heavily tested in Cargo, hence why I
didn't add a new test for just this.
@rustbot

rustbot commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@adamgemmell
adamgemmell force-pushed the dev/adagem01/opaque branch from 72910cc to 6bef594 Compare August 7, 2026 18:42
This commit is currently an unused API and will be tested in later
commits
generation for them

The old unit generation used to insert dependencies to every package on
the set of roots std was resolved with. The new unit generation replaces
dependencies with the relevant root of std, or ignores them if the
dependency is not needed.

This commit enables tests for the previous set of commits.
This test's assertion was completely broken because of the leading
spaces and use of `...` which doesn't work with stderr_does_not_contain.

See the comments in this test for additional details of shared
dependencies on why the test cannot be fixed right now.

It is feasibly possible for Cargo to share the user dep_test dependency
and the build-std dep_test, but this is complex as written as dep_test
gains an implicit std dependency while the mock-std depends on it. It
may be easier to share them with explicit builtin dependencies, but the
situation is complex due to -Zforce-unstable-if-unmarked and prone to
breakage due to RUSTC_BOOTSTRAP=1.
The ideal behaviour is probably to emit all packages, but currently only
the roots of std are emitted. A future PR will change this.
@adamgemmell
adamgemmell force-pushed the dev/adagem01/opaque branch from 6bef594 to 2164605 Compare August 7, 2026 19:19
@adamgemmell

Copy link
Copy Markdown
Contributor Author

CI does pass locally, the resolver-tests failure is because they need mock-std. I'll fix that monday then rustbot ready then, but it's reviewable now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area: Command-line interface, option parsing, etc. A-crate-dependencies Area: [dependencies] of any kind A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-lockfile Area: Cargo.lock issues A-registries Area: registries A-workspaces Area: workspaces Command-add Command-fix Command-metadata Command-package Command-tree Command-update Command-vendor S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants