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

cargo metadata: Don't show `null` deps. #6534

Merged
merged 1 commit into from Mar 26, 2019

Conversation

Projects
None yet
7 participants
@ehuss
Copy link
Contributor

ehuss commented Jan 10, 2019

If a package has a dependency without a library target, the "name" field was
showing up as null in resolve.nodes.deps. At this time (AFAIK), binary-only
dependencies are always ignored. Instead of making users filter out this entry
(or more commonly, crash), just don't include it.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jan 10, 2019

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@ehuss ehuss force-pushed the ehuss:meta-deps-name-null branch from b7630d6 to a2fc5cf Jan 10, 2019

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 10, 2019

Field missing vs field set to null.. Hmm, not sure which is best or if there's an established convention in we're using and should continue to follow.

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 10, 2019

Field missing vs field set to null.. Hmm, not sure which is best or if there's an established convention in we're using and should continue to follow.

It's not just the field, it's the entire Dep value. It's the difference between:

"deps": [
    {
        "name": null,
        "pkg": "bdep 0.5.0 (path+file:///bdep)"
    }
],

and

"deps": [],

This causes things like cargo_metadata to fail because they are requiring a string value for name.

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 10, 2019

What is cargo_metadata?

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 10, 2019

Oh right, https://github.com/oli-obk/cargo_metadata. So this is resolving an incoherence between cargo and that crate.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Jan 10, 2019

I would love to see us more involved in making sure that cargo and cargo_metadata always work together.

What is the use case for depending on a crate without a library? In that use case should cargo_metadata's users have to be aware that that crate exists?

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 10, 2019

What is the use case for depending on a crate without a library? In that use case should cargo_metadata's users have to be aware that that crate exists?

I can't think of a legitimate case. I scanned all of crates.io for packages with binary dependencies, and all I found were:

  • ./aquaengine-0.0.2 on clippy 0.0.302
  • ./cargo-name-1.0.1 on clippy 0.0.302
  • ./collapse-crate-0.1.0 on clippy 0.0.302
  • ./dagon-0.0.2 on clippy 0.0.302
  • ./dat-0.0.0 on clippy 0.0.302
  • ./github-release-0.1.2 on clippy 0.0.302
  • ./k-bucket-0.1.0 on clippy 0.0.302
  • ./pop-trait-0.2.1 on clippy 0.0.302
  • ./prefix-map-0.3.1 on clippy 0.0.302
  • ./rand-pop-0.1.1 on clippy 0.0.302
  • ./rrun-ssh-0.3.0 on clippy 0.0.302
  • ./rtps-0.2.3 on clippy 0.0.302
  • ./rumqtt-0.10.1 on clippy 0.0.302
  • ./speedometer-0.2.2 on clippy 0.0.302
  • ./statics-0.2.0 on cargo-watch 6.0.0
  • ./version-sort-0.1.0 on clippy 0.0.302

The clippy ones are because a special version of clippy was published that says "do not use me". I suspect the cargo-watch thing was just a mistake.

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 10, 2019

I would love to see us more involved in making sure that cargo and cargo_metadata always work together.

Oh, and the reason I found this is because I was working on a new test suite for cargo_metadata. 😆

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 10, 2019

Maybe we should depend on cargo_metadata, or maybe we should have a crate inside this repo that cargo_metadata depends on (or is replaced by). (sorry, veering off-topic)

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Jan 10, 2019

If the team is on board with having closer ties then we should open up a discussion of how that looks. (I have thoughts, but it is off topic here.)

I feel a little uncomfortable removing information from our output. This is a thing a project can do, and metadata is how tooling will find out about it. Like I am not sure the bug is on the cargo side.

That being said I don't want to make cargo_metadata have to use an Option<String> just to support this very niche case (not to mention the breaking change). Maybe cargo_metadata can convert null to ""?

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 10, 2019

Yeah, I think that's a sound argument.

I'm ambivalent, personally.

@ehuss ehuss referenced this pull request Jan 10, 2019

Merged

Add some more fields. #61

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Jan 10, 2019

This was semi intentional, to make deps strictly better than dependencies. If we filter dep without a library, the user wouldn't be able to figure out the dependency at all (field in Package specifies only requirement, not dependency).

It is true that library-less dependencies are weird, so perhaps it makes sense to put the fix closer to the core, report an error if we see such a dependency and just assume, everywhere, that every dependency has a library target?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Jan 10, 2019

Semi unrelated, but, while we are at it, perhaps it makes sense to add a dependency from a package on itself, while we are at it? That way clients won't have to deal with replacing - with _ and dealing with [lib] name = "other" for current package?

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 10, 2019

It is true that library-less dependencies are weird, so perhaps it makes sense to put the fix closer to the core, report an error if we see such a dependency and just assume, everywhere, that every dependency has a library target?

I'm not very excited about making a hard error. I'm often wary of adding new errors since they can cause trouble, and the warning→error transition is a hassle. It would break any package that had clippy="*" in them. I think a warning would be a very good idea, though.

It might help me if I knew who is currently using this information and for what purpose. In the past I've tried to use it, but it doesn't provide enough information for the things I wanted to do. The entries can't be (reliably) matched to the Package dependencies, so you can't tell what Kind they are, or anything else. My ideal situation would be to have access to the unit dependencies map.

