Repository navigation
feat(builtin-dep): Add Builtin Source #17567
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
c7367fd
cdb9142
04673d1
7b77571
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| use crate::{ | ||
| CargoResult, GlobalContext, | ||
| compiler::{CompileKind, RustcTargetData, standard_lib::detect_sysroot_src_path}, | ||
| sources::{ | ||
| IndexSummary, RecursivePathSource, | ||
| source::{MaybePackage, QueryKind, Source}, | ||
| }, | ||
| workspace::{Dependency, Package, PackageId, SourceId}, | ||
| }; | ||
|
|
||
| /// A builtin source represents standard library packages used in build-std, which are "built into" | ||
| /// the toolchain. | ||
| /// | ||
| /// It's a thin wrapper around a [`RecursivePathSource`] located in the sysroot's library source | ||
| /// path. | ||
| pub struct BuiltinSource<'gctx> { | ||
| /// The unique identifier for this source | ||
| source_id: SourceId, | ||
| /// The underlying path source which discovers packages | ||
| path_source: RecursivePathSource<'gctx>, | ||
| } | ||
|
|
||
| impl<'gctx> BuiltinSource<'gctx> { | ||
| pub fn new(source_id: SourceId, gctx: &'gctx GlobalContext) -> CargoResult<Self> { | ||
| let target_data = RustcTargetData::new(gctx, None, &[CompileKind::Host])?; | ||
| let path = detect_sysroot_src_path(&target_data)?; | ||
|
Contributor
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 seems overkill for users of builtin-deps without build-std and not needed for this patch series until we get to the build-std part.
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. It's not technically needed even with build-std, though I suspect it will improve error messages when doing some things intended to be unstable like depending on unstable crates or enabling features. The main reason I went down this route is that there's several fields which don't have sensible values and which could cause issues in the future or for users of Cargo as a library. For example, there's no valid value for the path to the manifest, and emitting an empty feature map would cause the resolver to fail on enabling features unless we hardcode it to work around that. This design performs worse, but I feel it's significantly simpler to work with.
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. To be clear, I'm happy going either way if you think returning synthetic packages is worth the complexity tradeoff. If we have to lie about anything significant, like the manifest path, I can rework We would also have to be comfortable with crate metadata (authors etc) possibly being different to the real package. Another thing to be aware of with this patch in it's current state is that resolving builtin dependencies would rely on
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. Small bump on this. Would you be ok with going this approach for now? I'll add a note to the tracking issue to revisit this when more features are implemented so we have more of an idea of what may break.
Contributor
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. I would recommend we start with synthetic and then adapt as needed.
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. After digging into this, I think the main user-facing concern with this approach is the fact that the manifest path is part of the cargo metadata json output:
Contributor
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. Ouch. However, in thinking through some trade offs of which route to go got me thinking, doesn't this require
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. Looking up real packages would require rust-src yes, but we plan on making it a default component. This is an unresolved question, though future RFCs plan to make "match-profile" the default for build-std which will make that change almost mandatory if it hasn't been done earlier. There's also a significant amount of churn involved just in making manifest_path() and root() return an Option, with about 50 callers each, which I'm currently working through. Personally I'm a bit worried about making the change to synthetic packages while most of the behaviour of builtins is untested.
Contributor
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. I had missed that it would be a default component. I think there is a
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. Yeah, looks like the github runners install the minimal profile. Lets try to avoid it until necessary then. |
||
| let path_source = RecursivePathSource::new(&path, source_id, gctx); | ||
| Ok(Self { | ||
| source_id, | ||
| path_source, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait::async_trait(?Send)] | ||
| impl<'gctx> Source for BuiltinSource<'gctx> { | ||
| /// All builtin dependencies are opaque, so this will return a summary without any dependencies when queried | ||
| async fn query( | ||
| &self, | ||
| dep: &Dependency, | ||
| kind: QueryKind, | ||
| f: &mut dyn FnMut(IndexSummary), | ||
| ) -> CargoResult<()> { | ||
| if !dep.source_id().is_builtin() { | ||
| // Avoid loading packages in the path source if it's not needed | ||
| return Ok(()); | ||
| } | ||
| self.path_source.query(dep, kind, f).await | ||
| } | ||
|
|
||
| fn supports_checksums(&self) -> bool { | ||
| self.path_source.supports_checksums() | ||
| } | ||
|
|
||
| fn requires_precise(&self) -> bool { | ||
| self.path_source.requires_precise() | ||
| } | ||
|
|
||
| fn source_id(&self) -> SourceId { | ||
| self.source_id | ||
| } | ||
|
|
||
| async fn download(&self, id: PackageId) -> CargoResult<MaybePackage> { | ||
| self.path_source.download(id).await | ||
| } | ||
|
|
||
| async fn finish_download(&self, id: PackageId, data: Vec<u8>) -> CargoResult<Package> { | ||
| self.path_source.finish_download(id, data).await | ||
| } | ||
|
|
||
| fn fingerprint(&self, pkg: &Package) -> CargoResult<String> { | ||
| self.path_source.fingerprint(pkg) | ||
| } | ||
|
|
||
| fn describe(&self) -> String { | ||
| self.source_id.to_string() | ||
| } | ||
|
|
||
| fn invalidate_cache(&self) { | ||
| self.path_source.invalidate_cache(); | ||
| } | ||
|
|
||
| fn set_quiet(&mut self, quiet: bool) { | ||
| self.path_source.set_quiet(quiet); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use std::path::Path; | ||
|
|
||
| use crate::{ | ||
| GlobalContext, | ||
| sources::IndexSummary, | ||
| util::{VersionReqMatchMode, data_structures::HashMap}, | ||
| workspace::{Dependency, SourceId}, | ||
| }; | ||
|
|
||
| #[test] | ||
| fn builtin_source() { | ||
| let mut gctx = GlobalContext::default().unwrap(); | ||
| let mock_std_root = | ||
| Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/testsuite/mock-std/library"); | ||
| let env = HashMap::from_iter([( | ||
| "__CARGO_TESTS_ONLY_SRC_ROOT".to_string(), | ||
| mock_std_root.display().to_string(), | ||
| )]); | ||
| gctx.set_env(env); | ||
|
|
||
| let source_id = SourceId::for_builtin().unwrap(); | ||
| let dep = Dependency::new_override("core".into(), source_id); | ||
|
|
||
| let source = dep.source_id().load(&gctx).unwrap(); | ||
| let results = | ||
| crate::util::block_on(source.query_vec(&dep, crate::sources::source::QueryKind::Exact)) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(results.len(), 1); | ||
| let result = results[0].clone(); | ||
| if let IndexSummary::Candidate(s) = result { | ||
| assert!(dep.matches(&s, VersionReqMatchMode::Default)); | ||
| assert!(s.dependencies().is_empty()); | ||
| assert!(s.source_id().is_builtin()) | ||
| } else { | ||
| panic!("no candidate found"); | ||
| }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| use crate::context; | ||
| use crate::sources::registry::CRATES_IO_HTTP_INDEX; | ||
| use crate::sources::source::Source; | ||
| use crate::sources::{CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY, DirectorySource}; | ||
| use crate::sources::{ | ||
| BuiltinSource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY, DirectorySource, | ||
| }; | ||
| use crate::sources::{GitSource, PathSource, RegistrySource}; | ||
| use crate::util::data_structures::HashSet; | ||
| use crate::util::interning::InternedString; | ||
|
|
@@ -395,6 +397,11 @@ impl SourceId { | |
| matches!(self.inner.kind, SourceKind::Git(_)) | ||
| } | ||
|
|
||
| /// Returns `true` if this source is builtin. | ||
| pub fn is_builtin(self) -> bool { | ||
| matches!(self.inner.kind, SourceKind::Builtin) | ||
| } | ||
|
|
||
| /// Creates an implementation of `Source` corresponding to this ID. | ||
| pub fn load<'a>(self, gctx: &'a GlobalContext) -> CargoResult<Box<dyn Source + 'a>> { | ||
| trace!("loading SourceId; {}", self); | ||
|
|
@@ -411,7 +418,7 @@ impl SourceId { | |
| } | ||
| Ok(Box::new(PathSource::new(&path, self, gctx))) | ||
| } | ||
| SourceKind::Builtin => todo!("builtin source"), | ||
| SourceKind::Builtin => Ok(Box::new(BuiltinSource::new(self, gctx)?)), | ||
|
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 which source to use: #16675 (comment) Since that comment I've moved to using a separate Builtin source which wraps a RecursivePathSouce. |
||
| SourceKind::Registry | SourceKind::SparseRegistry => { | ||
| Ok(Box::new(RegistrySource::remote(self, gctx)?)) | ||
| } | ||
|
|
||
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.
The decision to discover real packages here is an interesting one. I'm not sure it's technically necessary, but it certainly feels safer than creating synthetic Packages and Summarys especially for some unstable features like depending on crates other than core, alloc and std.
The build-std RFC intends to make the
rust-srccomponent mandatory, allowing us to do this, but this is an unresolved question.View changes since the review