Introduce cargo metadata subcommand #2196

matklad · 2015-12-05T00:32:47Z

Most of the work was done by @dan-t in #1225 and by @winger in #1434

I failed to properly rebase previous attempts so I just salvaged this from bits and pieces.

@alexcrichton are you sure that the default format should be TOML? I think that TOML is more suitable for humans, and JSON is better (at the moment at least) for tools. Maybe we should default to ~~TOML~~ JSON?

rust-highfive · 2015-12-05T00:32:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

matklad · 2015-12-05T00:33:37Z

src/cargo/ops/cargo_output_metadata.rs

+                        features: Vec<String>,
+                        no_default_features: bool)
+                        -> CargoResult<(Resolve, Vec<Package>)> {
+    let mut source = try!(PathSource::for_path(manifest.parent().unwrap(), config));


Is this unwrap safe?

Yeah this should be fine

matklad · 2015-12-05T00:49:55Z

Another question is where should we put documentation for the format? Is the help message a right place?

This hides `SerializedDependency` from general public, as requested [here](#1434 (comment)). It also hides `SerializedManifest` which was (wrongly?) exposed. This is required for #2196. I want to move in small steps this time, hence the separate PR. Technically this break backwards compatibility, because `SerializedDependency` and `SerializedManifest` were public (`SerializedPackage` was private however). Are such changes allowed in cargo?

Turbo87 · 2015-12-08T15:17:25Z

@matklad how about inline code documentation in the relevant module file?

matklad · 2015-12-09T21:34:51Z

I'd love to see a more targeted purpose for this subcommand which can precisely answer what sort of data needs to be emitted on stdout.

Sure :) We want to use cargo metadata with a plugin for Intellij IDEA. IDEA based IDEs are "smart": they build a syntax tree from source code and perform "static analysis" (completion, navigation, refactorings, ...) using it. To build such tree across several files, we need to know some metadata about the project.

The most vital piece of information is a list of crate roots that belong to the project. Only if you know crate roots (src/lib.rs, bin/whatever.rs, etc) you can correctly map files to modules. This piece of information comes from targets filed of a package.

Note that a rust project can include several cargo subprojects (path dependencies), so we need this information too.

Another place where we need help from cargo is extren crates declarations. We need to resolve extern crate foo; to the source file, containing foo. This information is available because a project includes dependencies and packages include all known crates.

We don't use feautes and kind information at the moment.

For example there's quite a bit of information missing from the output

Missing output is Ok, it will be easy to add more information later. Moreover, if we output too much, we may make changing the internal cargo structures harder, because the serialized output is currently tied to this structures.

The output is also currently ambiguous in some cases (e.g. you can't connect a dependency to what it's depending on).

I would argue that this also falls into "missing information" category. There are two pieces of data here: a flat list of known packages and a dependency DAG. At the moment, metadata outputs only flat list of known packages. dependencies key in the output lists only declared (that is, not resolved) dependencies. We can add a proper DAG serialization later. At the moment the only real DAG information that we need is which package is a main package in the project. The root key in the output provides this info.

Source information for where a crate originally came from is not existent. For example you can't know if a crate came from git or from crates.io

This is an important piece of information, but it is not needed at least for our use case. It can be added later.

The Target serialization is missing all of the flags, some of which are quite important (e.g. test which indicates an integration test)

Hm, target serialization does include kind. I am more worried that it also includes

"metadata": {
  "extra_filename": "-a129c63589d0f328",
  "metadata": "a129c63589d0f328"
}

Not sure what it is, but seems like it does not belong here :)

SerializedDependency is quite old (and should go away in favor of Encodable on Dependency directly) and doesn't include any of the information about a platform target, what kind of dependency, etc.

SerializedDependency is removed. Missing information is again easy to add latter.

The manifest in a package is almost entirely omitted from the serialization here, but this may be ok.

