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.

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

Add kind/platform info to cargo metadata #7132

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 14, 2019

This adds an array "dep_kinds" to the resolve nodes of the cargo metadata output. It looks something like this:

"resolve": {
  "nodes": [
    "id": "cargo 0.39.0 (path+file:///Users/eric/Proj/rust/cargo2)",
    "deps": [
      {
        "name": "bufstream",
        "pkg": "bufstream 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": "dev",
            "target": null
          }
        ]
      },
      {
        "name": "winapi",
        "pkg": "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": "cfg(windows)"
          }
        ]
      }
    ]
  ]
}

This allows one to filter the graph based on the dependency kind and platform.

I'm not completely confident that this is the right course, but I can't think of a better design. In particular, it seems a little strange to include all platforms, but features get filtered. This is probably not a problem in practice (one can use --all-features to ensure all features are shown for the top-level packages). Filtering out based on platform is very difficult, because you cannot determine from the resolve alone which nodes will be host vs target. That requires the entire Unit graph. We may expose the Unit graph in the future, but this seems like a useful and simple step.

This is a draft because I wanted to discuss this before moving forward. I'd like to add some more tests.

cc #4632. This doesn't filter based on target, but does expose the target names. As mentioned above, I don't think filtering is possible.
cc #5583. This adds more information.
Closes #3984.
Closes #4631 (I think).

cc @sfackler who filed some of these issues.

@alexcrichton
Copy link
Member

Seems like a reasonable approach to me! I agree this is a pretty simple, actionable, and useful step. Also agreed we can add more later!

@matklad
Copy link
Member

matklad commented Jul 25, 2019

LGTM!

@sfackler
Copy link
Member

Awesome! With respect to platform-specific cfgs, it'd be great to have some utility (whether it's part of the cargo binary or a library) that can evaluate them against a target to see if they match.

@alexcrichton alexcrichton added the T-cargo Team: Cargo label Jul 25, 2019
@alexcrichton
Copy link
Member

Ok I think this has sat long enough, let's see support from the team about adding these fields!

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 25, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jul 25, 2019
@bors
Copy link
Collaborator

bors commented Jul 29, 2019

