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

Implement the 'frontend' of public-private dependencies #6772

Merged
merged 11 commits into from Apr 26, 2019

Conversation

Projects
None yet
6 participants
@Aaron1011
Copy link
Contributor

commented Mar 21, 2019

This is part of rust-lang/rust#44663

This implements the 'frontend' portion of RFC 1977. Once PRs rust-lang/rust#59335 and rust-lang/crates.io#1685 are merged,
it will be possible to test the full public-private dependency feature:
marking a dependency a public, seeing exported_private_dependencies
warnings from rustc, and seeing pub-dep-reachability errors from Cargo.

Everything in this commit should be fully backwards-compatible - users
who don't enable the 'public-dependency' cargo feature won't notice any
changes.

Note that this commit does not implement the remaining two features of
the RFC:

  • Choosing smallest versions when 'cargo publish' is run
  • Turning exported_private_dependencies warnings into hard errors when 'cargo publish' is run

The former is a major change to Cargo's behavior, and should be done in a separate PR with some kind of rollout plan.

The latter is described by the RFC as being enabled at 'some point in the future'. This can be done via a follow-up PR.

@rust-highfive

This comment has been minimized.

Copy link

commented Mar 21, 2019

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

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Note that this PR requires rust-lang/rust#59335 to be merged for the tests to pass, since rustc's handling of --extern-private is currently broken.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

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

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/final-pub-priv branch from 603a3e6 to 24ebfd4 Apr 2, 2019

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/final-pub-priv branch from df28d39 to 39a36a6 Apr 15, 2019

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@nrc: Now that rust-lang/rust#59335 has been merged, this PR is ready for review.

@Eh2406
Copy link
Contributor

left a comment

I don't know cargos frontend all that well, but this looks good to me!

