Skip to content

feat(builtin-deps): Add SourceKind::Builtin - #17513

Merged
weihanglo merged 1 commit into
rust-lang:masterfrom
adamgemmell:dev/builtins/sourcekind
Sep 25, 2026
Merged

weihanglo merged 1 commit into
rust-lang:masterfrom
adamgemmell:dev/builtins/sourcekind

Conversation

@adamgemmell

@adamgemmell adamgemmell commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

View all comments

What does this PR try to resolve?

As part of #16960 this PR adds a new variant of SourceKind to represent Builtin packages and integrates it into explicit builtin dependencies. This allows resolution to proceed slightly further, but is now blocked a Source capable of returning Packages and Summaries for builtin packages.

A few notes:

  • A previous version of this PR bumped the version of cargo-util-schemas, but this has already been done this release cycle.
  • It's not quite clear what the behaviour of parsing/displaying URLs related to SourceIds should be, and I'm going to defer that to when I handle user-facing behaviour that requires it.
  • There's improvements possible to the way we fetch the standard library source path. We have a task on the goal's work plan to handle this later when the core implementation for Builtins is a bit more settled. For now I think this solution is reasonable.

How to test and review this PR?

Commit by commit - all commits pass tests. Existing user-facing behaviour is limited, but one test is updated to show the difference.

🤖 LLM disclosure: I used Codex to help me understand the codebase and asked it to review my branch before sharing it upstream. I also used it in limited ways to generate code, including resolving merge conflicts and updating test assertions. All code was originally written by hand.

r? @Muscraft (we're trying to share out these reviews a bit, but I don't mind who reviews)

@rustbot rustbot added A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-dependency-resolution Area: dependency resolution and the resolver A-manifest Area: Cargo.toml issues A-testing-cargo-itself Area: cargo's tests Command-clean Command-fetch Command-fix Command-metadata Command-tree S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2026
Comment thread src/workspace/source_id.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
/// A directory-based registry.
Directory,
/// Package sources distributed with the rust toolchain
Builtin,

@adamgemmell adamgemmell Sep 24, 2026 •

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.

Existing comment regarding updating package_id_spec: #16675 (comment)

I'll leave that for a future PR.

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it. I would expect it being an immediate follow-up though as it is also related to how we encode this in lockfile, in cargo metadata output, etc. All of those may affect how we snapshot our tests. I don't mind if we have a simple encode logic first.

Comment thread src/workspace/source_id.rs Outdated
@adamgemmell
adamgemmell force-pushed the dev/builtins/sourcekind branch from 81b2676 to cec0abc Compare September 24, 2026 16:30
Comment thread crates/cargo-util-schemas/src/core/source_kind.rs
/// A directory-based registry.
Directory,
/// Package sources distributed with the rust toolchain
Builtin,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it. I would expect it being an immediate follow-up though as it is also related to how we encode this in lockfile, in cargo metadata output, etc. All of those may affect how we snapshot our tests. I don't mind if we have a simple encode logic first.

