Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cargo metadata supports artifact dependencies #11550

Merged
merged 5 commits into from
Jan 16, 2023
Merged

cargo metadata supports artifact dependencies #11550

merged 5 commits into from
Jan 16, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

After this PR, cargo metadata will contain information of artifact dependencies in both packages and resolve.nodes.deps.dep_kinds. The implementation follows this comment #9992 (comment).

fixes #10607

How should we test and review this PR?

Raise some design decisions here:

  • For field resolve.nodes.deps.dep_kinds.compile_target, if the artifact dependency is specified as { …, target = "target" }, it will show as "target": "<target>" since Cargo doesn't know the exact target triple at that time.
  • It is forward-compatible with multidep. When Cargo sees multidep, it just appends a new dep_kind onto the array resolve.nodes.deps.dep_kinds.
  • resolve.nodes.deps.dep_kinds.name is deprecated. If a dependency doesn't have any lib target, it will show as empty string, e.g. "name": "".

Some integration tests:

  • metadata::workspace_metadata_with_dependencies_and_resolve reflects the change.
  • metadata::cargo_metadata_with_invalid_artifact_deps verifies syntax error of artifact

Additional information

I might revisit #10407 after this to do clean-ups.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2023
@weihanglo weihanglo force-pushed the issue-10607 branch 3 times, most recently from 09ec414 to 1111831 Compare January 7, 2023 00:19
@weihanglo weihanglo mentioned this pull request Jan 7, 2023
Closed
dtolnay
dtolnay reviewed Jan 7, 2023
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented support for this in reindeer (dtolnay-contrib/reindeer@6ba7df1) and it works for my use case. 👍

ehuss
ehuss reviewed Jan 9, 2023
View reviewed changes
src/cargo/ops/cargo_output_metadata.rs Outdated
Comment on lines 99 to 103
/// What the manifest calls the crate.
///
/// A renamed dependency will show the rename instead of original name.
#[serde(skip_serializing_if = "Option::is_none")]
extern_name: Option<InternedString>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this always be set? Related to deps.name being marked "deprecated", if this is None, then there would not be a way to determine the name of the dependency?

Copy link
Member Author

@weihanglo weihanglo Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. Update with d827e47.

⚠️ Potential breaking change ⚠️

However, previously cargo-metadata allows a dependency with different renamed co-exist. Its resolve.nodes.deps then will miss that dependency,
which is wrong I believe. After d827e47, cargo-metadata starts erroring out for that situation. That will introduce a "breaking change" to cargo-metadata, so I made 90044d1 to ignore that kind of error to keep the old behavior. If we're happy to ship this breaking change, 90044d1 can be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with generating an error in that case. It's an error for a normal build, so I wouldn't consider it a valid configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 90044d1 has been reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh crud, I think I misunderstood this. The extern_name is only relevant for artifact dependencies, correct? If so, I think I would like to keep it gated on -Zbindeps (that is, extern_name should only be set when -Zbindeps is set). Sorry for the confusion and runaround.

I think in the longer term when stabilizing we'll set it unconditionally (and add the deprecation notice).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with d02e36c and rebased to make extern_name optional. Thanks for the review!

src/cargo/ops/cargo_output_metadata.rs Outdated Show resolved Hide resolved
@weihanglo
refactor: reuse crate type strings in ArtifactKind
ca626c5
@weihanglo
cargo-metadata: support -Zbindeps info
e5ec492
@weihanglo
cargo-metadata: error out when encountering invalid artifact kind syntax
272193a 
This refactor reuse the logic of
`core::compiler::unit_dependencies::match_artifacts_kind_with_targets`
to emit error if there is any syntax error in `ArtifactKind`.

It also put `match_artifacts_kind_with_targets` to a better place `core::compiler::artifact`.
@weihanglo
cargo-metadata: error out if same dep with differnt names
690e6a4 
Previous, `cargo metadata` allows a dependency with different renamed
co-exist. However, its `resolve.nodes.deps` will miss that dependency,
which is wrong. After this commit, `cargo metadata starts erroring out
for that situation.
@weihanglo
Only output field extern_name when using -Zbindeps
d02e36c
@ehuss
Copy link
Contributor

ehuss commented Jan 16, 2023

Thanks! Sorry again about my confusion.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2023

📌 Commit d02e36c has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2023
@bors
Copy link
Contributor

bors commented Jan 16, 2023

⌛ Testing commit d02e36c with merge f0b6f81...

@bors
Copy link
Contributor

bors commented Jan 16, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing f0b6f81 to master...

@bors bors merged commit f0b6f81 into rust-lang:master Jan 16, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Jan 17, 2023
@weihanglo
Update cargo
335e904 
9 commits in 1cd6d3803dfb0b342272862a8590f5dfc9f72573..a5d47a72595dd6fbe7d4e4f6ec20dc5fe724edd1
2023-01-12 18:40:36 +0000 to 2023-01-16 18:51:50 +0000

- Add network container tests (rust-lang/cargo#11583)
- Show progress of crates.io index update even `net.git-fetch-with-cli` option enabled (rust-lang/cargo#11579)
- `cargo metadata` supports artifact dependencies (rust-lang/cargo#11550)
- fix(docs): add required "inherits" option to example profile (rust-lang/cargo#11504)
- add documentation that SSH markers aren't supported (rust-lang/cargo#11586)
- Fix typo (rust-lang/cargo#11585)
- Enable source_config_env test on Windows (rust-lang/cargo#11582)
- Support `codegen-backend` and `rustflags` in profiles in config file (rust-lang/cargo#11562)
- ci: reflect to clap updates (rust-lang/cargo#11578)
@weihanglo weihanglo mentioned this pull request Jan 17, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2023
@bors
Auto merge of rust-lang#107010 - weihanglo:update-cargo, r=weihanglo
e08b379 
Update cargo

9 commits in 1cd6d3803dfb0b342272862a8590f5dfc9f72573..a5d47a72595dd6fbe7d4e4f6ec20dc5fe724edd1 2023-01-12 18:40:36 +0000 to 2023-01-16 18:51:50 +0000

- Add network container tests (rust-lang/cargo#11583)
- Show progress of crates.io index update even `net.git-fetch-with-cli` option enabled (rust-lang/cargo#11579)
- `cargo metadata` supports artifact dependencies (rust-lang/cargo#11550)
- fix(docs): add required "inherits" option to example profile (rust-lang/cargo#11504)
- add documentation that SSH markers aren't supported (rust-lang/cargo#11586)
- Fix typo (rust-lang/cargo#11585)
- Enable source_config_env test on Windows (rust-lang/cargo#11582)
- Support `codegen-backend` and `rustflags` in profiles in config file (rust-lang/cargo#11562)
- ci: reflect to clap updates (rust-lang/cargo#11578)

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Jan 28, 2023
@weihanglo weihanglo deleted the issue-10607 branch March 1, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay left review comments

@ehuss ehuss ehuss left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.

Binary dependencies are not listed in metadata.packages or metadata.resolve
5 participants
@weihanglo @rustbot @ehuss @bors @dtolnay