-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(build-std): Introduce builtin packages #17396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
9dce7f7
c7d1019
584369a
5d56894
9716028
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,6 +204,14 @@ impl SourceId { | |
| SourceId::new(SourceKind::Path, url, None) | ||
| } | ||
|
|
||
| /// Creates a `SourceId` from a filesystem path representing a builtin package. | ||
| /// | ||
| /// `path`: an absolute path. | ||
| pub fn for_builtin(path: &Path) -> CargoResult<SourceId> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 url = path.into_url()?; | ||
| SourceId::new(SourceKind::Builtin, url, None) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 We'd then add a sanity check in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| /// Creates a `SourceId` from a filesystem path. | ||
| /// | ||
| /// `path`: an absolute path. | ||
|
|
@@ -345,6 +353,11 @@ impl SourceId { | |
| self.inner.kind == SourceKind::Path | ||
| } | ||
|
|
||
| /// Returns `true` if this source is built into Cargo | ||
| pub fn is_builtin(self) -> bool { | ||
| self.inner.kind == SourceKind::Builtin | ||
| } | ||
|
|
||
| /// Returns the local path if this is a path dependency. | ||
| pub fn local_path(self) -> Option<PathBuf> { | ||
| if self.inner.kind != SourceKind::Path { | ||
|
|
@@ -403,6 +416,7 @@ impl SourceId { | |
| } | ||
| Ok(Box::new(PathSource::new(&path, self, gctx))) | ||
| } | ||
| SourceKind::Builtin => todo!("builtin source"), | ||
| SourceKind::Registry | SourceKind::SparseRegistry => { | ||
| Ok(Box::new(RegistrySource::remote(self, gctx)?)) | ||
| } | ||
|
|
@@ -663,6 +677,7 @@ impl fmt::Display for SourceId { | |
| Ok(()) | ||
| } | ||
| SourceKind::Path => write!(f, "{}", url_display(&self.inner.url)), | ||
| SourceKind::Builtin => write!(f, "builtin {}", url_display(&self.inner.url)), | ||
| SourceKind::Registry | SourceKind::SparseRegistry => { | ||
| write!(f, "registry `{}`", self.display_registry_name()) | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
View changes since the review