feat(build-std): Introduce builtin packages - #17396
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| /// Creates a `SourceId` from a filesystem path representing a builtin package. | ||
| /// | ||
| /// `path`: an absolute path. | ||
| pub fn for_builtin(path: &Path) -> CargoResult<SourceId> { |
There was a problem hiding this comment.
Should we do path.is_absolute() check here?
There was a problem hiding this comment.
This is already checked by into_url() in this function. Should be enough unless you think we need to canonicalize here?
I've added an e2e test with a relative source path that does a conversion in detect_sysroot_src_path which I'll post in a separate PR.
There was a problem hiding this comment.
Actually nevermind, I tried that with a real workflow and overriding the sysroot with a relative source path doesn't work without build-std anyway. The into_url() check should be sufficient.
| let reg = registry(vec![pkg!(core)]); | ||
|
|
||
| assert_same(&res, &names(&["root", "core"])); | ||
| // No way to specify builtin deps - fails |
There was a problem hiding this comment.
Why do we comment them out instead of asserting the failure, if any?
There was a problem hiding this comment.
I've done this for behaviour that's unimplemented as the failure case is basically random and annoying to assert on sometimes. Real fixes should assert properly.
There was a problem hiding this comment.
To elaborate a bit more, this makes rebasing easier for me and I think reads easier for the reviewer too, but let me know if you'd prefer otherwise
| /// `path`: an absolute path. | ||
| pub fn for_builtin(path: &Path) -> CargoResult<SourceId> { | ||
| let url = path.into_url()?; | ||
| SourceId::new(SourceKind::Builtin, url, None) |
There was a problem hiding this comment.
What about fn stable_hash and impl Hash for builtin sources?
Currently in this PR, we treat builtin sources unique if they comes from different paths. However, what if they actually come form the same Rust version?
BTW, stable_hash is more for serialization and and cross-machine reproducibility , and Hash impl is for internal runtime uniqueness
There was a problem hiding this comment.
The intention is there's only ever one Url that builtin SourceIds are constructed with and so only one builtin source. If we can ensure this then implementation is simpler, and we cut down the RFC scope a bit to ensure assumptions like this will be possible.
I'm not sure how best to enforce that in code however other than something awkward like caching a Builtin SourceId in the gctx and ensuring we only use that. We could modify impl Hash to not hash URLs if the kind is builtin, but I'm worried that would just mask when this assumption is broken.
There was a problem hiding this comment.
Just for a bit of context not included in this PR, the packages returned by the builtin source are not built - the real Units will still come from the separate std resolve, which in theory can support patching, source replacement or whatever else the main resolve currently supports.
To ensure there's only ever one Builtin source loaded I could modify impl Hash, Eq, Ord etc to not consider the URL for builtins.
We'd then add a sanity check in PackageRegistry::ensure_loaded() whenever the cached SourceId's Url differs from the one we're trying to load, but I'm not sure what could go wrong in the meantime before this check happens.
|
BTW, if there is any prior discussion, would be great if you can share the specific comment links :) |
bea94ff to
77c8c93
Compare
This comment has been minimized.
This comment has been minimized.
| /// A directory-based registry. | ||
| Directory, | ||
| /// Package sources distributed with the rust toolchain | ||
| Builtin, |
There was a problem hiding this comment.
Existing comment from @epage regarding updating package_id_spec.rs #16675 (comment)
I opted to implement pkg id spec input/output in a separate PR after this sequence. We don't expect to see builtin pkg ids in cargo's output as they're not present in the resolve, and will be filtered from cargo metadata (see discussion at #16675 (comment))
| /// `path`: an absolute path. | ||
| pub fn for_builtin(path: &Path) -> CargoResult<SourceId> { | ||
| let url = path.into_url()?; | ||
| SourceId::new(SourceKind::Builtin, url, None) |
There was a problem hiding this comment.
Existing comment from @epage regarding the fact that the URL in source ids is public facing: #16675 (comment)
Since then the URL used is a little more generic, and later work on the pkg id format will hide this further. The build-std goal task plan involves a thorough look at the output of each subcommand, so if this URL leaks later I expect to catch it.
77c8c93 to
9716028
Compare
|
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. |
What does this PR try to resolve?
This PR, part of a stack split off from #16675, adds a new
SourceKindvariant called builtin, which will be used to represent builtin packages which will be used in the implementation ofbuild-std=alwaysas mentioned in the explicit builtin dependencies RFC. Later, dependencies on builtin packages will be able to be specified as part of the rest of the latter RFC.This PR's scope is limited to what's needed in order for the resolver to satisfy dependencies on builtins given a registry which already knows about builtin packages, though no actual resolver changes are needed for this.
How to test and review this PR?
The PR introduces unit tests for all new behaviour and I recommend reading the commits in order. This is all dead code under Cargo's normal operation and there's no way to invoke cargo that uses
SourceKind::Builtinat this point.There is a very large amount of context behind build-std. Please feel free to spam me with questions and I can link to relevant bits of this context than reviewers needing to reread this context every time you need to review a build-std PR if you prefer.
This PR is part of a stack: