Path deps outside workspace are not members #3443

matklad · 2016-12-22T17:59:57Z

This implements @Boscop suggestion: path dependencies pointing outside the workspace are never members.

Not sure that it handled #3192 fully: you can't exclude a path dependency within workspace. Not sure this is super useful though...

rust-highfive · 2016-12-22T18:00:09Z

r? @brson

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

matklad · 2016-12-22T18:00:44Z

r? @alexcrichton

matklad · 2016-12-22T19:24:14Z

src/cargo/core/workspace.rs

        if self.members.iter().any(|p| p == manifest_path) {
            return Ok(())
        }
+        if is_path_dep


This check is a bit more complicated that what one would expect.

Firstly, we want to allow path dependencies outside the wrokspace dir if they have an explicit parent pointer.

Secondly, we want to add explicitelly specified members, even if they are outside the workspace and lack a parent pointer. This way we'd bail out with the error in validate, instead of simply ignoring a bad member.

matklad · 2016-12-29T11:52:27Z

ping @alexcrichton

alexcrichton · 2016-12-29T16:32:38Z

Thanks for the PR! (and the ping)

I've been noodling on this in the background a bit. I'm unsure if this'll play out nicely. The purpose for the rules around workspaces were basically to ensure that Cargo.lock never oscillated or changed spuriously. That is, if you cargo build on any crate in a workspace it should always generate the same Cargo.lock.

That being said I think that this still preserves that invariant. I'm slightly worried about path dependencies like:

workspace root -> not in a workspace -> back in the workspace

I think with this PR as written such a construction is guaranteed to not work if you run build in the crate "not in a workspace". (right?)

I'm not sure if we should be worried about such cases, though, as they have to be constructed manually anyway. In any case though I don't think that this should fully close out #3192 because I could imagine hierarchical constructions of workspaces still want to explicitly say that some subdirectories aren't workspace members.

matklad · 2016-12-29T21:44:33Z

In any case though I don't think that this should fully close out #3192 because I could imagine hierarchical constructions of workspaces still want to explicitly say that some subdirectories aren't workspace members.

Yes, this does not add an "explicit exclusion" feature, this just excludes all paths outside of the workspace directory from automatic inclusion. But perhaps we don't need an extra knob to control this? If you don't want a path dep to be a part of workspace, put it alongside, and not inside, the workspace directory, just like in the example from #3192. @phil-opp, will this be convenient enough to you?

And, by the way, I think there is already a crude explicit exclusion mechanism in the RFC: you can explicitelly specify members, and then Cargo should not include path dependencies automatically.

I think with this PR as written such a construction is guaranteed to not work if you run build in the crate "not in a workspace". (right?)

Hm, I think this will and should write exactly one lockfile inside not in a workspace. Consider a simplified (but equivalent) example

not a workspace package -- path dependency --> package in some workspace

Executing cargo build inside the first package should completely ignore the lockfiles of the workspace, because lockfiles of dependencies are ignored.

Random musings:

Hm, the fact that combinations of workspace.members and workspace.root allows you to have a package which is a part of the workspace, but is not hierarchically under workspace root worries me a bit. Are there some real world use cases for this, or it is just an artifact of the RFC? Could we have started more conservatively, without the root key and with strict directory hierarchy requirements?

matklad · 2016-12-29T21:46:42Z

Hm, this generally deviates from RFC, because not all path deps are automatically included. @alexcrichton, should the RFC text be amended, or just the docs at https://github.com/rust-lang/cargo/blob/master/src/doc/manifest.md#the-workspace-section ?

phil-opp · 2016-12-30T11:48:47Z

Yes, this does not add an "explicit exclusion" feature, this just excludes all paths outside of the workspace directory from automatic inclusion. But perhaps we don't need an extra knob to control this? If you don't want a path dep to be a part of workspace, put it alongside, and not inside, the workspace directory, just like in the example from #3192. @phil-opp, will this be convenient enough to you?

I really like this solution. It's a much better and less surprising default.

And, by the way, I think there is already a crude explicit exclusion mechanism in the RFC: you can explicitelly specify members, and then Cargo should not include path dependencies automatically.

The problem is that the implementation differs from the RFC. The current implementation implicitly adds all path dependencies to the workspace members, so that all path dependencies are included, even with members = [].

matklad · 2017-01-02T17:58:03Z

Hm, I think there's another corner case which I am not entirely sure how to deal with.

Consider this directory/package layout:

.
├── root
│   
└── member
    │
    └── path_dep

Here, root is the root of the workspace, and it explicitelly includes member. Now, member has a path dependency which is hierarchically bellow it, but not below root. Should this path_dep be a member of the workspace?

I would say yes, but currently the PR does not implement this.

matklad · 2017-01-03T11:40:14Z

Huh, found two more bugs in the current (without this PR applied) Cargo while pondering over this.

First, in the layout above path_dep believes it does not belong to any workspace, because there's no package with workpsace above it.

The second is #3489.

alexcrichton · 2017-01-06T01:18:43Z

@matklad yes the current implementation diverges from the RFC, but explicitly so. I found it far more ergonomic and convenient if all path dependencies were included by default, as it ended up being the 90% case of what you wanted anyway. As a result the RFC isn't quite the source of truth here as the current implementation just pulls in all path deps transitively to the workspace.

I think the example I gave actually works fine because the intermediate crate isn't in a workspace, so we won't try to do any workspace inference or anything. Can you add a test to this effect though with some more... "flavorful" workspace constructions?

Hm, the fact that combinations of workspace.members and workspace.root allows you to have a package which is a part of the workspace, but is not hierarchically under workspace root worries me a bit. Are there some real world use cases for this, or it is just an artifact of the RFC?

Yes this was intentional. Workspaces like the compiler and Servo don't have a hierarchical workspace root, but it's off on the side.

Here, root is the root of the workspace, and it explicitelly includes member. Now, member has a path dependency which is hierarchically bellow it, but not below root. Should this path_dep be a member of the workspace?

Functionally, yes, I think it would make sense. As implemented, no, it would not work as you'd have to manually configure the workspace.

The intention with logic around workspace inference and such was to support all constructions via explicit configuration and almost all constructions with little configuration through inference. Right now we don't support all workspace constructions though as there's no way to exclude members from workspaces or turn off path inclusion logic.

bors · 2017-01-10T09:00:08Z

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

alexcrichton · 2017-01-13T21:26:33Z

@matklad thoughts about my most recent comment?

matklad · 2017-01-13T21:50:29Z

Sounds reasonable! I'll rebase this PR and add more tests during the weekend.

So the end resolve would look like this:

There is workspace root. There is a list of explicit members. They define a set of workspace directories.
Any path dependencies in the graph which are under any of the ws dirs are workspace members (this means that when reading Cargo.toml traversing the directory upwards we should look for explicit members with workspace_root = , and not only for [workspace], this would be handled in another PR).

alexcrichton · 2017-01-13T23:04:06Z

@matklad that sounds about right to me, yeah.

closes rust-lang#3192

alexcrichton · 2017-01-15T19:02:10Z

@bors: r+

Thanks!

bors · 2017-01-15T19:02:11Z

📌 Commit 7429347 has been approved by alexcrichton

bors · 2017-01-16T05:08:25Z

⌛ Testing commit 7429347 with merge 5822e2c...

bors · 2017-01-16T15:08:31Z

💥 Test timed out

alexcrichton · 2017-01-16T17:48:19Z

@bors: retry

matklad · 2017-01-16T23:03:51Z

Wait a minute, @alexcrichton , shouldn't the test_path_dependency_under_member fail in validate worksapce at the moment?

See, when writing a test, I did not specify [workspace] here, so this points to the workspace that isn't a workspace. Should we validate this?

bors · 2017-01-17T01:08:47Z

⌛ Testing commit 7429347 with merge 468fbd7...

bors · 2017-01-17T02:03:20Z

💔 Test failed - status-appveyor

alexcrichton · 2017-01-17T02:48:45Z

@matklad hm yes that is indeed worrisome! Is that an existing bug or an accidental bug here?

matklad · 2017-01-17T09:51:10Z

Is that an existing bug or an accidental bug here?

It's an existing bug. When we start compilation in the non-workspace package, we don't check workspace configuration of the path dependencies at all.

I am not 100% sure that that's a bug though. Basically, we work the same way as the pre-workspace Cargo. Though validating this would be helpful.

