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 "metadata" subcommand (machine-readable package metadata output) #1434

Closed
wants to merge 13 commits into from

Conversation

winger
Copy link
Contributor

@winger winger commented Mar 19, 2015

Export subcommand outputs project info in machine-readable format (currently TOML or JSON).
Currently output include: resolved dependencies (with overrides applied) with versions, local paths and features list.

Intended use cases include IDE and other build systems integration.

Example TOML output for cargo: https://gist.github.com/winger/d218f25aeb3c9ba36b3a

Note: this PR is based on @dan-t's PR: #1225

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wycats (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. 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 CONTRIBUTING.md for more information.

@wycats
Copy link
Contributor

wycats commented Mar 19, 2015

Can you say more about the intended use case?

@winger
Copy link
Contributor Author

winger commented Mar 19, 2015

I'm currently working on implementing cargo import support for intellij idea's rust plugin (https://github.com/Vektah/idea-rust).
Dependency graph will be used to generate project structure and index libraries' code properly.

I'm going to send a PR to idea-rust with cargo import support in a couple of days, will post the link here.

@wycats
Copy link
Contributor

wycats commented Mar 19, 2015

export seems like a weird name for that, since you wouldn't want to use this information across machines, which export suggests might work.

What about cargo metadata or something?

@dan-t
Copy link

dan-t commented Mar 19, 2015

On Wed, Mar 18, 2015 at 09:05:41PM -0700, Yehuda Katz wrote:

Can you say more about the intended use case?

My motivation for starting the subcommand 'dependencies' was to get more
easily at the source code of the dependencies for my 'rusty-tags'[1] command.

Currently it's a bit of a mess to extract this information from the
'Cargo.toml' and 'Cargo.lock' files, and then it still doesn't support
local dependencies and dependency overrides.

So having a command that outputs infos about the really used
dependencies seems quite useful for a lot of external tools.

[1] https://github.com/dan-t/rusty-tags

@winger
Copy link
Contributor Author

winger commented Mar 19, 2015

@wycats, sure, will rename it to 'metadata'.
I've created a PR to idea-rust with basic cargo import support: https://github.com/Vektah/idea-rust/pull/42

@dan-t, sorry for hijacking your PR :)

@dan-t
Copy link

dan-t commented Mar 20, 2015

On Thu, Mar 19, 2015 at 03:10:15PM -0700, Vladislav Isenbaev wrote:

@wycats, sure, will rename it to 'metadata'.

The name 'metadata' is a bit abstract and doesn't tell that much. I still
consider my first idea naming it 'dependencies' not that bad, because
it's in the first place about the dependencies, and if someone looks
at the list of subcommands he might assume a operation like this.

@dan-t, sorry for hijacking your PR :)

Well, I can't say that I'm absolutely happy about this, but the PR was
a bit on rest, most likely because of the work to get Rust 1.0 out,
so having another push might be a good idea, but I don't quite like the form.

@winger
Copy link
Contributor Author

winger commented Mar 20, 2015

@dan-t, sorry, I just wanted to push this change forward but chose inappropriate way to do so. I'll update this request so that it would be easier to incorporate my changes to your's and close it.

As for naming, I don't think that limiting command to only output dependencies' info is a bit too strict. External programs may require information beyond that, for example project metadata, available features, targets information.

@dan-t
Copy link

dan-t commented Mar 20, 2015

On Fri, Mar 20, 2015 at 10:32:09AM -0700, Vladislav Isenbaev wrote:

@dan-t, sorry, I just wanted to push this change forward but chose
inappropriate way to do so. I'll update this request so that it would be easier
to incorporate my changes to your's and close it.

It's ok :).

As for naming, I don't think that limiting command to only output dependencies'
info is a bit too strict. External programs may require information beyond
that, for example project metadata, available features, targets information.

The question is if one command has to output all of these informations
or if it could be split into several commands?

@winger winger force-pushed the export branch 3 times, most recently from a46a574 to f0af937 Compare March 20, 2015 20:51
@winger
Copy link
Contributor Author

winger commented Mar 20, 2015

In my opinion, one configurable command is better, because calling multiple commands will cause additional overhead for parsing manifests/resolving dependencies compared to a single call.

