Implement workspaces in Cargo

[alexcrichton]

This commit is an implementation of RFC 1525 which specifies the addition of
workspaces to Cargo.

A workspace is a group of crates which are all compiled into the same output
directory and share the same Cargo.lock file. This means that dependencies are
cached between builds as well as dependencies all being shared at the same
versions. An update to any one dependency transitively affects all other members
of the workspace.

Typical repository layouts with a crate at the root and a number of path
dependencies simply need to add the following to the root Cargo.toml:

[workspace]

Otherwise more advanced configuration may be necessary through the
package.workspace or workspace.members keys. More information can be found
as part of RFC 1525.

[rust-highfive]

r? @wycats

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

[alexcrichton]

I wanted to put this up for review, but I don't think that this is ready to merge yet. I believe I've covered the bases of an implementation, tests, and documentation. So far [workspace] and virtual manifests are both implemented.

In implementing this I can only think of one possible catch in the RFC, which is that workspace.members is only filled in automatically if it's not present. The filling in happens transitively through path dependencies. This however does not play nicely with virtual manifests because there are no implicit path dependencies. This means that all virtual manifests would have to exhaustively list out members of the workspace, which can sometimes be tedious. I might recommend tweaking the RFC to say that "path dependencies of workspace members are always transitively included in a workspace". I'm not sure if this is too restrictive, however, as there's no way to turn off this behavior. For now this implements what was in the RFC.

The major piece that this has not implemented yet is the modification to cargo new. The intention was to have cargo new basically automatically manage your workspace so you never have to do so yourself. That is, heuristics like this would be implemented:

• Whenever a new root crate is created, a [workspace] key is filled in.
• Whenever a child crate is created in a subdirectory, it's connected to that workspace.

Unfortunately connecting a crate to a workspace is not always a simple and local operation. We can always provide a pointer back to the workspace (e.g. package.workspace), but the original root must contain the new crate in its workspace. The only way to do that is to list it in workspace.members or list it as a path dependency. Because this is a new crate it probably isn't listed as a path dependency, so it'll have to be added to workspace.members. Eventually, however, it'll probably be added as a path dependency and this won't be necessary.

I've left this unimplemented for now as I'm unsure what the best way to move forward this would be. On one hand if we omit [workspace] as part of cargo new we don't have any problems (e.g. what we do today), but you don't get the benefit of workspaces by default (which probably everyone wants). If we do emit [workspace] by default then we'll at least need to add the ability to read/write TOML which round trips comments and formatting. This would then likely also entail auto-adding an entry to [dependencies] or auto-adding an entry to workspace.members, both of which are implementable but somewhat complicated.

Curious on others' thoughts though!

cc @wycats
cc @brson
cc @rust-lang/tools

[Ericson2314]

I might recommend tweaking the RFC to say that "path dependencies of workspace members are always transitively included in a workspace".

Those transitive deps still need to specify the virtual manifest as workspace root, or be in a subdirectory, for symmetry, right?

[alexcrichton]

No, they're typically lower in the filesystem hierarchy.

[Ericson2314]

Hmm? You mean the subtree with virtual manifest at its root contains the specified members' path dependencies---what I meant by "or be in a subdirectory", or something else?

[alexcrichton]

Ah yes I misread, the original statement you made is correct (it just follows from what was written in the RFC)

[alexcrichton]

ping r? @brson, thoughts?

[brson]

I know I need to read this one closely and I just haven't yet.

[alexcrichton]

Ah yeah sure, we should talk in person tomorrow as well

[brson]

This 'workspace' key is under project, but the RFC says it goes under 'package'. Why the discrepancy?

[alexcrichton]

Oh the [package] and [project] keys are actually synonymous right now. There were initial ideas of how to treat them differently but that never came to fruition, so it's it's just a semi-unfortunate legacy "feature"

[brson]

Great patch. This integrates really cleanly for such a big feature.

[alexcrichton]

I'll work on some better error messages, but @wycats do you have thoughts about the implicitly traveling all path dependencies in the members array unconditionally?

[alexcrichton]

Ok, after some more discussion with @wycats and @brson offline, I've updated the PR with two new pieces:

• Path dependencies are always transitively traversed, including packages which are explicitly mentioned in workspace.members, as well as when some members are explicitly listed.
• cargo new and cargo init will now issue a warning if they detect that a crate was just created with invalid workspace configuration. This is the best we can do for now without automatically updating configuration for you, and should hopefully tie us over to when that happens.

Due to the size of this feature and the concerns of the @rust-lang/tools team, r? @brson but this should probably merge after we branch beta next week.

[alexcrichton]

@larsbergstrom as a heads up, this is what it looks like for me to add workspaces to Servo (probably incomplete) alexcrichton/servo@1094428.

I found it cool that the top-level Cargo.toml has a pretty small workspace.members list, and I imagine it'll also be nice get rid of all those pesky duplicate Cargo.lock files!

It looked like there were spurious rebuilds between switching build and build-geckolib (build and build-cef were fine) but it looked to be related to legitimate issues rather than versions being swapped out.

In any case, just wanted to ensure that it worked at scale, and it looks like it's working like a charm!

[brson]

r=me

[larsbergstrom]

@alexcrichton Thanks for trying that out - it looks REALLY fantastic!

One area of questioning: @SimonSapin is doing some work to help us do a dual-build using Rust Stable for just the geckolib target. Do you have any ideas for how that should integrate into this? And, can we still share the Cargo.lock file across nightly/stable builds?

IIRC, we will need separate target directories for the two builds, but since it's mainly a CI thing, I'm not super concerned about it.

[Ericson2314]

@larsbergstrom perhaps I misunderstand, but Cargo has long been able to compile multiple targets with a single target/ directory. When the --target <triple> flag is passed, the build artifacts go inside target/<triple>.

[larsbergstrom]

@Ericson2314 Aha, yes! The difference here is that it's the same target triple, but with different versions of the Rust compiler itself (the stable versus nightly releases).

[alexcrichton]

@bors: r=brson

[bors]

📌 Commit 8b3933d has been approved by brson

[bors]

⌛ Testing commit 8b3933d with merge 2af552d...

[bors]

💔 Test failed - cargo-cross-linux

[alexcrichton]

@bors: r=brson

[bors]

📌 Commit 0c2ee50 has been approved by brson

[bors]

⌛ Testing commit 0c2ee50 with merge 23f2962...

[bors]

💔 Test failed - cargo-win-msvc-64

[alexcrichton]

@bors: r=brson

[bors]

📌 Commit 9a6aebe has been approved by brson

[bors]

⌛ Testing commit 9a6aebe with merge 4daf6b5...

[bors]

💔 Test failed - cargo-linux-64

[alexcrichton]

@bors: r=brson

[bors]

📌 Commit 58ddb28 has been approved by brson

[bors]

⌛ Testing commit 58ddb28 with merge b036f19...

[bors]

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: brson
Pushing b036f19 to master...