@@ -41,6 +44,21 @@ impl Resolve {
unused_patches: Vec<PackageId>,
) -> Resolve {
let reverse_replacements = replacements.iter().map(|(&p, &r)| (r, p)).collect();
let public_dependencies = graph.iter().map(|p| {
let public_deps = graph.edges(p).flat_map(|(dep_package, deps)| {
let id_opt: Option<PackageId> = deps.iter().find(|d| d.kind() == Kind::Normal).and_then(|d| {

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Apr 18, 2019

Contributor

Just to be sure I understand, this calculates the direct (not transitive) public dependencies for each package. Yes?
Why the the d.kind() == Kind::Normal?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Apr 18, 2019

Author Contributor

The other kinds are Development and Build, neither of which can ever be exposed as transitive dependencies.

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Apr 18, 2019

Contributor

That makes sense. Should we have an error if you try a Cargo.toml that has a Development or Build dep with public set to true? Then add asserts in the set_public to make sure we never let it slip in?

@@ -443,6 +444,14 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car
let pkg_fingerprint = hash_all(&dst)?;
let ws = Workspace::ephemeral(new_pkg, config, None, true)?;

let rustc_args = if pkg.manifest().features().require(Feature::public_dependency()).is_ok() {
// FIXME: Turn this on at some point in the future
//Some(vec!["-D exported_private_dependencies".to_string()])

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Apr 18, 2019

Contributor

Sorry for my ignorance. I don't know what this will do or why it is off now. Can you elaborate?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Apr 18, 2019

Author Contributor

This turns all exported_private_dependencies warnings into hard errors when publishing. According to the RFC, 'cargo publish might change this warning into an error in its lint step'

I thought it was best to leave it off for now, so that we don't imemdiately break publishing for people who enable this feature.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

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

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/final-pub-priv branch from 39a36a6 to 343d72f Apr 18, 2019

@ehuss ehuss assigned ehuss and unassigned nrc Apr 18, 2019

@ehuss
Copy link
Contributor

left a comment

This is looking great, thanks!

Can you add a section to the bottom of https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/unstable.md that briefly explains how to use the feature? Just explain how to add cargo-features and how to set public = true, and a link to the tracking issue (rust-lang/rust#44663).

Are you planning to add the public field to the publish code after rust-lang/crates.io#1685 is done in a separate PR?

I think maybe it needs to only issue warnings for library targets? Otherwise you'll get unwanted warnings for leaking in binaries, tests, etc. I think, maybe, just check unit.target.is_lib() to determine if --extern-private should be used?

I think the public field should be included in the fingerprint (which is used to detect if something should be recompiled). If you compile, find warnings, then edit Cargo.toml to add private=true, and then compile again, the second compile won't do anything because Cargo does not think anything has changed. I think this can be fixed by adding a public: bool field to DepFingerprint.

Unrelated to this PR: I think the lint message will need to provide more information to the user about what is wrong and how to fix it. Historically rustc has avoided talking about Cargo in its diagnostics, but I don't see a way to avoid it here. Can you follow up with someone on the compiler team who works on diagnostics (like estebank, or anyone really) to figure out better wording? I would think it would be something like adding a "help" to the diagnostic that says something like "if you are using Cargo, consider adding public = true to the dependency in Cargo.toml". I would think it should also have a brief explanation of why a private dependency is bad to have in a public interface, but I don't know how to word that succinctly. Clippy has links to explanations, but I don't think rustc ever does that. Unfortunately I'm not very familiar with writing compiler lints.


#[test]
fn requires_feature() {

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 19, 2019

Contributor

Some extra whitespace here. Can you run rustfmt on this file to fix this and a few other minor formatting things?


if current.pkg.manifest().features().require(Feature::public_dependency()).is_ok() {
if !bcx.is_public_dependency(current, dep) {
cmd.arg("--extern-private").arg(&v);

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 19, 2019

Contributor

If I'm reading rust-lang/rust#59335 correctly, I think this should add either --extern OR --extern-private, not both?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Apr 23, 2019

Author Contributor

All external crates as passed via --extern. Private dependencies have an additional --extern-private flag (see https://github.com/rust-lang/rust/pull/59335/files#diff-5475738f2f1f16057af4f7ee93412ec4R3)

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 23, 2019

Contributor

I'm not sure I understand. Looking at this code it seems like it overrides whatever --extern* flags appeared previously. Why does it need to have both --extern and --extern-private if the first flag gets overridden/ignored? They both carry the entire package name and path, so I don't see how it carries more information to issue just --extern-private. If I run rustc manually with just --extern-private, it seems to work?

I'm concerned that the command line is exploding to extreme lengths. Windows has a limit of 32k (AFAIK), and with doubling the command line length, I see a few larger projects would be pushing 20-25k, which is getting close to that limit. And a 20k command-line does seem a bit extreme to me, just from a practicality standpoint.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Apr 23, 2019

Author Contributor

Originally, the plan was to pass --extern name=path and --extern-private name. However, during the course of implementing the Rustc end, we decided on --extern-private name=path`.

Now that I think about it, there's no particular reason for us to pass both --extern and --extern-private.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Maybe this is a question for @Eh2406, but when using renamed dependencies with this feature, I am getting a resolver error:

error: failed to select a version for `priv_dep`.
    ... required by package `foo v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/foo)`
versions that meet the requirements `= 0.2.0` are: 0.2.0

all possible versions conflict with previously selected packages.

  previously selected package `foo v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/foo)`

failed to select a version for `priv_dep` which could resolve this conflict

My dependencies are:

[dependencies]
priv_dep = "0.1.0"
priv_dep2 = {version="0.2.0", package="priv_dep"}

This does not happen if the feature is turned off.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@ehuss the rename thing is a known unresolved question, see discussion at #6129 (comment)

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Ah! Hopefully it won't be too difficult to fix.

Also, I noticed that the diagnostic lint uses the original crate name with renamed dependencies (when there is only one rename). I'm wondering if that might be confusing. For example, if you have:

[dependencies]
my_much_better_name = {version="0.1.0", package="priv_dep"}

You get a message like:

warning: type `my_much_better_name::FromPriv` from private dependency 'priv_dep' in public interface

It's probably fine, just contemplating out-loud.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@ehuss:

Are you planning to add the public field to the publish code after rust-lang/crates.io#1685 is done in a separate PR?

Yes, I am. There was some discussion around what the exact index format should be (in regard to how it will interact with renamed dependencies).

I think maybe it needs to only issue warnings for library targets? Otherwise, you'll get unwanted warnings for leaking in binaries, tests, etc. I think, maybe, just check unit.target.is_lib() to determine if --extern-private should be used?

I think it makes more sense to emit it unconditionally, at least for now. Binary crates can still 'export' types (to the rest of the crate) by marking them as pub. I don't know how useful the lint will end up being for binary crates in practice, but I think it should be on by default to allow people to easily try it out.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@ehuss: I've made the changes you requested, and respoded to your questions.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@ehuss: I've modified the PR to only pass --extern-private or --extern. There's currently a spurious macOS failure affecting all Cargo prs, so the build isn't passing atm.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

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

@@ -993,7 +1004,24 @@ fn build_deps_args<'a, 'cfg>(
v.push(cx.files().out_dir(dep));
v.push(&path::MAIN_SEPARATOR.to_string());
v.push(&output.path.file_name().unwrap());
cmd.arg("--extern").arg(&v);

let mut private = false;

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 25, 2019

Contributor

Just from a style standpoint, I would avoid the flag here. Just have something like (pseudo-code):

if …feature enabled… && !bcx.is_public_dependency(…) {
    cmd.arg("--extern-private");
    *need_unstable_opts |= true;
} else {
    cmd.arg("--extern");
}
@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Sorry about the delay. The travis issues were related to rustup, they should be fixed now.

Can you add the restrictions noted in #6772 (comment)? I think it makes sense to reject public in dev or build dependencies for now (probably somewhere in the toml parsing).

After rebasing, addressing the minor style nit, and the restrictions, I think this should be ready to go. Thanks so much!

Aaron1011 added some commits Mar 20, 2019

Implement the 'frontend' of public-private dependencies
This is part of rust-lang/rust#44663

This implements the 'frontend' portion of RFC 1977. Once PRs
rust-lang/rust#59335 and
rust-lang/crates.io#1685 are merged,
it will be possible to test the full public-private dependency feature:
marking a dependency a public, seeing exported_private_dependencies
warnings from rustc, and seeing pub-dep-reachability errors from Cargo.

Everything in this commit should be fully backwards-compatible - users
who don't enable the 'public-dependency' cargo feature won't notice any
changes.

Note that this commit does *not* implement the remaining two features of
the RFC:

* Choosing smallest versions when 'cargo publish' is run
* Turning exported_private_dependencies warnings into hard errors when
'cargo publish' is run

The former is a major change to Cargo's behavior, and should be done
in a separate PR with some kind of rollout plan.

The latter is described by the RFC as being enabled at 'some point in
the future'. This can be done via a follow-up PR.

Aaron1011 added some commits Apr 26, 2019

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/final-pub-priv branch from 7147616 to df4d209 Apr 26, 2019

Aaron1011 added some commits Apr 26, 2019

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@ehuss: Rebased and updated

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Nice! I can't think of anything else, so let's do it! Let us know if you need any help with the publishing side of things.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

📌 Commit f4aac94 has been approved by ehuss

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

Auto merge of #6772 - Aaron1011:feature/final-pub-priv, r=ehuss
Implement the 'frontend' of public-private dependencies

This is part of rust-lang/rust#44663

This implements the 'frontend' portion of [RFC 1977](https://github.com/rust-lang/rfcs/blob/master/text/1977-public-private-dependencies.md). Once PRs rust-lang/rust#59335 and rust-lang/crates.io#1685 are merged,
it will be possible to test the full public-private dependency feature:
marking a dependency a public, seeing exported_private_dependencies
warnings from rustc, and seeing pub-dep-reachability errors from Cargo.

Everything in this commit should be fully backwards-compatible - users
who don't enable the 'public-dependency' cargo feature won't notice any
changes.

Note that this commit does *not* implement the remaining two features of
the RFC:

* Choosing smallest versions when 'cargo publish' is run
* Turning exported_private_dependencies warnings into hard errors when 'cargo publish' is run

The former is a major change to Cargo's behavior, and should be done in a separate PR with some kind of rollout plan.

The latter is described by the RFC as being enabled at 'some point in the future'. This can be done via a follow-up PR.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

⌛️ Testing commit f4aac94 with merge 7b13469...

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: ehuss
Pushing 7b13469 to master...

@bors bors merged commit f4aac94 into rust-lang:master Apr 26, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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.