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

Support crate renames in cargo metadata #5461

Merged
merged 2 commits into from
May 5, 2018
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented May 2, 2018

This adds information about (currently unstable) crate renames to metadata. Unfortunately, we already expose dependencies as a list of package ids (which are strings), so we can't easily add a rename field easily. For this reason, I've added a parallel deps key, which basically deprecates dependencies field.

So, the new format looks like this

{
    "dependencies": [
        "bar 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
        "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)"
    ],
    "deps": [
        {
            "extern_crate_name": "baz", // this one is actually renamed
            "id": "bar 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)"
        },
        {
            "extern_crate_name": "bar",
            "id": "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)"
        }
    ],
    "features": [],
    "id": "foo 0.5.0 (path+file://[..])"
},

Questions:

  • Is there a better name for extern_crate_name? This name is precise, but it's longer than I like, and might become opaque in meaning if we remove extern crate from the language :)
  • Should we feature gate this (i.e, only produce deps if we have a feature in Cargo.toml)? I think the answer is yes, but that'll require threading Features to Workspace...

r? @alexcrichton

@alexcrichton
Copy link
Member

I wonder if we could hit a few birds with one stone by changing the output to something like:

{
  "dependencies": [ ... ],
  "rename": {
    "foo": "package id here",
  },
  ..
}

We could only include rename if at least one dependency is renamed (automatically feature-gating this) and also avoids the extern_crate_name naming I think?

@matklad
Copy link
Member Author

matklad commented May 2, 2018

Hm, this sounded really neat, and I was really excited about using a map instead of a list, but now I think it won't fly :(

There are two problems:

First, you can't use renames as keys, because of examples like this

[dependencies]
rand = "0.3"

[build-dependencies]
rand = "0.4"

