-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 update
behaves different with and without workspaces
#3675
Comments
Sorry my STR aren't quite correct... it worked before but for some reason doesn't work now. Gimme a bit to figure out what changed. |
Ok, I updated the cargo-bug repo from my initial comment so that it reproduces the problem properly. It turns out there needs to be one extra "optional" layer in between. But still, the |
Thanks for the report and the reproduction! I think you're running into expected behavior, however. Currently a workspace contains one lock file for all its members, and all that lock file contains all optional dependencies for all members. So following the instructions in your README the first situation doesn't have subproj1 as a member of the workspace, so ipc-channel is omitted. In the second scenario, however, subproj1 is a member so its optional dependency is included. Does that make sense? Currently there's no way to explicitly remove a member from a workspace, but that's tracked by #3192 |
Hm, ok. I guess my mental model of how a workspace is supposed to work is wrong then. We can close this. However, I'd also like to document why my mental model was wrong, so that you can take it into account in future changes to cargo workspaces (although I suspect for backwards compat we're kind of stuck with this now). Upon reading the cargo documentation, it seems that there's two ways to include members into a workspace: one is to put it inside the subtree rooted at the workspace, and the other is to list it in the "members" list. Because these are orthogonal there are four possible combinations: If W is the workspace and A is a crate, then: In my mind both outcomes (2) and (3) are unexpected. I would expect a member to either be in a workspace or standalone, whereas in (2) it is neither and in (3) it is both. Since I was not expecting this behaviour, I subconsciously figured that the more "explicit" behaviour (listing A as a member of W) would take priority so that (2) would get resolved as "A is standalone" and (3) would get resolved as "A gets built as part of W". In this world, subproj1 would be standalone and not get included into the top-level workspace in my reproduction project. Perhaps the documentation on cargo workspaces should be made more explicit to cover these four options? |
I don't think your mental model with "subtree" is quite right, per http://doc.crates.io/manifest.html#the-workspace-section
There's sort of a weird non-symmetric property here which is that you can have a workspace defined at the root of a directory tree, and it can specify members, so you could leave out some crates that are in subdirectories, but |
Ah, you're right. While doing more experimentation I found what appears to be a real bug: https://github.com/staktrace/cargo-bug-2 In this scenario we have this structure:
Where all the Cargo.toml files are consistently pointing to their workspaces and members. But running |
@alexcrichton for my previous comment - Is my understanding wrong, or should I file an issue for that? |
@staktrace yeah that looks like it's a bug, but I wonder if that's just #3586? |
It appears that
cargo update
on a workspace will pull in all the optional dependencies of the members, whereas doingcargo update
on a single member crate will not. This behaviour difference seems like a bug. It is also blocking use of a workspace to reduce build times in the Firefox build, because the extra optional dependencies need to get vendored into the Firefox tree even though they are never used.To illustrate the problem, I created a little test repo with a workspace - https://github.com/staktrace/cargo-bug. The STR to reproduce are in the README file.
/cc @froydnj
The text was updated successfully, but these errors were encountered: