Implement public/private dependency feature #57586

Aaron1011 · 2019-01-14T03:43:17Z

Implements #44663

The core implementation is done - however, there are a few issues that still need to be resolved:

The EXTERNAL_PRIVATE_DEPENDENCY lint currently does notthing when the public_private_dependencies is not enabled. Should mentioning the lint (in an allow or deny attribute) be an error if the feature is not enabled? (Resolved- the feature was removed)
Crates with the name core and std are always marked public, without the need to explcitily specify them on the command line. Is this what we want to do? Do we want to allowno_std/no_core crates to explicitly control this in some way? (Resolved - private crates are now explicitly specified)
Should I add additional UI tests? (Resolved - added more tests)
Does it make sense to be able to allow/deny the EXTERNAL_PRIVATE_DEPENDENCY on an individual item? (Resolved - this is implemented)

rust-highfive · 2019-01-14T03:43:20Z

r? @estebank

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

Aaron1011 · 2019-01-14T03:43:30Z

r? @alexcrichton

src/librustc/lint/builtin.rs

src/librustc_privacy/lib.rs

src/librustc_lint/lib.rs

src/librustc_privacy/lib.rs

petrochenkov · 2019-01-14T23:32:21Z

src/librustc_privacy/lib.rs

+        if !self.tcx.features().public_private_dependencies {
+            return false
+        }
+        let ret = self.required_visibility == ty::Visibility::Public &&


This will report things used in pub, but unreachable items, e.g.

mod private_details { pub fn details() -> PrivateDep { ... } // Will be linted }

, but that's probably okay for a start.

rust-highfive · 2019-01-21T01:42:00Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:039df983:start=1548033551792912346,finish=1548033554187915130,duration=2395002784
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:22:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:22:31] expected success, got: exit code: 101
[00:22:31] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:22:31] Build completed unsuccessfully in 0:18:33
[00:22:31] make: *** [all] Error 1
[00:22:31] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:23b0df10
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Jan 21 01:41:56 UTC 2019

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

estebank · 2019-01-22T01:53:45Z

r? @petrochenkov

petrochenkov · 2019-01-22T08:33:17Z

@estebank
The needs a review from someone from Cargo side, that's why @alexcrichton was assigned.

Otherwise, comments beside #57586 (comment) still need to be addressed.

src/librustc/session/config.rs

alexcrichton · 2019-01-22T21:47:11Z

This is looking good to me, it is a lot smaller than I was expecting!

Were there particular design questions that I should weigh in on? Or is it just technical details?

petrochenkov · 2019-01-22T21:50:52Z

@alexcrichton

Were there particular design questions that I should weigh in on?

#57586 (comment)

bors · 2019-01-26T09:48:35Z

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