Second, we already really need to add more info to dependencies (like, is it a normal dep or a build-dep), and we would need to add even more info, if we implement target-specific deps. (EDIT: so we need to switch from package_id's to objects anyway)

I also would love to include not only explicit renames, but non-renamed crate names as well: that way, clients won't have to figure out that library name might be different from the package name, and that - should be translated to _.

I guess I'll just feature gate this properly, and will try to find a better name for extern_crate_name :)

@alexcrichton
Copy link
Member

Ah ok that makes sense to me, sounds like we'll need an auxiliary array no matter what to express the kind of dependencies, so seems fine by me!

@sfackler
Copy link
Member

sfackler commented May 2, 2018

cc #3984, #4632, and #4631 for other other dependency-related info that'd be good to add when refactoring!

@matklad matklad force-pushed the meta-rename branch 2 times, most recently from 034f8e3 to 1a13469 Compare May 5, 2018 17:01
@matklad
Copy link
Member Author

matklad commented May 5, 2018

Updated! The strategy now is to basically dump edges of resolve, which were introduced recently.

extern_crate_name is just name now, and I think we don't need to feature-gate it, because it does not expose rename key directly anyway. That is, even if rip rename of, having name: pkg.lib_target().unwrap().crate_name() would still be useful.

#3984 is fixed
#4631 is fixed

#4632 is not fixed, because we don't know cfgs when producing metadata, and the command itself even lacks the --target option.

@alexcrichton
Copy link
Member

Looks good to me, thanks! Could this also update https://doc.rust-lang.org/nightly/cargo/reference/external-tools.html with relevant information?

@matklad
Copy link
Member Author

matklad commented May 5, 2018

Could this also update https://doc.rust-lang.org/nightly/cargo/reference/external-tools.html with relevant information?

Sure! We historically have been bad with keeping that page really comprehensive. What do you think about saying: "this is API intended for developers of tools, so it is documented via doc comments in the source code, [link to some revision of ExportInfo]"?

@bors
Copy link
Contributor

bors commented May 5, 2018

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

@alexcrichton
Copy link
Member

Yeah I think that's ok if the previous documentation is deleted, b/c I think after this PR it'd be out of date

The old `dependencies` key in `resolve` is deprecated.

Instead, the new `deps` key introduced which lists dependencies kinds
(dev/build/normal) as well as `extern crate` name for the dep.
@matklad
Copy link
Member Author

matklad commented May 5, 2018

Yeah I think that's ok if the previous documentation is deleted, b/c I think after this PR it'd be out of date

Ok! Let's then merge this PR first, and then update the separately, so that I have an url with reasonable revision to put into the docs!

@alexcrichton
Copy link
Member

@bors: r+

Sounds good to me!

@bors
Copy link
Contributor

bors commented May 5, 2018

📌 Commit a312c55 has been approved by alexcrichton

bors added a commit that referenced this pull request May 5, 2018
Support crate renames in `cargo metadata`

This adds information about (currently unstable) crate renames to metadata. Unfortunately, we already expose dependencies as a list of package ids (which are strings), so we can't easily add a `rename` field easily. For this reason, I've added a parallel `deps` key, which basically deprecates `dependencies` field.

So, the new format looks like this

```JavaScript
{
    "dependencies": [
        "bar 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
        "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)"
    ],
    "deps": [
        {
            "extern_crate_name": "baz", // this one is actually renamed
            "id": "bar 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)"
        },
        {
            "extern_crate_name": "bar",
            "id": "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)"
        }
    ],
    "features": [],
    "id": "foo 0.5.0 (path+file://[..])"
},
```

Questions:

* Is there a better name for `extern_crate_name`? This name is precise, but it's longer than I like, and might become opaque in meaning if we remove `extern crate` from the language :)
* Should we feature gate this (i.e, only produce `deps` if we have a feature in `Cargo.toml`)? I think the answer is yes, but that'll require threading `Features` to `Workspace`...

r? @alexcrichton
@bors
Copy link
Contributor

bors commented May 5, 2018

⌛ Testing commit a312c55 with merge d0d3cb5...

@sfackler
Copy link
Member

sfackler commented May 5, 2018

#4632 is not fixed, because we don't know cfgs when producing metadata, and the command itself even lacks the --target option.

Cargo doesn't need to know which cfgs would be active if it was building the crate, it just needs to say that the dependency is only loaded for target X or cfg Y in the metadata JSON.

@bors
Copy link
Contributor

bors commented May 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d0d3cb5 to master...

@bors bors merged commit a312c55 into rust-lang:master May 5, 2018
@matklad matklad deleted the meta-rename branch May 6, 2018 11:21
@matklad
Copy link
Member Author

matklad commented May 10, 2018

I am having second thoughts here. Currently, we expose raw dependencies and make the clients to the work of figuring out which dep applies to which target. A theoretically better solution would be to directly produce, for each target, its dependent targets. The problem here is that we don't have a target_id concept, to refer to a particular target of a package. I've filed #5508 about that.

matklad added a commit to matklad/cargo that referenced this pull request May 27, 2018
…chton"

This reverts commit d0d3cb5, reversing
changes made to 757112c.

It is unclear if the design is right, see
rust-lang#5558 (comment)
matklad added a commit to matklad/cargo that referenced this pull request May 27, 2018
…chton"

This reverts commit d0d3cb5, reversing
changes made to 757112c.

It is not clear that the design is right, see
rust-lang#5558 (comment)
bors added a commit that referenced this pull request May 27, 2018
Revert "Auto merge of #5461 - matklad:meta-rename, r=alexcrichton"

This reverts commit d0d3cb5, reversing
changes made to 757112c.

It is unclear if the design is right, see
#5558 (comment)
bors added a commit that referenced this pull request May 27, 2018
Revert "Auto merge of #5461 - matklad:meta-rename, r=alexcrichton"

This reverts commit d0d3cb5, reversing
changes made to 757112c.

It is not clear that the design is right, see
#5558 (comment)

this is the backport sibling of #5576
ehuss added a commit to ehuss/rust that referenced this pull request Jun 12, 2018
- rust-lang/cargo#5577 - revert rust-lang/cargo#5461 (Support crate renames in `cargo metadata`)
- rust-lang/cargo#5567 - Copy `--all-features` request to all workspace members
bors added a commit to rust-lang/rust that referenced this pull request Jun 12, 2018
[BETA] Update Cargo

- rust-lang/cargo#5577 - revert rust-lang/cargo#5461 (Support crate renames in `cargo metadata`)
- rust-lang/cargo#5567 - Copy `--all-features` request to all workspace members
@matklad matklad mentioned this pull request Aug 7, 2018
@ehuss ehuss added this to the 1.27.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants