Cargo: Tracking issue for Deduplicate Cargo workspace information RFC

[Mark-Simulacrum]

This is a tracking issue for the RFC: Deduplicate Cargo workspace information (https://github.com/rust-lang/rfcs/pull/2906).

Steps:

• [ ] Implement the RFC (cc @rust-lang/cargo -- can anyone write up mentoring
  instructions?)
• [ ] Adjust documentation (see instructions on rustc-dev-guide)
• [ ] Stabilization PR (see instructions on rustc-dev-guide)

Unresolved questions:

• One thing we'll want to resolve for sure is nailing down all the syntactical decision here, which is expected to evolve through consensus.
• It's not clear how complex an implementation of this proposal will be in Cargo. It could be prohibitively complex, but it's hoped that it's a relatively simple refactoring to implement this in Cargo.

[Mark-Simulacrum]

cc @ehuss @alexcrichton -- please update the description wrt to documentation/stabilization and potentially some of the unresolved questions, I myself don't really know what Cargo procedures are like these days.

[alexcrichton]

Ok I've got a moment to write up some instructions, I shall do so! I think this'll be posted on TWiR soon to see if others are interested, so I'm hoping that I can help guide someone interested to implement this.

To get started, I'd probably break this down based on the reference level explanation:

Update [workspace] parsing

First you'll probably want to implement [workspace] parsing updates.

That'll be done by updating this structure with all the relevant fields. You'll need to take care when handling the dependencies, being careful to share code with existing dependency parsing.

Once the fields are parsed you'll want to carry them over to WorkspaceRootConfig which contains the format of each field that is persisted throughout the lifetime of Cargo itself. This'll be where dependencies are elaborated (again, sharing code from before) into a Dependency structure for example.

At this point you should be able to write some tests. All our tests are in tests/testsuite/*.rs and you'll probably want to make a new test suite at something like tests/testsuite/deduplicate_workspace.rs and add mod deduplicate_workspace; to tests/testsuite/main.rs. You can then run tests with cargo test deduplicate_workspace.

Placeholder { workspace = true }

Next you can tackle workspace = true directives in Cargo.toml. The best way to implement this is probably going to be making a generic thing like:

pub enum MaybeWorkspace<T> {
    Defined(T),
    Workspace { workspace: bool },
}

If possible, the best way to handle this will be to not persist this MaybeWorkspace type all throughout Cargo. Ideally a Manifest in Cargo would retain the actual value for each manifest. I'm not sure whether this is available at this time, but in theory you'll want to "elaborate" a manifest with a WorkspaceRootConfig on-hand where you can resolve any MaybeWorkspace<T> with the &WorkspaceRootConfig next to it.

New dependency directives

To implement new dependency directives you'll update this structure along with this method. Again ideally there will be a new &WorkspaceRootConfig argument which allows resolving all workspace = true dependencies by basically clone-ing the Dependency from the workspace.

Inferring version directives

For inferring version on path dependencies that will need to happen on the packaging phase of Cargo. First you'll want to update where the error is generated today. Next you'll want to update to_registry_toml which will update the manifest (or probably a clone of the current manifest) with version numbers.

---

And I think that should at least be enough to get almost all the way there! I'm sure there's things that will likely come up in review and questions a long the way. The hardest part will probably be getting WorkspaceRootConfig present while parsing other manifests, that may require some refactoring in Cargo (or maybe there's a better way I'm not seeing!)

[hbina]

I am not sure if I am capable enough to work on this, but I would like to follow the development to see how things work. Does https://github.com/alexcrichton/toml-rs also needs to be updated?

[kornelski]

The general-purpose toml crate doesn't need any changes, because this feature uses standard TOML syntax.

I will need to update my Manifest-parsing crate: https://gitlab.com/crates.rs/cargo_toml (help welcome)

[yaymukund]

I've started work on this.

Thank you for the super detailed writeup, @alexcrichton . You can see my progress here, but for convenience I'll keep this list updated:

EDIT: The PR is open, so please check there.

Aiming to complete this list relatively soon - hopefully ~tomorrow~ in the coming week.

[yaymukund]

Hello, I've been making steady progress on this and wanted to give an update:

The WorkspaceRootConfig is not available when we're parsing the manifest, so we have to locate and parse it somewhere in do_read_manifest(). Currently the logic for finding the root and parsing its Config.toml lives in struct Workspace itself and makes use of workspace.packages to cache parsed tomls.

I see a few different ways to proceed:

1. Require a Workspace in do_read_manifest somehow. Workspace::new() currently parses the entire workspace, so this could get complicated.
2. Extract a new ParserCache out of Workspace. It would sit in front of the Packages cache, accept a manifest_path argument, and return a deserialized TomlManifest.
3. Ignore the packages registry entirely. This would be easiest, but worst-case would require parsing every toml in the workspace every time you parse any manifest. I don't think this is a good solution, but mention it for completeness' sake.

I'm not blocked - am currently leaning towards approach 2 - but please let me know if I'm missing anything or if there's a better way to proceed.

Edit: In case anyone's seeing this now, I went with 2 and it seems to work :)

[salewski]

Hi Folks,

First of all, a big "thank you" to @alexcrichton, @yaymukund, and everyone else who has put in so much effort on this feature. As my projects are growing in size, being able to manage dependency versions from a single location in the workspace Cargo.toml file will be a very welcome feature.

I do have a question about the following snippet from the "New dependency directives" section of rust-lang/rfcs#2906:

New dependency directives

...

• features - this indicates, as usual, that extra features are being enabled
  over the already-enabled features in the directive found in
  [workspace.dependencies]. The result set of enabled features is the union of
  the features specified inline with the features specified in the directive in
  the workspace table.

For now if a workspace = true dependency is specified then also specifying the
default-features value is disallowed. The default-features value for a
directive is inherited from the [workspace.dependencies] declaration, which
defaults to true if nothing else is specified.

Should the "For now..." be understood as meaning "we want to revisit this, but it's not a Day One concern"?

The reason I ask is that it seems to preclude the ability to inherit from the workspace the version number for a given dependency if one or more of the package-level configurations requires a non-default feature set in only some instances. It would be nice if a package-level dependency were to be able to benefit from the centralized configuration of the crate 'version' number spec, yet still be able to specify a non-default 'features' set locally in the package-level config.

I think variations of the 'features' specification for dependent packages could benefit quite a bit from centralized configuration at the workspace level, but the need to specify 'features' at the package-level should not preclude the ability to still inherit the 'version' number from the workspace (at least not in the long term).

Two scenarios come to mind:

1. A workspace with some number of packages that all have a dependency on a third-party crate of the same version, and for the most part all need the same set of features enabled. However one package needs that same version of the third-party crate, but with a different set of features enabled (not just additional features).

2. A workspace with some number of packages that all have a dependency on a third-party crate of the same version, but all of the packages need a different set of features enabled.

If I understand rust-lang/rfcs#2906 correctly:

• In scenario (1), most of the packages will benefit from the proposed workspace enhancements: They will all use the inherited version number and the inherited features configuration from the workspace.

  However, the package that needs the different set of features from the third-party will have to fully specify the dependency, its version, and the feature set that it needs in the package's Cargo.toml file. In particular, that package will not benefit from the ability to control the dependency version at the workspace level, so will need to have that dep's version number kept in sync with the workspace Cargo.toml manually.

  We would like to write:
  # my-workspace/Cargo.toml [workspace] members = [ 'thromdimbulator1', 'thromdimbulator2', ] ... [dependencies] [dependencies.gick] version = 1973.9.12 default-features = false features = ['skrux']
  and:
  # my-workspace/thromdimbulator1/Cargo.toml ... [dependencies.gick] version = { workspace = true } # inherits: default-features = false # inherits: features = ['skrux']
  and:
  # my-workspace/thromdimbulator2/Cargo.toml ... [dependencies.gick] version = { workspace = true } default-features = false features = ['snoor']
  But instead would have to write the thromdimbulator2/Cargo.toml file as:
  # my-workspace/thromdimbulator2/Cargo.toml ... [dependencies.gick] # Have to explicitly specify the version, and keep it in sync with the parent workspace... version = 1973.9.12 # ...because we need a custom feature set: default-features = false features = ['snoor']

• In scenario(2), none of the packages benefit from the third-party dep version number declared in the workspace-level Cargo.toml file because they each need a different set of features.

  We would like to write:
  # my-workspace/Cargo.toml [workspace] members = [ 'thromdimbulator3', 'thromdimbulator4', 'thromdimbulator5', ] ... [dependencies] [dependencies.gick] version = 1973.9.12 # no features explicitly configured at workspace level b/c each package needs a different set
  and:
  # my-workspace/thromdimbulator3/Cargo.toml ... [dependencies.gick] version = { workspace = true } default-features = false features = ['goor', 'skrux']
  and:
  # my-workspace/thromdimbulator4/Cargo.toml ... [dependencies.gick] version = { workspace = true } default-features = false features = ['goor', 'snux']
  and:
  # my-workspace/thromdimbulator5/Cargo.toml ... [dependencies.gick] version = { workspace = true } default-features = false features = ['goor', 'snoor']
  But instead would not be able to use version = { workspace = true } in any of the package-level Cargo.toml files. All of the package-level declarations of the dep need to have their version numbers kept in sync manually.

With the ability to inherit the 'version' number of a dep so tightly co-mingled with that dep's 'features' configuration, the workspace enhancement will be of only limited usefulness wherever different sets of features for a dep are in use within the different packages of a workspace. And since feature sets are so widely used, I would expect many users to stumble over this caveat and find the behavior counter-intuitive.

None of this makes us worse off than we are today, where we need to manually keep the version numbers in sync. Especially since we have cargo-edit, which lets us do, e.g., cargo upgrade --workspace 'gick@=1973.9.12'.

[alexcrichton]

@salewski it's always something we can tweak at a later date (or even now before stabilization), but in our discussions amongst the Cargo team awhile back the conclusion was that we couldn't really think of a non-surprising way of dealing with default-features. For projects that need to toggle it we figured that you could disable default features in [workspace.dependencies] and then enable the default feature in directives as necessary. This should still give the same benefit of a workspace-level definition and dependencies enabling the default feature would be just a little more wordy.

[salewski]

For projects that need to toggle it we figured that you could disable default features in [workspace.dependencies] and then enable the default feature in directives as necessary. This should still give the same benefit of a workspace-level definition and dependencies enabling the default feature would be just a little more wordy.

@alexcrichton Ah, I see. That's not bad at all, and addresses my main concern about managing the version numbers centrally. Thanks for clarifying that!

The piece I was missing is the fact that specifying a non-additive, non-default feature set at the package level will no longer require that default-features = false be set locally (in the package-level Cargo.toml file), as long as that value has been set at the workspace level.

I now see that it is right there in the text of the rfc that I quoted, but I suppose the way I was reading it was too heavily shaped by current practice (where default-features = false and features = [...] always appear as a pair in the non-additive case) :-)