rust-highfive · 2019-01-29T22:24:48Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:05fb5810:start=1548796732319083221,finish=1548796818901711168,duration=86582627947
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:03:18] ................................................................................................i... 3800/5358
[01:03:20] .................................................................................................... 3900/5358
[01:03:22] .....................................................i.............................................. 4000/5358
[01:03:25] .................................................................................................... 4100/5358
[01:03:33] .........................................F.......................................................... 4200/5358
[01:03:41] .................................................................................................... 4400/5358
[01:03:45] .................................................................................................... 4500/5358
[01:03:50] .........i.......................................................................................... 4600/5358
[01:03:54] .................................................................................................... 4700/5358
---
[01:04:14] .................................................................................................i.. 5300/5358
[01:04:15] ..........................................................
[01:04:15] failures:
[01:04:15] 
[01:04:15] ---- [ui] ui/privacy/pub-priv-dep/std-pub.rs stdout ----
[01:04:15] 
[01:04:15] error: test compilation failed although it shouldn't!
[01:04:15] status: exit code: 1
[01:04:15] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/privacy/pub-priv-dep/std-pub.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/privacy/pub-priv-dep/std-pub/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/privacy/pub-priv-dep/std-pub/auxiliary" "-A" "unused"
[01:04:15] ------------------------------------------
[01:04:15] 
[01:04:15] ------------------------------------------
[01:04:15] stderr:
[01:04:15] stderr:
[01:04:15] ------------------------------------------
[01:04:15] {"message":"unknown feature `public_private_dependencies`","code":{"code":"E0635","explanation":"\nThe `#![feature]` attribute specified an unknown feature.\n\nErroneous code example:\n\n```compile_fail,E0635\n#![feature(nonexistent_rust_feature)] // error: unknown feature\n```\n\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/privacy/pub-priv-dep/std-pub.rs","byte_start":130,"byte_end":157,"line_start":6,"line_end":6,"column_start":12,"column_end":39,"is_primary":true,"text":[{"text":"#![feature(public_private_dependencies)]","highlight_start":12,"highlight_end":39}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0635]: unknown feature `public_private_dependencies`\n  --> /checkout/src/test/ui/privacy/pub-priv-dep/std-pub.rs:6:12\n   |\nLL | #![feature(public_private_dependencies)]\n   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n"}
[01:04:15] {"message":"For more information about this error, try `rustc --explain E0635`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0635`.\n"}
[01:04:15] 
[01:04:15] ------------------------------------------
[01:04:15] 
[01:04:15] 
[01:04:15] thread '[ui] ui/privacy/pub-priv-dep/std-pub.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3291:9
[01:04:15] 
[01:0t code: 101
[01:04:15] 
[01:04:15] 
[01:04:15] 
[01:04:15] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:04:15] Build completed unsuccessfully in 0:04:07
[01:04:15] make: *** [check] Error 1
[01:04:15] Makefile:48: recipe for target 'check' failed

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Aaron1011 · 2019-02-01T14:45:09Z

@petrochenkov: I've rebased against master.

petrochenkov · 2019-02-01T15:01:50Z

@bors r+

bors · 2019-02-01T15:01:51Z

📌 Commit 369faae has been approved by petrochenkov

bors · 2019-02-01T15:24:33Z

⌛ Testing commit 369faae with merge 742fcc7...

Implement public/private dependency feature Implements #44663 The core implementation is done - however, there are a few issues that still need to be resolved: - [x] The `EXTERNAL_PRIVATE_DEPENDENCY` lint currently does notthing when the `public_private_dependencies` is not enabled. Should mentioning the lint (in an `allow` or `deny` attribute) be an error if the feature is not enabled? (Resolved- the feature was removed) - [x] Crates with the name `core` and `std` are always marked public, without the need to explcitily specify them on the command line. Is this what we want to do? Do we want to allow`no_std`/`no_core` crates to explicitly control this in some way? (Resolved - private crates are now explicitly specified) - [x] Should I add additional UI tests? (Resolved - added more tests) - [x] Does it make sense to be able to allow/deny the `EXTERNAL_PRIVATE_DEPENDENCY` on an individual item? (Resolved - this is implemented)

bors · 2019-02-01T17:59:30Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 742fcc7 to master...

Eh2406 · 2019-02-01T19:18:15Z

Grate work on getting this landed! Thank you! That is 1 of 3 major parts of the implementation for that RFC done! I am working on the cargo dependency resolution part. It is working, just exponentially slow. At any time I can make a PR (with the fuzz testing turned off) with something functional enough for someone to start working on the 3ed part. Namely the cargo front end part, adding it to the Cargo.toml, to the registry, a future flag, and passing the args to rustc. Let me know if I should make that PR.

Aaron1011 · 2019-02-07T21:04:54Z

@Eh2406: That sounds like a good idea

Eh2406 · 2019-02-12T03:20:23Z

PR is up rust-lang/cargo#6653

part of the infrastructure for public & private dependencies in the resolver This is part of my work on public & private dependencies in the resolver from #6129. As discussed there the proptest fuzzers are happy to find exponential blow up with all the back jumping strategies I have tried. So this PR does not have a back jumping strategie nor does it have the proptest fuzzers generating public dependencies. These will both need to change for the feature to stabilize. In the meantime it gives the correct results on the cases it can handle. With rust-lang/rust#57586 landed there is a lot of work to do on Cargos front end. Adding a UI for this, passing the relevant things to rustc, passing it to crates.io, passing it to cargo-metadata. This is good enough to allow that work to proceed.

Eh2406 · 2019-03-13T20:33:17Z

@Aaron1011 The PR is merged, we would love to guide you (or anyone) to doing the cargo-frontend side!

Aaron1011 · 2019-03-20T03:25:45Z

I've started work on the cargo-frontend side.

…petrochenkov Properly parse '--extern-private' with name and path It turns out that rust-lang#57586 didn't properly parse `--extern-private name=path`. This PR properly implements the `--extern-private` option. I've added a new `extern-private` option to `compiletest`, which causes an `--extern-private` option to be passed to the compiler with the proper path. Part of rust-lang#44663

Properly parse '--extern-private' with name and path It turns out that #57586 didn't properly parse `--extern-private name=path`. This PR properly implements the `--extern-private` option. I've added a new `extern-private` option to `compiletest`, which causes an `--extern-private` option to be passed to the compiler with the proper path. Part of #44663

ehuss · 2019-04-19T20:54:14Z

@Aaron1011 Was src/test/ui/feature-gates/feature-gate-public_private_dependencies.rs supposed to be deleted now that there is no feature?

rust-highfive assigned estebank Jan 14, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2019

rust-highfive assigned alexcrichton and unassigned estebank Jan 14, 2019

petrochenkov self-assigned this Jan 14, 2019