Are there any plans of making workspaces the default? Like, adding an empty [workspace] when doing cargo new?

alexcrichton · 2017-01-17T19:32:50Z

I think we need more support before making workspaces the default as cargo new should basically "work no matter what". The main thing missing there is Cargo.toml editing support

Also the test here is an invalid workspace, but it's working? That is, the pointer is wrong? I could have sworn that we verified that...

matklad · 2017-01-17T19:35:57Z

I could have sworn that we verified that...

The root package is not a workspace (It was intended to be, but I've forgot to put [workspace] there), I believe this just doesn't trigger any workspace verification, despite the fact that we have a path dependency which thinks we are inside a workspace.

alexcrichton · 2017-01-17T19:37:08Z

Oh so a build in ws, with no [workspace], works. But a build in foo, with a pointer to an invalid root, fails?

If that's the case then that's actually expected behavior, and yeah is largely backcompat business.

matklad · 2017-01-17T19:41:45Z

Oh so a build in ws, with no [workspace], works. But a build in foo, with a pointer to an invalid root, fails?

Yeah, exactly!

If that's the case then that's actually expected behavior, and yeah is largely backcompat business.

I think we can safely make this an error, no? There seems to be no backcompat hazards here. Not sure if we should make this an error. On the one hand, users can misconfigure the workspace and think that everything is OK (as demonstrated by me:) ). On the other hand, this is a rather obscure failure mode, and seems to require a bit of work to warn about.

alexcrichton · 2017-01-17T23:49:26Z

Yeah I agree that this seems reasonable to generate an error for in terms of it's not backwards incompatible. It seems reasonable as well to just go ahead and generate the error anyway so long as it doesn't impose too much work.

In any case the test failures here are spurious and otherwise I think we still want this PR, so...

@bors: retry

bors · 2017-01-18T00:22:26Z

⌛ Testing commit 7429347 with merge 82ea175...

@Boscop

Path deps outside workspace are not members closes #3192 This implements @Boscop suggestion: path dependencies pointing outside the workspace are never members. Not sure that it handled #3192 fully: you can't exclude a path dependency within workspace. Not sure this is super useful though...

bors · 2017-01-18T02:44:33Z

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

Fix a test. That's the test we've discussed in #3443

matklad · 2017-01-28T12:40:35Z

Should this be tagged with relnotes?