☔ The latest upstream changes (presumably #7186) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jul 29, 2019
Copy link
Member

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

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

Looks good!

@bors
Copy link
Collaborator

bors commented Aug 7, 2019

☔ The latest upstream changes (presumably #7214) made this pull request unmergeable. Please resolve the merge conflicts.

@joshtriplett
Copy link
Member

Based on discussion in the cargo meeting:

@rfcbot concern target should be cfg or similar

@rfcbot concern Document handling of features (and decide if we should emit a features field)

@ehuss
Copy link
Contributor Author

ehuss commented Sep 4, 2019

@joshtriplett I took a look at renaming target. I would prefer a term like platform, but I'm concerned that it may cause more confusion, because the term target is already used in Dependency. Although "target" is a little vague, I think it would be preferable to be consistent. Do you feel strongly about using a different name, if it results in an inconsistency?

As for features, there already is a field that tracks features. The comment just above "result" here documents that features are resolved.

@joshtriplett
Copy link
Member

@rfcbot resolve target should be cfg or similar

Withdrawn, I didn't know target was already used elsewhere in the same structure for the same purpose, and I agree that we want consistency.

@joshtriplett
Copy link
Member

@ehuss I'm not talking about "resolve as if these features were set", I'm talking about annotating the metadata so that it's possible to know which features require which dependencies. Debian, for instance, generates librust-cratename+featurename-dev packages for any featurename that adds additional dependencies.

@ehuss
Copy link
Contributor Author

ehuss commented Sep 18, 2019

cc #7375 and #7376 for extensions/alternatives for platform handling.

@bors
Copy link
Collaborator

bors commented Oct 28, 2019

☔ The latest upstream changes (presumably #7409) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Oct 28, 2019

☔ The latest upstream changes (presumably #7376) made this pull request unmergeable. Please resolve the merge conflicts.

Due to how markdown works there can't be blank lines in the embedded HTML.
@sfackler
Copy link
Member

What needs to happen to get this moving again? I'd really like to be able to stop using cargo-the-library in cargo-tree.

@ehuss
Copy link
Contributor Author

ehuss commented Nov 13, 2019

@joshtriplett I'm still not sure I fully understand the use case. Optional dependencies are added by a feature of the same name as the dependency. I put together a small script to show how one can determine which optional dependencies are enabled: https://gist.github.com/ehuss/b91bc1701c1e3de1480d389fe0d9750b

So maybe to make it more clear, given the following manifest:

[package]
name = "foo"
version = "1.0.0"

[dependencies]
bitflags = {version="1.0", optional=true}

[features]
feat1 = ["feat2"]
feat2 = ["bitflags"]

There are three ways in this example to enable the bitflags dependency: --features=bitflags, --features=feat1, and --features=feat2. The script linked above will tell you that bitflags is enabled, but doesn't say which other features may have caused it to be added (it could be several). One could walk the feature graph and determine any other ancestor features that are enabled. For example, given --features=feat1 it could determine that bitflags is enabled by feat1, feat2, and bitflags. It's not really clear to me how that would be useful, though, and I'm not really sure which information you are looking for. Also, a dependency could be enabled by some other package in the graph, so it's really hard to draw a clear distinction of what enables a dependency. But that can be done by creating a reverse-dep graph and looking at all the edges.

Regardless, I think this is still pretty unrelated to adding the kind information to the resolve graph. The metadata contains all feature definitions, and which features are enabled on each package, and I think you can combine the two to understand how they interact.

Also, I think we are displaying as much information as Cargo has. Adding anything else will require changing the internals of how Cargo (particularly the resolver) works. But I don't know what else could be added.

@joshtriplett
Copy link
Member

@ehuss Thank you for the explanation. Given how Cargo handles features internally...

@rfcbot resolve Document handling of features (and decide if we should emit a features field)

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Nov 13, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 13, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 14, 2019

📌 Commit b65ebc6 has been approved by alexcrichton

@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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Nov 14, 2019
@bors
Copy link
Collaborator

bors commented Nov 14, 2019

⌛ Testing commit b65ebc6 with merge d0a41be...

bors added a commit that referenced this pull request Nov 14, 2019
Add kind/platform info to `cargo metadata`

This adds an array `"dep_kinds"` to the resolve nodes of the `cargo metadata` output. It looks something like this:

```javascript
"resolve": {
  "nodes": [
    "id": "cargo 0.39.0 (path+file:///Users/eric/Proj/rust/cargo2)",
    "deps": [
      {
        "name": "bufstream",
        "pkg": "bufstream 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": "dev",
            "target": null
          }
        ]
      },
      {
        "name": "winapi",
        "pkg": "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": "cfg(windows)"
          }
        ]
      }
    ]
  ]
}
```

This allows one to filter the graph based on the dependency kind and platform.

I'm not completely confident that this is the right course, but I can't think of a better design. In particular, it seems a little strange to include all platforms, but features get filtered. This is probably not a problem in practice (one can use `--all-features` to ensure all features are shown for the top-level packages). Filtering out based on platform is very difficult, because you cannot determine from the resolve alone which nodes will be host vs target. That requires the entire Unit graph. We may expose the Unit graph in the future, but this seems like a useful and simple step.

This is a draft because I wanted to discuss this before moving forward. I'd like to add some more tests.

cc #4632. This doesn't filter based on target, but does expose the target names. As mentioned above, I don't think filtering is possible.
cc #5583. This adds more information.
Closes #3984.
Closes #4631 (I think).

cc @sfackler who filed some of these issues.
@bors
Copy link
Collaborator

bors commented Nov 14, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing d0a41be to master...

@bors bors merged commit b65ebc6 into rust-lang:master Nov 14, 2019
@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Nov 23, 2019
bors added a commit to rust-lang/rust that referenced this pull request Nov 25, 2019
Update cargo, rls, books.

## nomicon

1 commits in 58e36e0e08dec5a379ac568827c058e25990d6cd..041c46e692a2592853aeca132c8dfe8eb5a79a9e
2019-10-30 08:14:24 -0500 to 2019-11-20 16:46:45 +0100
- Update unsafe-code-guidelines link (rust-lang/nomicon#175)

## cargo

15 commits in 8280633db680dec5bfe1de25156d1a1d53e6d190..750cb1482e4d0e74822cded7ab8b3c677ed8b041
2019-11-11 23:17:05 +0000 to 2019-11-23 23:06:36 +0000
- Some random comments and docstrings. (rust-lang/cargo#7625)
- Add value OUT_DIR to build-script-executed JSON message (rust-lang/cargo#7622)
- Update documentation for custom target dependencies. (rust-lang/cargo#7623)
- Document private items for binary crates by default (rust-lang/cargo#7593)
- Extend documentation on security concerns of crate names in a registry. (rust-lang/cargo#7616)
- Stabilize install-upgrade. (rust-lang/cargo#7560)
- Turn the new lock file format on by default (rust-lang/cargo#7579)
- bump im-rc version (rust-lang/cargo#7609)
- Ignore file lock errors if unsupported, on Windows (rust-lang/cargo#7602)
- Add hack for fwdansi change. (rust-lang/cargo#7607)
- Document Cargo's JSON output. (rust-lang/cargo#7595)
- Remove "cargo login" from user input when asking for login token. (rust-lang/cargo#7588)
- Fix all Clippy suggestions (but not add it to CI 🙃) (rust-lang/cargo#7574)
- Add kind/platform info to `cargo metadata` (rust-lang/cargo#7132)
- Update core-foundation requirement from 0.6.0 to 0.7.0 (rust-lang/cargo#7585)

## reference

2 commits in 45558c4..9e843ae
2019-11-08 14:47:35 +0100 to 2019-11-24 17:44:04 +0100
- Minor never type additions. (rust-lang/reference#723)
- Update associated-items.md.  "it"->is (rust-lang/reference#721)

## book

3 commits in e79dd62aa63396714278d484d91d48826737f47f..81ebaa2a3f88d4d106516c489682e64cacba4f60
2019-10-30 07:33:12 -0500 to 2019-11-15 08:30:04 -0800
- small fix ch04-03 & code block typo ch07-02 (rust-lang/book#2138)
- Adapt content of Chapter 16.3 in order to be consistent with improved compiler message (rust-lang/book#1779)
- [Rust 1.35] Remove FnBox and use builtin impl FnOnce for Box<FnOnce()> instead. (rust-lang/book#1906)

## rls

3 commits in 5db91c7b94ca81eead6b25bcf6196b869a44ece0..9ec2b8cb57c87517bcb506ac302eae339ffa2025
2019-10-30 16:04:39 +0100 to 2019-11-24 23:16:11 +0100
- Fix test for latest nightly. (rust-lang/rls#1595)
- doc: contributing: Remove outdated LSP extension (rust-lang/rls#1594)
- Update cargo. (rust-lang/rls#1591)

## rust-by-example

1 commits in dcee312c66267eb5a2f6f1561354003950e29105..4835e025826729827a94fdeb7cb85fed288d08bb
2019-10-31 11:26:53 -0300 to 2019-11-14 09:20:43 -0300
- crates: fix suggested value for --crate-type flag (rust-lang/rust-by-example#1292)

## edition-guide

1 commits in f553fb26c60c4623ea88a1cfe731eafe0643ce34..6601cab4666596494a569f94aa63b7b3230e9769
2019-10-30 08:27:42 -0500 to 2019-11-22 12:08:58 -0500
- Remove final nursery reference
@ehuss ehuss added this to the 1.41.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
8 participants