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

Tracking issue for --unit-graph #8002

Open
3 tasks
ehuss opened this issue Mar 15, 2020 · 2 comments
Open
3 tasks

Tracking issue for --unit-graph #8002

ehuss opened this issue Mar 15, 2020 · 2 comments
Labels
A-json-output Area: JSON message output C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2020

Implementation: #7977
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#unit-graph

Summary
The --unit-graph flag emits a JSON structure showing Cargo's internal unit graph.

Unresolved issues

  • Is there a better way to expose this information? I think in many cases, to make this useful, you have to also run cargo metadata (to get package names, for example). It seems a little silly to have to run two commands. Adding this to cargo metadata I think will be hard from a UX standpoint. It would require a superset of all flags of all commands (for example, cargo check --profile=test fundamentally changes things, so it would need to reproduce that flag). Every time I look at that, it seems really messy. There's also some flags like --target that don't interact with cargo metadata very well.
    • The primary motivator is to get access to feature relationships. But maybe there is a better way to deal with that?
  • Which parts of the output should be stabilized? This structure is very low-level, and thus inherently at high risk of changing in the future and being unstable. I think at a minimum we should document that it is not guaranteed to be backwards compatible. Should we filter some fields on stable?
  • Can't differentiate a normal check and a check for a test. The serialize implementation for CompileMode does not output the different kinds. Not sure how this should be displayed.
@ehuss ehuss added the C-tracking-issue Category: A tracking issue for something unstable. label Mar 15, 2020
@ehuss ehuss added the A-json-output Area: JSON message output label Mar 24, 2020
@matklad
Copy link
Member

matklad commented Mar 27, 2020

cc me and @vlad20012

Which parts of the output should be stabilized? This structure is very low-level, and thus inherently at high risk of changing in the future and being unstable.

I think there are two different low-levels in play here. First, this exposes the command line flags we pass to rustc (like --crate-type lib). Second, this exposes some of the Cargo's internals (like the profile field). One way to get something stabilizeable is to clearly split "rustc cli" from "cargo guts" here. We already provide stability guarantee for the compiler cli, so that bit would be easier to stabilize. Here's for example how this two-level representation would work for features:

{
  // stuff here is specific to Cargo and can change.
  "meta": { 
    "features": ["somefeat"],
    "pkg_id": "my-package 0.1.0 (path+file:///path/to/my-package)",
    "target":  {
      "src_path": "/path/to/my-package/src/lib.rs",
      ...
    }
  },
  // stuff here is build-system independent
  "cfg": ["feature=somefeat"],
  "root": "/path/to/my-package/src/lib.rs",
}

The biggest problem with this plan would be build-scripts. We can easily express the recipe for compiling build script. However, the bit about "run the excutble and append cfgs it prints to stdout to the next unit" would have to go to meta as it really doesn't have a pure rustc analogue.

@luser
Copy link
Contributor

luser commented Apr 1, 2020

This is great stuff, thanks for doing this work!

* Is there a better way to expose this information? I think in many cases, to make this useful, you have to also run `cargo metadata` (to get package names, for example). It seems a little silly to have to run two commands.

I agree that it would be nice to not have to run two commands. I think cargo metadata has shown enough usefulness on its own that it doesn't make sense to merge this into it, but maybe it would be useful to merge the metadata output into the unit graph output? The simplest thing would just be to stick it all under a "metadata": {} key, but I could also imagine a slightly more invasive merge where you have packages as a top-level key, say.

It would require a superset of all flags of all commands

I totally understand why this would be a PITA, however it does seem weird to have an option to a command like cargo build that makes it not actually build but do something different. cargo metadata doesn't currently take any positional arguments, right? What would you think about making cargo metadata {build|check|...} [options] produce this output instead? You'd have to copy all of those top-level subcommands down as second-level subcommands under metadata but that feels feasible.

I think at a minimum we should document that it is not guaranteed to be backwards compatible.

If you're going to ship it as stable then it needs to be stable. You've got a version in the output so presumably you could support --format-version like cargo metadata does. I don't know how much of a pain that would be internally. Your idea of separating out the cargo internals bit sounds good--perhaps it's worth a refactor to see if you can produce a stable subset of this output that's still sufficient to generate compiler commandlines?

The biggest problem with this plan would be build-scripts. We can easily express the recipe for compiling build script. However, the bit about "run the excutble and append cfgs it prints to stdout to the next unit"

I think it's fair to punt on this for now. It's straightforward to detect build scripts with "kind": ["custom-build"]. Ideally in the long run we'd figure out a solution that interops better with other build tools but for now having this info is still a net gain even if people have to write tooling to handle this.

A couple of notes from looking at the unit graph output from one of my crates locally:

  • "doctest": true is kind of confusing to me. I assume this means "this crate should get doctests built for it" but that doesn't seem useful when I'm invoking it with cargo build?
  • Is it possible to de-duplicate the profile key across units? I assume a lot of them are identical. (Not sure if this is worthwhile or not.)
  • I really don't like "platform": null. I assume this is the cargo weirdness around "no target specified" and I understand that it does behave differently than explicitly passing --target=$DEFAULT_TARGET but I'd rather see it omitted than have a null value if that's how it has to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-json-output Area: JSON message output C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.
Projects
Status: Unstable, baking
Development

No branches or pull requests

3 participants