-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Build-std: Add builtin dependencies #16675
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
78e7d26
5bfab35
3b87c7c
1bee864
e7473c1
c78fd63
c7483da
038ad3a
703e41d
984988b
f8cc6c0
6e57738
a734a4f
8b48454
e5fc4a9
13f0c62
c640bb9
a48a9d0
a288440
4e9354a
6d6fe8f
12d0b08
08574b8
cc1e02b
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 |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ pub enum SourceKind { | |
| LocalRegistry, | ||
| /// A directory-based registry. | ||
| Directory, | ||
| /// Package sources distributed with the rust toolchain | ||
| Builtin, | ||
|
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. https://github.com/rust-lang/cargo/blob/master/crates/cargo-util-schemas/src/core/package_id_spec.rs is at least one other place that would need updating
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 will impact the unique identifier for the packages from this source in cargo's json output when compiling,
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. 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 An interesting point 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. I've opted to implement pkg spec input/output in a later PR. I've added tests for common output commands like metadata/tree. json output (from |
||
| } | ||
|
|
||
| // The hash here is important for what folder packages get downloaded into. | ||
|
|
@@ -40,6 +42,7 @@ impl SourceKind { | |
| SourceKind::SparseRegistry => None, | ||
| SourceKind::LocalRegistry => Some("local-registry"), | ||
| SourceKind::Directory => Some("directory"), | ||
| SourceKind::Builtin => Some("builtin"), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -71,6 +74,10 @@ impl Ord for SourceKind { | |
| (_, SourceKind::Directory) => Ordering::Greater, | ||
|
|
||
| (SourceKind::Git(a), SourceKind::Git(b)) => a.cmp(b), | ||
| (SourceKind::Git(_), _) => Ordering::Less, | ||
| (_, SourceKind::Git(_)) => Ordering::Greater, | ||
|
|
||
| (SourceKind::Builtin, SourceKind::Builtin) => Ordering::Equal, | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Where does this get exposed? None of the tests contain
builtin://. And is this supposed to be roundtripable? The parse code didn't get changed afaict.View changes since the review
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.
See #16675 (comment) for the parsing code. I was worried it could be exposed through error messages or something but I'm not able to find an example. This pkgid format is described in the RFC and I included it because I judged it as low-risk mainly, but I'm happy to move it to a more comprehensive PR if you'd rather.
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.
I'm fine with keeping it in this PR myself, but I don't know much about the cargo internals.
The pkgid current format seems to requires a single version of every standard library dependency. I added a tidy lint for it a while back, but I don't think it is a policy of the libs team to never ever add multiple versions of a crate. I'm guessing the only parts that would ever end up in the lockfile would be things like core, alloc and std for which I would be very surprised if there were ever two versions of in the same rustc version.
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.
PackageIdSpec's get exposed via
cargo pkgid,cargo metadata, json messagesThere 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.
In general yes, but in this PR builtin sourcekinds are not present in the unit-graph. They're replaced by paths from the std resolve. Most ways I found that view pkgids operate on the unit-graph, with some exceptions.
cargo pkgidactually requires a lockfile, and can't print builtins as we make sure they're not emitted in the lockfile.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.
What about
cargo metadata?There are also the places where PackageIdSpecs can be passed in. The only one that I can think of off the top of my head that can take non-local packages is meaningless yet should work:
cargo update.Is that the long term plan or just for this PR?
Potential impacts
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.
I'm not currently sure. My plan for this PR is to ensure consistency with the current -Zbuild-std implementation where possible. Future PRs will address individual subcommands in a more detailed way. For example,
cargo update -p std -Zbuild-stddoesn't work before or after this PR (but succeeds if there's no lockfile as if-pwasn't passed).Thanks for the list of impacts, I haven't considered these and have added them to our plan. The main factor I was previously weighing up was between wanting to hide/obscure builtins from the user, but still make them visible to tooling like rust-analyzer which only cares about the unit-graph really.
On Unit::is_local(), there's already an override for std present:
cargo/src/compiler/unit.rs
Line 165 in 593ae3e
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.
As someone working on tooling to execute Cargo's build plans with other build systems, I absolutely want to get standard library deps in the unit graph for my purposes.