Version 1.16.0 (2017-03-16) =========================== Language -------- * Lifetimes in statics and consts default to `'static`. [RFC 1623] * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * [Clean up semantics of `self` in an import list][38313] * [`Self` may appear in `impl` headers][38920] * [`Self` may appear in struct expressions][39282] Compiler -------- * [`rustc` now supports `--emit=metadata`, which causes rustc to emit a `.rmeta` file containing only crate metadata][38571]. This can be used by tools like the Rust Language Service to perform metadata-only builds. * [Levenshtein based typo suggestions now work in most places, while previously they worked only for fields and sometimes for local variables][38927]. Together with the overhaul of "no resolution"/"unexpected resolution" errors (#[38154]) they result in large and systematic improvement in resolution diagnostics. * [Fix `transmute::<T, U>` where `T` requires a bigger alignment than `U`][38670] * [rustc: use -Xlinker when specifying an rpath with ',' in it][38798] * [`rustc` no longer attempts to provide "consider using an explicit lifetime" suggestions][37057]. They were inaccurate. Stabilized APIs --------------- * [`VecDeque::truncate`] * [`VecDeque::resize`] * [`String::insert_str`] * [`Duration::checked_add`] * [`Duration::checked_sub`] * [`Duration::checked_div`] * [`Duration::checked_mul`] * [`str::replacen`] * [`str::repeat`] * [`SocketAddr::is_ipv4`] * [`SocketAddr::is_ipv6`] * [`IpAddr::is_ipv4`] * [`IpAddr::is_ipv6`] * [`Vec::dedup_by`] * [`Vec::dedup_by_key`] * [`Result::unwrap_or_default`] * [`<*const T>::wrapping_offset`] * [`<*mut T>::wrapping_offset`] * `CommandExt::creation_flags` * [`File::set_permissions`] * [`String::split_off`] Libraries --------- * [`[T]::binary_search` and `[T]::binary_search_by_key` now take their argument by `Borrow` parameter][37761] * [All public types in std implement `Debug`][38006] * [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327] * [`Ipv6Addr` implements `From<[u16; 8]>`][38131] * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [std: Fix partial writes in `LineWriter`][38062] * [std: Clamp max read/write sizes on Unix][38062] * [Use more specific panic message for `&str` slicing errors][38066] * [`TcpListener::set_only_v6` is deprecated][38304]. This functionality cannot be achieved in std currently. * [`writeln!`, like `println!`, now accepts a form with no string or formatting arguments, to just print a newline][38469] * [Implement `iter::Sum` and `iter::Product` for `Result`][38580] * [Reduce the size of static data in `std_unicode::tables`][38781] * [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`, `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement `Display`][38909] * [`Duration` implements `Sum`][38712] * [`String` implements `ToSocketAddrs`][39048] Cargo ----- * [The `cargo check` command does a type check of a project without building it][cargo/3296] * [crates.io will display CI badges from Travis and AppVeyor, if specified in Cargo.toml][cargo/3546] * [crates.io will display categories listed in Cargo.toml][cargo/3301] * [Compilation profiles accept integer values for `debug`, in addition to `true` and `false`. These are passed to `rustc` as the value to `-C debuginfo`][cargo/3534] * [Implement `cargo --version --verbose`][cargo/3604] * [All builds now output 'dep-info' build dependencies compatible with make and ninja][cargo/3557] * [Build all workspace members with `build --all`][cargo/3511] * [Document all workspace members with `doc --all`][cargo/3515] * [Path deps outside workspace are not members][cargo/3443] Misc ---- * [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies the path to the Rust implementation][38589] * [The `armv7-linux-androideabi` target no longer enables NEON extensions, per Google's ABI guide][38413] * [The stock standard library can be compiled for Redox OS][38401] * [Rust has initial SPARC support][38726]. Tier 3. No builds available. * [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No builds available. * [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379] Compatibility Notes ------------------- * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * In this release, references to uninhabited types can not be pattern-matched. This was accidentally allowed in 1.15. * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [Clean up semantics of `self` in an import list][38313] [37057]: rust-lang/rust#37057 [37761]: rust-lang/rust#37761 [38006]: rust-lang/rust#38006 [38051]: rust-lang/rust#38051 [38062]: rust-lang/rust#38062 [38062]: rust-lang/rust#38622 [38066]: rust-lang/rust#38066 [38069]: rust-lang/rust#38069 [38131]: rust-lang/rust#38131 [38154]: rust-lang/rust#38154 [38274]: rust-lang/rust#38274 [38304]: rust-lang/rust#38304 [38313]: rust-lang/rust#38313 [38314]: rust-lang/rust#38314 [38327]: rust-lang/rust#38327 [38401]: rust-lang/rust#38401 [38413]: rust-lang/rust#38413 [38469]: rust-lang/rust#38469 [38559]: rust-lang/rust#38559 [38571]: rust-lang/rust#38571 [38580]: rust-lang/rust#38580 [38589]: rust-lang/rust#38589 [38670]: rust-lang/rust#38670 [38712]: rust-lang/rust#38712 [38726]: rust-lang/rust#38726 [38781]: rust-lang/rust#38781 [38798]: rust-lang/rust#38798 [38909]: rust-lang/rust#38909 [38920]: rust-lang/rust#38920 [38927]: rust-lang/rust#38927 [39048]: rust-lang/rust#39048 [39282]: rust-lang/rust#39282 [39379]: rust-lang/rust#39379 [`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add [`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div [`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul [`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub [`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions [`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4 [`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6 [`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default [`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4 [`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6 [`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str [`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off [`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key [`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by [`VecDeque::resize`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize [`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate [`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat [`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen [cargo/3296]: rust-lang/cargo#3296 [cargo/3301]: rust-lang/cargo#3301 [cargo/3443]: rust-lang/cargo#3443 [cargo/3511]: rust-lang/cargo#3511 [cargo/3515]: rust-lang/cargo#3515 [cargo/3534]: rust-lang/cargo#3534 [cargo/3546]: rust-lang/cargo#3546 [cargo/3557]: rust-lang/cargo#3557 [cargo/3604]: rust-lang/cargo#3604 [RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md

rust-highfive assigned brson Dec 22, 2016

rust-highfive assigned alexcrichton and unassigned brson Dec 22, 2016

matklad force-pushed the path-dep-non-ws branch from c1e9a41 to 30d394f Compare December 22, 2016 19:16

matklad commented Dec 22, 2016

View reviewed changes

matklad added 2 commits January 14, 2017 23:52

Path deps outside workspace are not members

07c667f

closes rust-lang#3192

Document new rules about path deps in workspace

c8e861c

matklad force-pushed the path-dep-non-ws branch from 4420cf2 to c8e861c Compare January 14, 2017 20:54

More test for workspaces \w path dependencies

7429347

SergioBenitez mentioned this pull request Jan 15, 2017

Workspaces: Add a way to disable transitive inclusion of path dependencies #3192

Closed

matklad mentioned this pull request Jan 16, 2017

Find workspace root via encompassing member #3549

Closed

bors merged commit 7429347 into rust-lang:master Jan 18, 2017

matklad mentioned this pull request Jan 18, 2017

Fix a test. #3558

Merged

bors added a commit that referenced this pull request Jan 18, 2017

Auto merge of #3558 - matklad:fix-test, r=alexcrichton

5363c80

Fix a test. That's the test we've discussed in #3443

matklad mentioned this pull request Jan 28, 2017

Find workspace via workspace_root link in containing member #3562

Merged

alexcrichton added the relnotes Release-note worthy label Jan 29, 2017

matklad deleted the path-dep-non-ws branch February 14, 2017 09:59

ehuss added this to the 1.16.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path deps outside workspace are not members #3443

Path deps outside workspace are not members #3443

matklad commented Dec 22, 2016

rust-highfive commented Dec 22, 2016

matklad commented Dec 22, 2016

matklad Dec 22, 2016 •

edited

Loading

matklad commented Dec 29, 2016

alexcrichton commented Dec 29, 2016

matklad commented Dec 29, 2016

matklad commented Dec 29, 2016 •

edited

Loading

phil-opp commented Dec 30, 2016

matklad commented Jan 2, 2017 •

edited

Loading

matklad commented Jan 3, 2017 •

edited

Loading

alexcrichton commented Jan 6, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 13, 2017

matklad commented Jan 13, 2017

alexcrichton commented Jan 13, 2017

alexcrichton commented Jan 15, 2017

bors commented Jan 15, 2017

bors commented Jan 16, 2017

bors commented Jan 16, 2017

alexcrichton commented Jan 16, 2017

matklad commented Jan 16, 2017 •

edited

Loading

bors commented Jan 17, 2017

bors commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

bors commented Jan 18, 2017

bors commented Jan 18, 2017

matklad commented Jan 28, 2017

Path deps outside workspace are not members #3443

Path deps outside workspace are not members #3443

Conversation

matklad commented Dec 22, 2016

rust-highfive commented Dec 22, 2016

matklad commented Dec 22, 2016

matklad Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

matklad commented Dec 29, 2016

alexcrichton commented Dec 29, 2016

matklad commented Dec 29, 2016

matklad commented Dec 29, 2016 • edited Loading

phil-opp commented Dec 30, 2016

matklad commented Jan 2, 2017 • edited Loading

matklad commented Jan 3, 2017 • edited Loading

alexcrichton commented Jan 6, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 13, 2017

matklad commented Jan 13, 2017

alexcrichton commented Jan 13, 2017

alexcrichton commented Jan 15, 2017

bors commented Jan 15, 2017

bors commented Jan 16, 2017

bors commented Jan 16, 2017

alexcrichton commented Jan 16, 2017

matklad commented Jan 16, 2017 • edited Loading

bors commented Jan 17, 2017

bors commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

matklad commented Jan 17, 2017

alexcrichton commented Jan 17, 2017

bors commented Jan 18, 2017

bors commented Jan 18, 2017

matklad commented Jan 28, 2017

matklad Dec 22, 2016 •

edited

Loading

matklad commented Dec 29, 2016 •

edited

Loading

matklad commented Jan 2, 2017 •

edited

Loading

matklad commented Jan 3, 2017 •

edited

Loading

matklad commented Jan 16, 2017 •

edited

Loading