the user wouldn't be able to figure out the dependency at all

I'm still of the position that these binary dependencies are useless information that is not actionable, and just requires all users to filter them out. I don't know what purpose they serve to know about a dependency that can't be used.

@Eh2406 Eh2406 referenced this pull request Jan 16, 2019

Open

How can Cargo help #63

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2019

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

cargo metadata: Don't show `null` deps.
If a package has a dependency without a library target, the "name" field was
showing up as null in `resolve.nodes.deps`. At this time (AFAIK), binary-only
dependencies are always ignored. Instead of making users filter out this entry
(or more commonly, crash), just don't include it.
@matklad

This comment has been minimized.

Copy link
Member

matklad commented Mar 16, 2019

Heh, hit this in rust-analyzer: rust-analyzer/rust-analyzer#979

The current PR definitely looks good, r=me.

Adding a warning can done separately.

let nodes = &meta["resolve"]["nodes"];
assert!(nodes[0]["deps"].as_array().unwrap().is_empty());
assert!(nodes[1]["deps"].as_array().unwrap().is_empty());
}

This comment has been minimized.

Copy link
@matklad

matklad Mar 16, 2019

Member

We might have a test for this (I remember dealing with this problem explicitelly) already. Might be a good idea to put .unwrap on the name and run the test-suite.

This comment has been minimized.

Copy link
@ehuss

ehuss Mar 16, 2019

Author Contributor

I tried with an unwrap, but none of the tests failed.

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Mar 26, 2019

Ping @Eh2406 or @alexcrichton, just wondering if we can make a decision on this. I see three options:

  1. Do nothing, force all users to filter out these null entries.
  2. Merge as-is.
  3. Enhance or redesign how dependency information is exposed.

I still lean for 2, for the reasons outlined above. I don't think we have the list of use cases to properly do 3.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Mar 26, 2019

I don't understand this well enough to have a deciding opinion. Looks like the knowledgeable people seem to like merging.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 26, 2019

I'd personally be fine with option 2 myself as recommended!

Is this just waiting on an r+ otherwise?

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Mar 26, 2019

Is this just waiting on an r+ otherwise?

Yep.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 26, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2019

📌 Commit a2fc5cf has been approved by alexcrichton

@ehuss ehuss closed this Mar 26, 2019

@ehuss ehuss reopened this Mar 26, 2019

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Mar 26, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2019

📌 Commit c3f4b0d has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2019

⌛️ Testing commit c3f4b0d with merge b3b65c3...

bors added a commit that referenced this pull request Mar 26, 2019

Auto merge of #6534 - ehuss:meta-deps-name-null, r=alexcrichton
cargo metadata: Don't show `null` deps.

If a package has a dependency without a library target, the "name" field was
showing up as null in `resolve.nodes.deps`. At this time (AFAIK), binary-only
dependencies are always ignored. Instead of making users filter out this entry
(or more commonly, crash), just don't include it.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2019

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

@bors bors merged commit c3f4b0d into rust-lang:master Mar 26, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@ehuss ehuss referenced this pull request Mar 29, 2019

Merged

Update cargo #59516

bors added a commit to rust-lang/rust that referenced this pull request Mar 30, 2019

Auto merge of #59516 - ehuss:update-cargo, r=alexcrichton
Update cargo

Update cargo

22 commits in 0e35bd8af0ec72d3225c4819b330b94628f0e9d0..63231f438a2b5b84ccf319a5de22343ee0316323
2019-03-13 06:52:51 +0000 to 2019-03-27 12:26:45 +0000
- Code cleanup (rust-lang/cargo#6787)
- Add cargo:rustc-link-arg to pass custom linker arguments (rust-lang/cargo#6298)
- Testsuite: remove some unnecessary is_nightly checks. (rust-lang/cargo#6786)
- cargo metadata: Don't show `null` deps. (rust-lang/cargo#6534)
- Some fingerprint cleanup. (rust-lang/cargo#6785)
- Fix fingerprint for canceled build script. (rust-lang/cargo#6782)
- Canonicalize default target if it ends with `.json` (rust-lang/cargo#6778)
- Fix setting `panic=unwind` compiling lib a extra time. (rust-lang/cargo#6781)
- Always nicely show errors from crates.io if possible (rust-lang/cargo#6771)
- Testsuite: Make `cwd()` relative to project root. (rust-lang/cargo#6768)
- Allow `cargo fix` if gitignore matches root working dir. (rust-lang/cargo#6767)
- Remove redundant imports (rust-lang/cargo#6763)
- Handle backcompat hazard with `toml` crate (rust-lang/cargo#6761)
- Fix spurious error in dirty_both_lib_and_test. (rust-lang/cargo#6756)
- Update toml requirement from 0.4.2 to 0.5.0 (rust-lang/cargo#6760)
- Reuse std::env::consts::EXE_SUFFIX (rust-lang/cargo#6758)
- Proptest 0.9.1 (rust-lang/cargo#6753)
- Don't need extern crate in 2018 (rust-lang/cargo#6752)
- Release a jobserver token while locking a file (rust-lang/cargo#6748)
- Minor doc fix for publish command synopsis (rust-lang/cargo#6749)
- Stricter package change detection. (rust-lang/cargo#6740)
- Fix resolving yanked crates when using a local registry. (rust-lang/cargo#6742)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.