What is the relation between a Package and a Manifest? The only extra bit of information that Package (and it's serialized representation) has is a path to Cargo.toml.

matklad · 2015-12-11T13:30:04Z

@alexcrichton 1.5 is 🎉 released 🎉 so any feedback on this? :)

Thinking of it more, I propose the following format:

{
  "packages": [
    {
      "name": "foo",
      "package_id": "some_name some_version some_source_id",
      //...
    },
    //...
  ],

  "graph": [
    "root": "some_name some_version some_source_id",
    "edges": [
      //...
    ]
  ] 
}

That is, we should include a PackageId to the representation of Package and store a graph of PackageIds separately. As I said, we can live with only root node in the graph for a start.

So we can remove this from the output.

A thing that bothers me a little is that PackageIds are supposed to be opaque identifiers, but they are serialized in a highly structured string.

alexcrichton · 2015-12-11T14:46:31Z

Thanks for pushing on this again @matklad! Currently I'm at a work week in Orlando, but I hope to digest more of this first thing next week!

alexcrichton · 2015-12-16T18:23:49Z

src/bin/metadata.rs

+    flag_manifest_path: Option<String>,
+    flag_no_default_features: bool,
+    flag_output_format: OutputFormat,
+    flag_output_path: OutputTo,


Perhaps this could just be Option<String>? That way a custom Decodable implementation isn't needed

alexcrichton · 2015-12-16T18:36:09Z

Thanks @matklad! This is looking good to me. I think it's important to rely on the standard Encodable implementations as much as possible to ensure consistency among commands (e.g. Encodable for Package).

I've got a few bike-sheddy points that I'd like to discuss here:

The actual data emitted as part of SerializedPackage is pretty limiting now. I think we may want to tighten it up a bit (e.g. use package ids instead of name/version), and we may want to expose a few standard features (like features).
I think that we may also want to do an audit of other Encodable implementations transitively included as well. For example I'm not sure that Target and Dependency have suitable encodings, would be a good thing to check up on!
The term metadata is interesting here when compared to read-manifest. The difference between the two commands is that metadata will give you the entire dependency graph whereas read-manifest will only give you the current package. That distinction seems fine (one fetches deps, one doesn't), but the naming between the two seems a little inconsistent.

Thoughts?

To respond to some of your points as well, I agree that limiting the amount of information being printed is a good idea. I'd just want to make sure that this is useful in a first draft rather than restricting it too much in terms of the information being printed. For example I'm ok with not emitting the dependency graph for now, that can always be added later.

matklad · 2015-12-16T19:06:01Z

(e.g. use package ids instead of name/version),

Hm, DependencyInner does not include a package id. It can be found in Resolve, which is not encodable. Lets serialize Resolve in the graph thing?

For example I'm not sure that Target and Dependency have suitable encodings, would be a good thing to check up on!

Will look at the Encodables today.

That distinction seems fine (one fetches deps, one doesn't), but the naming between the two seems a little inconsistent.

Does anybody uses read-manifest in the wild? I dislike a bit that there are two machine oriented commands in cargo. Maybe we should change from cargo metadata to cargo metadata --dependencies to allow for future extensions (like cargo metadata --manifest)?

alexcrichton · 2015-12-16T20:06:32Z

Ah yes Dependency intentionally does not include PackageId (because it's not a resolved dependency). To reconstruct the same graph that Cargo has this'll need to serialize Resolve. I think it'd be possible with a structure like:

{
    "packages": [ ... ],
    "resolve": {
        "root": "...",
        "packages": {
            "package1": ["dep1", "dep2"],
        }
    }

Where all the packages in resolve are listed as package ids. That way tools can use resolve to figure out what the precise structure is, and then they can use the package metadata information in the packages array to figure out things like source paths and such.

alexcrichton · 2015-12-16T20:07:06Z

Also I'm not sure if there are too many users of read-manifest, but I'd be fine deprecating in favor of this command if they served the same purpose. I do feel like the modes of operation may be important to preserve, however.

matklad · 2015-12-16T20:51:01Z

Ah yes Dependency intentionally does not include PackageId (because it's not a resolved dependency).

Should we include both declared and resolved dependencies in the output? I'd say yes: this will help to detect issues like #2064

matklad · 2015-12-16T20:57:35Z

@alexcrichton I'm a bit worried that Package and Manifest have almost identical representation:

https://github.com/rust-lang/cargo/blob/master/src/cargo/core/manifest.rs#L47-L52
https://github.com/rust-lang/cargo/blob/master/src/cargo/core/package.rs#L26-L32

If I add features, I need to add it to both places, which is troubling.

Given this comment I'm not even sure that we need both a Package and a Manifest ;)

alexcrichton · 2015-12-16T21:21:07Z

Should we include both declared and resolved dependencies in the output?

I think we should, yeah, but in separate locations (to mirror what Cargo does)

I'm a bit worried that Package and Manifest have almost identical representation:

An excellent point! I think it should be ok to actually just have the Encodable implementation for Package delegate to the manifest, or to just delete Encodable for Manifest and deal with it all in Package.

matklad · 2015-12-17T18:44:08Z

this'll need to serialize Resolve.

There is EncodebleResolve which is used for the Cargo.lock, but the format is a bit different from what we want here.

Particularly, there is no package_id in the package node, (version, name, source) are used instead. However for deps package_id is used.

I'd better provide a separate EncodableResolve implementation for the purpose of metadata.

alexcrichton · 2015-12-17T18:56:09Z

Ah actually I think that's basically exactly what we want here, could you elaborate on what's missing from constructing it? (e.g. you've already got a Resolve itself). I think it may also be important to preserve the metadata section for future purposes.

I will say though idiomatically the implementation of Encodable should probably mirror what you've been migrating to elsewhere!

matklad · 2015-12-17T18:59:57Z

The entry in the lock file looks like this:

[[package]]
name = "aho-corasick"
version = "0.3.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
 "memchr 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
]

Note that in dependencies we have a string (a package id). But in package itself we have only name, version, source, but no package id.

Package id string can be reconstructed from this three things, but it does not feel right.

@dan-t

Most of the work was done by @dan-t in rust-lang#1225 and by @winger in rust-lang#1434 Fixes rust-lang#2193

alexcrichton · 2016-01-25T17:35:50Z

@bors: r+ b24bf7e

Looks good to me, thanks again @matklad!

bors · 2016-01-25T17:56:01Z

⌛ Testing commit b24bf7e with merge 859c5d3...

@dan-t

Most of the work was done by @dan-t in #1225 and by @winger in #1434 Fixes #2193 I failed to properly rebase previous attempts so I just salvaged this from bits and pieces. @alexcrichton are you sure that the default format should be TOML? I think that TOML is more suitable for humans, and JSON is better (at the moment at least) for tools. Maybe we should default to ~~TOML~~ JSON?

matklad · 2016-01-25T18:03:13Z

@alexcrichton what is the release schedule for cargo?

Am I correct that metadata will be available right away in nightlies? And that it will get to stable Rust only with 1.8.0?

bors · 2016-01-25T18:09:44Z

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64

alexcrichton · 2016-01-25T18:16:44Z

@matklad yeah this'll ride the normal release trains and will be available in Rust 1.8

matklad · 2016-01-29T00:11:26Z

Ouch, @alexcrichton, I think we've missed one rather major issue here, this one

Especially given that there are two "implementations" of Encodable for package id.

https://github.com/matklad/cargo/blob/metadata2/src/cargo/core/resolver/encode.rs#L99

https://github.com/matklad/cargo/blob/metadata2/src/cargo/core/package_id.rs#L29

The id's in resolve and in packages are indeed different :( Here is an example

{
  "packages": [
    {
      "name": "geom",
      "version": "0.1.0",
      // This id is different 
      "id": "geom 0.1.0 (path+file:\/\/\/home\/matklad\/projects\/rustraytracer)",
      "source": null,
      ...
    },
    ...
  ],
  "resolve": {
    "root": {
      "name": "rustraytracer",
      "version": "0.1.0",
      "source": null,
      "dependencies": [
        // from this id :(
        "geom 0.1.0", 
        "rand 0.3.11 (registry+https:\/\/github.com\/rust-lang\/crates.io-index)",
        "regex 0.1.41 (registry+https:\/\/github.com\/rust-lang\/crates.io-index)",
        "rustc-serialize 0.3.16 (registry+https:\/\/github.com\/rust-lang\/crates.io-index)",
        "simple_parallel 0.3.0 (registry+https:\/\/github.com\/rust-lang\/crates.io-index)",
        "time 0.1.33 (registry+https:\/\/github.com\/rust-lang\/crates.io-index)",
        "utils 0.1.0"
      ]
    },
    ...
  },
}

In resolve, there is no source part in id for path dependencies. In packages, source is always included in id. We can fix it if we omit source from packages.

alexcrichton · 2016-01-29T00:45:09Z

Oh dear that is indeed not good!

I can't seem to recall myself why there are two Encodable implementations for PackageId, but perhaps they can be merged (favoring the one from resolve)?

matklad · 2016-01-29T01:03:12Z

but perhaps they can be merged (favoring the one from resolve)?

Probably not. The one from resolve is "context sensitive". Look at this function: https://github.com/rust-lang/cargo/blob/master/src/cargo/core/resolver/encode.rs#L189. It creates an EncodablePackageId from PackageId.

Unfortunately it depends on the root package and because of this can't be reused or repeated in the general Encodable implementation for PackageId.

alexcrichton · 2016-01-29T05:09:08Z

Hm ok, so the requirements of resolve are indeed a little different. We don't want to emit filesystem paths to the lock file because otherwise they'd just oscillate over time as you migrate among machines.

That being said there aren't too many uses for encoding package ids, and in general it amounts to an assertion that path-based source ids are omitted where everything else is included.

It's probably fine for now to change the encodable implementation for package ids to ignore path sources and that way it'll match resolve (and resolve can use the same implementation)

matklad · 2016-01-29T15:46:46Z

It's probably fine for now to change the encodable implementation for package ids to ignore path sources and that way it'll match resolve (and resolve can use the same implementation)

I've tried to do it here and the result is unsatisfactory.

The crux of the problem is that PackageId should be both Encodable and Decodable. Without serializing source it is impossible to preserve Decodability. And without correct Decodable implementation, cargo-install tests fail.

EncodableResolve sidesteps the issue by retrieving the source from elsewhere: https://github.com/rust-lang/cargo/blob/master/src/cargo/core/resolver/encode.rs#L138

This makes me think again that maybe we should leave lock-file resolve serilization to lockfile only, and instead provide a simpler and more natural generic serialization for resolve :)

And there is one more thing, SourceId.precise is serialized differently in two places, but this is a minor issue.

alexcrichton · 2016-01-29T19:03:33Z

Hm ok, I forgot that package ids were being encoded for cargo-install. For now let's go to what you were mentioning early on which is to not use the EncodableResolve implementation for resolve itself. The cargo metadata command can implement its own version of Encodable for that.

Sorry for the roundabout way to conclude that!

matklad · 2016-01-29T21:42:08Z

Sorry for the roundabout way to conclude that!

Experiment is a nice way to make a ( I hope so :) ) correct conclusion.

here is a PR: #2331

rust-highfive assigned alexcrichton Dec 5, 2015

matklad reviewed Dec 5, 2015
View reviewed changes

matklad force-pushed the metadata2 branch from e58e6d4 to fcc1831 Compare December 5, 2015 13:35

matklad mentioned this pull request Dec 7, 2015

Hide SerializedDependency #2201

Merged

alexcrichton reviewed Dec 16, 2015
View reviewed changes

matklad mentioned this pull request Dec 16, 2015

refactor Target serialization #2219

Merged

matklad force-pushed the metadata2 branch from 209e354 to c24746e Compare December 17, 2015 16:27

matklad force-pushed the metadata2 branch 2 times, most recently from 4757aca to f8d5a21 Compare January 25, 2016 14:06

matklad added 4 commits January 25, 2016 17:12

Introduce cargo metadata subcommand

fec3ce9

Most of the work was done by @dan-t in rust-lang#1225 and by @winger in rust-lang#1434 Fixes rust-lang#2193

metadata: serialize Resolve

fe7291a

metadata: simplify

16e4d72

metadata: add version to format

874fe26

matklad force-pushed the metadata2 branch from f8d5a21 to 4c66b8f Compare January 25, 2016 14:38

matklad mentioned this pull request Jan 25, 2016

Improve output of machine oriented commands #2313

Closed

metadata: use existing infra for JSON output

b24bf7e

matklad force-pushed the metadata2 branch from 4c66b8f to b24bf7e Compare January 25, 2016 15:04

bors merged commit b24bf7e into rust-lang:master Jan 25, 2016

alexeykudinkin mentioned this pull request Jan 28, 2016

Rolling Nightly Channel intellij-rust/intellij-rust#161

Closed

matklad mentioned this pull request Feb 4, 2016

move read-manifest into metadata? #2356

Closed

swsnr mentioned this pull request Feb 7, 2016

Extract build settings and project layout from Cargo itself flycheck/flycheck-rust#8

Closed

matklad deleted the metadata2 branch April 11, 2016 23:32

alexcrichton mentioned this pull request Apr 13, 2016

1.8 release announcement rust-lang/blog.rust-lang.org#82

Merged

andersk mentioned this pull request Apr 2, 2017

Fix ‘cargo metadata’ call for cargo 0.19.0-nightly flycheck/flycheck-rust#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce cargo metadata subcommand #2196

Introduce cargo metadata subcommand #2196

matklad commented Dec 5, 2015

rust-highfive commented Dec 5, 2015

matklad Dec 5, 2015

alexcrichton Dec 16, 2015

matklad commented Dec 5, 2015

Turbo87 commented Dec 8, 2015

matklad commented Dec 9, 2015

matklad commented Dec 11, 2015

alexcrichton commented Dec 11, 2015

alexcrichton Dec 16, 2015

alexcrichton commented Dec 16, 2015

matklad commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

matklad commented Dec 16, 2015

matklad commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

matklad commented Dec 17, 2015

alexcrichton commented Dec 17, 2015

matklad commented Dec 17, 2015

alexcrichton commented Jan 25, 2016

bors commented Jan 25, 2016

matklad commented Jan 25, 2016

bors commented Jan 25, 2016

alexcrichton commented Jan 25, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016

Introduce cargo metadata subcommand #2196

Introduce cargo metadata subcommand #2196

Conversation

matklad commented Dec 5, 2015

rust-highfive commented Dec 5, 2015

matklad Dec 5, 2015

Choose a reason for hiding this comment

alexcrichton Dec 16, 2015

Choose a reason for hiding this comment

matklad commented Dec 5, 2015

Turbo87 commented Dec 8, 2015

matklad commented Dec 9, 2015

matklad commented Dec 11, 2015

alexcrichton commented Dec 11, 2015

alexcrichton Dec 16, 2015

Choose a reason for hiding this comment

alexcrichton commented Dec 16, 2015

matklad commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

matklad commented Dec 16, 2015

matklad commented Dec 16, 2015

alexcrichton commented Dec 16, 2015

matklad commented Dec 17, 2015

alexcrichton commented Dec 17, 2015

matklad commented Dec 17, 2015

alexcrichton commented Jan 25, 2016

bors commented Jan 25, 2016

matklad commented Jan 25, 2016

bors commented Jan 25, 2016

alexcrichton commented Jan 25, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

matklad commented Jan 29, 2016