Comment thread src/compiler/build_context/target_info.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
Comment thread src/workspace/parser/mod.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
Comment thread src/workspace/source_id.rs Outdated
let mut path = BUILTIN_SRC_PATH.lock().unwrap();
if path.is_none() {
let target_data = RustcTargetData::new(gctx, None, &[CompileKind::Host])?;
*path = Some(detect_sysroot_src_path(&target_data)?);

@weihanglo weihanglo Sep 25, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unsurprisingly, parsing lockfile now requires rust-src rustup compoment present, which fails at manifest parsing in a couple of commands, including cargo metadata --no-deps, cargo pkgid, and cargo rm. I don't think this is ideal but also not blocking if we have plan to revisit.

However, I think there is a fudamental question around builtin source identity: whether we should embed absolute path to sysroot in SourceId. I didn't see why this is required at this point when reviewing this PR. Any future features that motivated this?

View changes since the review

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 path seems more a property of the Source than the SourceId, much like the IP address used for a registry is a property of the Source while the URL is a property of the SourceId.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Guess we can look at how rustc version is hashed in -Cmetadata/unit id / fingerprint, and reuse or get some inspiration from it. I remembered it is a bit messy when dealing with platform-agnostic target like wasm, for example this issue #8140

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.

Unsure how that is relevant, patching?

Shouldn't there always be a 1:1 between the sysroot and rust version so nothing else is needed? For patching, I think we track to 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.

The RFC does propose making rust-src a default component in the future, albeit with an unresolved question.

I put this here as other sources store the SourceId and use that to determine the package location. However there's no information conveyed by it when the source can just look up the sysroot later, so I've replaced this lookup with a placeholder URL.

I'm also experimenting with a different Builtin source that doesn't need the sysroot at all and returns completely synthetic packages, but this it still seems a bit risky at the moment.

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.

For other source kinds, there can be multiple unique sources. For this source kind, there is only one valid source. The sysroot location is an implementation detail of that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so I've replaced this lookup with a placeholder URL.

The placeholder could also be something similar to trim-paths rule (and rustc also does this):

Some(commit_hash) => format!("/rustc/{commit_hash}"),
None => format!("/rustc/{}", rustc.version),

@adamgemmell
adamgemmell force-pushed the dev/builtins/sourcekind branch from cec0abc to ccef5f5 Compare September 25, 2026 18:00

/// Creates a `SourceId` for the builtin packages in the configured toolchain.
pub fn for_builtin() -> CargoResult<SourceId> {
let url = "builtin://".into_url()?;

@weihanglo weihanglo Sep 25, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is probably good to move on. Just note that this affects other serialized output (lockfile and cargo metadata and probably others) and we might want to revisit this. For example,

  • Should we encode version?
    • Doing it might make lockfile less portable.
    • Not doing it mean we can not know for sure which std a lockfile use at creation time (or does it matter?)
  • Do we care about libcargo user that may have two different of builtin source?

View changes since the review

@weihanglo
weihanglo added this pull request to the merge queue Sep 25, 2026
Merged via the queue into rust-lang:master with commit f888d00 Sep 25, 2026
29 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2026
@adamgemmell
adamgemmell deleted the dev/builtins/sourcekind branch September 29, 2026 10:22
rust-bors Bot pushed a commit to rust-lang/rust that referenced this pull request Sep 30, 2026
Update cargo submodule

8 commits in 3d7cf6e937d6127d0f49881bf689c560b36d35c4..f3865b2a4d1acc5276f6b3c67d0e057f4dab3928
2026-09-25 01:47:29 +0000 to 2026-09-29 19:58:08 +0000
- fix(config): Proper dotted tuple support with legacy fallback (rust-lang/cargo#17536)
- refactor: Rename internal content from target-triple to target-tuple (rust-lang/cargo#17535)
- docs(changelog): remove duplicate items (rust-lang/cargo#17528)
- chore: bump to 0.102.0; update changelog (rust-lang/cargo#17525)
- feat(metadata): mirror package features in features_v2 (rust-lang/cargo#17517)
- feat(config): Add build.profile, install.profile (rust-lang/cargo#17215)
- feat(builtin-deps): Add `SourceKind::Builtin` (rust-lang/cargo#17513)
- fix(compilation): Preventing OUT_DIR env var from leaking into cargo run after build.rs run (rust-lang/cargo#17503)

r? ghost
rust-bors Bot pushed a commit to rust-lang/rust that referenced this pull request Sep 30, 2026
Update cargo submodule

8 commits in 3d7cf6e937d6127d0f49881bf689c560b36d35c4..f3865b2a4d1acc5276f6b3c67d0e057f4dab3928
2026-09-25 01:47:29 +0000 to 2026-09-29 19:58:08 +0000
- fix(config): Proper dotted tuple support with legacy fallback (rust-lang/cargo#17536)
- refactor: Rename internal content from target-triple to target-tuple (rust-lang/cargo#17535)
- docs(changelog): remove duplicate items (rust-lang/cargo#17528)
- chore: bump to 0.102.0; update changelog (rust-lang/cargo#17525)
- feat(metadata): mirror package features in features_v2 (rust-lang/cargo#17517)
- feat(config): Add build.profile, install.profile (rust-lang/cargo#17215)
- feat(builtin-deps): Add `SourceKind::Builtin` (rust-lang/cargo#17513)
- fix(compilation): Preventing OUT_DIR env var from leaking into cargo run after build.rs run (rust-lang/cargo#17503)

r? ghost
@rustbot rustbot added this to the 1.101.0 milestone Sep 30, 2026
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 1, 2026
Update cargo submodule

8 commits in 3d7cf6e937d6127d0f49881bf689c560b36d35c4..f3865b2a4d1acc5276f6b3c67d0e057f4dab3928
2026-09-25 01:47:29 +0000 to 2026-09-29 19:58:08 +0000
- fix(config): Proper dotted tuple support with legacy fallback (rust-lang/cargo#17536)
- refactor: Rename internal content from target-triple to target-tuple (rust-lang/cargo#17535)
- docs(changelog): remove duplicate items (rust-lang/cargo#17528)
- chore: bump to 0.102.0; update changelog (rust-lang/cargo#17525)
- feat(metadata): mirror package features in features_v2 (rust-lang/cargo#17517)
- feat(config): Add build.profile, install.profile (rust-lang/cargo#17215)
- feat(builtin-deps): Add `SourceKind::Builtin` (rust-lang/cargo#17513)
- fix(compilation): Preventing OUT_DIR env var from leaking into cargo run after build.rs run (rust-lang/cargo#17503)

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

Labels

A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-dependency-resolution Area: dependency resolution and the resolver A-manifest Area: Cargo.toml issues A-testing-cargo-itself Area: cargo's tests Command-clean Command-fetch Command-fix Command-metadata Command-tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants