-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
cargo: Add support for system-deps
dependencies
#13778
Conversation
2ce0fde
to
35226b1
Compare
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 have one comment, but otherwise this looks good to me.
feature_overrides: T.Dict[str, T.Dict[str, str]] | ||
|
||
@classmethod | ||
def from_raw(cls, name: str, raw: T.Any) -> SystemDependency: |
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.
def from_raw(cls, name: str, raw: T.Any) -> SystemDependency: | |
def from_raw(cls, name: str, raw: T.Dict[str, T.Union[str, T.Dict[str, T..Any]]) -> SystemDependency: |
Would still be an improvement, though it would be nicer if we actually annotated this, which would help us with our expectations. But looking at the documentation it's fairly non-trival.
Which, actually, it doesn't look like this handles the case of:
[package.metadata.system-deps]
testlib = "1.2"
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 don't see any benefit in using complex typing here, you'll have to cast it for that type anyway... The reality is from_raw is fundamentally untyped because it's what converts a random toml into a typed SystemDependency class. That's why I would like to massively simplify the typing with #12983.
You can't even really know the type at all, metadata field can really be anything.
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 think raw: T.Dict[str, T.Any]
is the most you can really tell.
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.
Fixed the testlib = "1.2"
case.
Introduce a global Cargo interpreter state that keeps track of enabled features on each crate. Before generating AST of a Cargo subproject, it downloads every sub-subproject and resolves the set of features enabled on each of them recursively. When it later generates AST for one its dependencies, its set of features and dependencies is already determined.
The library name defaults to its package name, but it can be different. For example: - package name: cairo-sys-rs - library name: cairo-sys - dependency name: ffi
In the case the main project has a .wrap file for a cargo subproject, that subproject's Cargo.lock must be loaded before we can recursively fetch all its dependencies.
962eec5
to
9033de8
Compare
9033de8
to
4dd5518
Compare
Based on top of #12952