@dan-t
Copy link

dan-t commented Mar 23, 2015

On Fri, Mar 20, 2015 at 02:02:45PM -0700, Vladislav Isenbaev wrote:

In my opinion, one configurable command is better, because calling multiple
commands will cause additional overhead for parsing manifests/resolving
dependencies compared to a single call.

Yes, it might be the better option.

I think that having a low level function that does only the resolving -
someting like I proposed in the other PR - is the way to go.

Then everything else could be build on top of it.

Currently I don't have that much time and energy, so if you want to push
this functionality further, then I'm fine with it.

We shouldn't keep two PRs open for the same thing, so if you want to
develop this further, then I will close my PR.

@bors
Copy link
Collaborator

bors commented Mar 23, 2015

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

@winger
Copy link
Contributor Author

winger commented Mar 26, 2015

It should be mergeable now.

@dan-t dan-t mentioned this pull request Mar 26, 2015
@swsnr
Copy link

swsnr commented Mar 30, 2015

I'd really like to see this command as well, for flycheck-rust, which enables Emacs to check Rust code in Cargo projects on the fly. Currently it's pretty flaky, since it's essentially just guessing how the Cargo layout looks like.

dan-t and others added 5 commits April 10, 2015 13:29
The subcommand dependencies outputs the resolved dependencies
of a project, the concrete used versions including overrides,
in a TOML format.
- Rebase to latest
- Fix build
- Rename dependencies -> metadata
- Implement rustc-serialize based output with toml and json support
- Add output-format and features flags
- Output status messages to stderr instead of stdout
@winger winger changed the title Add "export" subcommand Add "metadata" subcommand (machine-readable package metadata output) Apr 11, 2015
@alexcrichton
Copy link
Member

I'm not sure I know what you're referring to? What src_path is that?

@winger
Copy link
Contributor Author

winger commented Apr 18, 2015

Target::src_path from cargo/core/manifest.rs (it contains path of the root .rs file for target). IDEs need this, because without knowing root srcs it is impossible to resolve relative use statements.

@huonw
Copy link
Member

huonw commented Apr 30, 2015

cc me

@alexcrichton
Copy link
Member

@winger so this is looking good, but I'm still pretty worried about the output. For example there's quite a bit of information missing from the output, and the output is also currently ambiguous in some cases (e.g. you can't connect a dependency to what it's depending on).

A lot of this stems from the fact that the Encodable implementation blocks for Package, Dependency, Target, etc, have not been updated in ages. Most of this is currently used by the cargo read-manifest command (which is becoming dangerously similar to this).

Some specific information that I'm thinking is missing is:

  • 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
  • The Target serialization is missing all of the flags, some of which are quite important (e.g. test which indicates an integration test)
  • 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.
  • The manifest in a package is almost entirely omitted from the serialization here, but this may be ok.

Overall it seems like this command is kinda just a grab bag of putting information on stdout, and it's unclear what its purpose is beyond "learn some metadata about the local crate and its dependencies". 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. It's ok to start out small/conservative (e.g. we can leave some fields out), but the output should never be ambiguous where dots can't be connected.

One example could possibly be intending to use this command in conjunction with cargo read-manifest. This command could be tasked with ensure the entire dependency graph is downloaded and available, while cargo read-manifest just prints all the manifest information for a package. That way you can use cargo metadata to learn about all the fun information of a root crate, but it displays very little information about dependencies (other than where to find them), and if you want more information you can use cargo read-manifest.


[packages.libB.features]
featureA = ["featureB"]
featureB = []
Copy link
Member

Choose a reason for hiding this comment

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

This documentation seems to have diverged from what's actually output

@bors
Copy link
Collaborator

bors commented May 19, 2015

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

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase and comments addressed!

bors added a commit that referenced this pull request Nov 3, 2015
API changes needed for https://github.com/winger/cargo-metadata external command.
See #1434 for previous discussion.
bors added a commit that referenced this pull request Dec 8, 2015
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?
matklad added a commit to matklad/cargo that referenced this pull request Jan 25, 2016
bors added a commit that referenced this pull request Jan 25, 2016
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?
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.

8 participants