Skip to content

Build-std: Add builtin dependencies#16675

Open
adamgemmell wants to merge 9 commits intorust-lang:masterfrom
adamgemmell:dev/adagem01/opaque
Open

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

Conversation

@adamgemmell
Copy link
Copy Markdown

@adamgemmell adamgemmell commented Feb 25, 2026

View all comments

This introduces a builtin source, the concept of opaque dependencies and implicit builtin dependencies and moves the -Zbuild-std implementation to use them, as part of rust-lang/rfcs#3875. While not strictly required for build-std=always, resolver support for builtin dependencies is required for the explicit builtin dependencies RFC and I wanted to ensure the implementation moving forward had that RFC in mind.

  • A builtin source is one which Cargo innately knows how to find, such as one included with the toolchain.
  • An opaque dependency isn't yet fully agreed upon, but is a possible future feature that "hides" transitive dependencies from the main resolve in order to reduce rebuilds. The build-std=always RFC makes reference to them. Currently, all opaque dependencies are builtin dependencies and vice versa.
  • An implicit builtin dependency is one that is injected by Cargo when there are no explicit builtin dependencies present on a package, for backwards compatibility reasons. Cargo injects implicit builtin dependencies for all non-build packages when -Zbuild-std is enabled.

This PR does this by:

  • Discovering library workspace members, and registering them as Sources to be queried by the PackageRegistry later.
  • Creating a default set of opaque, builtin dependencies from these members, and injects them in the RegistryQueryer.
  • The registry, when queried for a package satisfying one of these dependencies, returns builtin Summary. This package has no dependencies, because the dependency injected is opaque. This allows the resolve to succeed as normal without modifying the resolver itself. The intended behaviour, though 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 the dummy Summarys with roots from the separate std resolve. Dependencies of the roots are then added afterwards.

This PR will inevitably break some behaviour of -Zbuild-std, and fixing that will come in future PRs (as will adding tests for it). A fair enough of that behaviour probably worked by accident. -Zbuild-std-features still works as I haven't touched the main std resolve and take Units directly from that. 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.

At the time of writing, I'd particularly like to check behaviour with multiple targets and no_std targets before merging.

@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
Copy link
Copy Markdown
Member

davidtwco commented Feb 26, 2026

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
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.

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
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

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
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
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
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/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
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()

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
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
);
if name == "compiler_builtins" {
Self::from_url(&format!(
"builtin+{path}compiler-builtins/compiler-builtins"
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 the special case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's the only direct dependency that isn't directly in the library folder. We need a better way of discovering packages for sure.

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
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

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
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.

@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
Copy link
Copy Markdown
Author

@adamgemmell adamgemmell left a comment

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
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
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
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
);
if name == "compiler_builtins" {
Self::from_url(&format!(
"builtin+{path}compiler-builtins/compiler-builtins"
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's the only direct dependency that isn't directly in the library folder. We need a better way of discovering packages for sure.

Comment thread src/cargo/ops/resolve.rs Outdated
keep_previous: Option<Keep<'_>>,
specs: &[PackageIdSpec],
register_patches: bool,
inject_builtins: bool,
Copy link
Copy Markdown
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.

@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
Copy link
Copy Markdown
Author

@adamgemmell adamgemmell left a comment

Choose a reason for hiding this comment

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

I've addressed these comments. Excuse my big force-push, but I think the patch is easier to review with this commit structure

View changes since this review

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
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

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

if dep.is_opaque() {
Copy link
Copy Markdown
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()

Comment thread src/cargo/core/resolver/dep_cache.rs Outdated
None
} else {
Some(builtins.iter())
};
Copy link
Copy Markdown
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/ops/fix/mod.rs Outdated
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
dry_run,
true,
Copy link
Copy Markdown
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

@adamgemmell adamgemmell changed the title WIP: Builtin/opaque dependencies Build-std: Add builtin dependencies May 7, 2026
@adamgemmell adamgemmell marked this pull request as ready for review May 7, 2026 16:03
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 7, 2026

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ehuss, @epage, @weihanglo
  • @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
  • Random selection from ehuss, epage, weihanglo

@adamgemmell adamgemmell force-pushed the dev/adagem01/opaque branch from 8607d8c to 65e2e17 Compare May 8, 2026 16:55
@rustbot rustbot added the A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. label May 8, 2026
@adamgemmell
Copy link
Copy Markdown
Author

Something I've noticed is that we may need to resolve std multiple times - if one supports std, the requested std (or test) crate in the resolve will enable features (like the proposed external-mem compiler-builtins` one) and crates that might not build on a no-std platform.

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 S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants