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

[WIP] Implement binary-only dependencies #3870

Closed
wants to merge 3 commits into from
Closed

[WIP] Implement binary-only dependencies #3870

wants to merge 3 commits into from

Conversation

jonas-schievink
Copy link
Contributor

It's not as clean as I'd like, but it can be easily extended to allow
target-specific deps for all kinds of targets.

Still needs more tests, I'll probably add them tomorrow.

Cheers!

Closes #1982

@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 @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.

@alexcrichton
Copy link
Member

Thanks for the PR! I'll be traveling for the next few weeks so it may take me a moment to really dig into this PR. My main worry here is compatibility with the index and crates.io. Currently dependency kinds are encoded into the index and they're also understood by crates.io. That means that we need to be wary of:

  1. Publishing packages with bin-specific deps. Is this rejected by crates.io?
  2. Published packages with bin-specific deps. Do historical Cargo implementations choke on the index?

(etc)

I'll dig into these details as soon as I get a chance to look at this.

@matklad
Copy link
Member

matklad commented Mar 27, 2017

Hm, I have an interesting idea about the general problem we are trying to solve (and it is long past midnight in Saint-Petersburg, so the idea might be nonsense :)

What we actually carry about is the dependencies of the library. There's always at most one library per package, and downstream crates use only the library. We care about library dependencies because they affect the consumers of the library (because consumers also get all our dependencies and potential version conflicts). Looks like all the issues linked to #1982 are of the form "I don't want my lib to depend on X", not "I want my bin to depend on X"

By using binary-only dependencies we are black-listing deps we don't want to see in the library.

What if instead we whitelist the dependencies we want to see?

Whitelisting can work like this:

  • Introduce a new lib-dependencies key. All dependencies all the library must be included in this section.

  • If lib-dependencies is present, dependencies key lists additional dependencies for binaries, examples and whatnot.

@jonas-schievink
Copy link
Contributor Author

@matklad AFAIK, dependencies of tests, benchmarks and examples are already covered by the [dev-dependencies] key, and I'm thinking about what would happen if we'd just register binary-specific deps as dev-deps on crates.io (this should only affect display on crates.io anyways).

It's not as clean as I'd like, but it can be easily extended to allow
target-specific deps for all kinds of targets.
@bors
Copy link
Contributor

bors commented Apr 6, 2017

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

@alexcrichton
Copy link
Member

Ok so technically this looks pretty good to me. I'm wary to make much progress while we have active RFCs such as rust-lang/rfcs#1956 in flight as well though. In addition to what @matklad mentioned I could also imagine that with this implementation one may also readily expect for example:

[[test]]
name = "..."

[test.dependencies]
# ...

to also work, but it doesn't as part of this PR, unfortunately. Additionally, I'm still particularly worried about the format of the registry index here, can you describe what happens in these various situations?

  • An older version of Cargo uses a newly published crate with bin-dependencies
  • An new Cargo runs cargo install on a crate with bin-dependencies
  • An older version of Cargo runs cargo install on a crate with bin-dependencies

(etc)

@jonas-schievink
Copy link
Contributor Author

I'm wary to make much progress while we have active RFCs such as rust-lang/rfcs#1956 in flight as well though.

Agreed, that's why I'm currently not doing much work here.

I'm still particularly worried about the format of the registry index

Bin-deps are handled as dev-dependencies for now because I'm lazy :P

Of course, the cleaner solution would be to add proper support to crates.io and the index.

An older version of Cargo uses a newly published crate with bin-dependencies

Depending on such a crate is not a problem as binaries aren't available to downstream crates (they aren't built).

An new Cargo runs cargo install on a crate with bin-dependencies

This will correctly add dependencies of all installed binaries.

(I think I haven't yet added a test case for this, though)

An older version of Cargo runs cargo install on a crate with bin-dependencies

Installing or building a binary from said crate will not work, because the older Cargo version will not add the required dependencies (it will also print warnings/errors because the manifest doesn't parse correctly, of course).

In general, the only difference should be when using (building/installing) a binary that uses this new feature, all other behaviour should stay the same (and should be compatible with older Cargos).

@alexcrichton
Copy link
Member

@jonas-schievink yeah so what you describe for each scenario is what I would expect, but my point I guess was that I wasn't 100% sure that the current implementation would actually exhibit such behavior. For example I don't know if publishing bin-deps as dev-deps would keep cargo install working, this is just something I'd want to thoroughly test and work through.

We may wish to start consolidating conversation about the feature itself though around rust-lang/rfcs#1956, and we likely want to hold off on merging this until that's decided on one way or another as well.

@alexcrichton
Copy link
Member

I'm going to close this for now due to inactivity, and I think that we may wish to continue to discuss this externally outside of a PR before resubmitting.

@alexreg
Copy link
Contributor

alexreg commented Jul 24, 2017

Okay. Disappointing, but I hope this gets brought up again soon in discussion! There were some good ideas here.

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.

6 participants