Add named path bases to cargo (v2)

[dpaoliello]

Introduce shared base directories in Cargo configuration files that in turn enable base-relative path dependencies.

Rendered

Draft Implementation PR: rust-lang/cargo#12974

This is a resurrection of @jonhoo's RFC (#3074). Many thanks and kudos to him for the idea, original RFC text and original implementation.

FCP proposed: checkboxes at #3529 (comment)

[runiq]

Mind fixing the 'Rendered' link in the OP? https://github.com/dpaoliello/rfcs/blob/basepath/text/3529-cargo-path-bases.md :)

[dpaoliello]

Mind fixing the 'Rendered' link in the OP? https://github.com/dpaoliello/rfcs/blob/basepath/text/3529-cargo-path-bases.md :)

Oops, my bad, thanks!

[dpaoliello]

Creating a thread on Rust internals to discuss alternative solutions for this issue: https://internals.rust-lang.org/t/integration-with-mono-repos-via-intermediate-directories/20160

[joshtriplett]

We discussed this in today's @rust-lang/cargo meeting. We were generally in favor, but wanted to see a few changes made.

@rfcbot merge

First, this should be base-paths, rather than base_path, for consistency with other manifest fields which use plurals and dashes.

@rfcbot concern rename-to-base-paths

Second, it should be possible to define this in Cargo.toml, not only in config.toml. config.toml or environment variables can always override, but Cargo.toml should be able to define the default values.

@rfcbot concern define-in-manifest-override-in-config

Third, we should restrict the possible names for bases, to reserve space for extension in the future. We felt that base names should be restricted to the same naming as package names: "The name must use only alphanumeric characters or - or _, and cannot be empty."

@rfcbot concern base-names-alphanumeric-with-dash-or-underscore

Finally, this should define a mechanism for Cargo to provide built-in base paths. User-defined base paths should shadow built-in base paths, so that we can add more in the future without creating conflicts. And we should provide a built-in workspace base path that refers to your workspace root.

@rfcbot concern allow-built-in-base-paths-with-shadowing
@rfcbot concern define-built-in-workspace-base-path

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• allow-built-in-base-paths-with-shadowing resolved by Add named path bases to cargo (v2) #3529 (comment)
• base-names-alphanumeric-with-dash-or-underscore resolved by Add named path bases to cargo (v2) #3529 (comment)
• comments (Add named path bases to cargo (v2) #3529 (comment))
• define-built-in-workspace-base-path resolved by Add named path bases to cargo (v2) #3529 (comment)
• define-in-manifest-override-in-config resolved by Add named path bases to cargo (v2) #3529 (comment)
• need-for-manifest resolved by Add named path bases to cargo (v2) #3529 (comment)
• rename-to-base-paths resolved by Add named path bases to cargo (v2) #3529 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dpaoliello]

@joshtriplett I've updated the RFC to address these concerns

Also, I especially love the built-in workspace base path - that's super nice!

[joshtriplett]

@rfcbot resolved rename-to-base-paths

[joshtriplett]

@rfcbot resolved allow-built-in-base-paths-with-shadowing
@rfcbot resolved define-built-in-workspace-base-path

[joshtriplett]

@rfcbot resolved base-names-alphanumeric-with-dash-or-underscore

[joshtriplett]

@rfcbot resolved define-in-manifest-override-in-config

[joshtriplett]

@dpaoliello Thank you for the updates!

[dpaoliello]

The more that I think about having base paths in the manifest, the less that I like it.

If you are using it for convenience, then this suggests that you have a bunch of packages locally which you'd like to reference from each other. In which case, why not drop a config.toml in a common parent directory that then points to each of the useful subdirectories with your packages? If you are putting the base paths in the manifests, then you'll need to duplicate that list into each of your manifests.

The original concern mentioned "default values". I'm not sure how useful that is versus using the patch feature - if you want to override where Cargo looks for packages, then that's what patch is for.

I'm going to scope it out of this RFC, and it can be proposed later if someone has a strong case for it.

[joshtriplett]

I continue to think there's value in placing this in the manifest. We regularly find ourselves wishing that things in the config also existed in the manifest.

However, I'm not going to treat that as a blocking concern. We can add it to the large pile of things that only exist in config and not yet in the manifest. Hopefully we shrink that pile over time.

[dpaoliello]

@epage I've removed support for defining this in the manifest, can you please resolve the need-for-manifest concern?

[Eh2406]

I had missed an important piece of the conversation. Specifically that @joshtriplett's use case is not hypothetical. The second paragraph here is his actual use case. He has two different workspaces and regularly needs to construct path dependencies from one to the other. (I would like to apologize again, and publicly, for the rude thing I said when calling for a non-hypothetical user.) This leads me to being comfortable merging a version of this RFC that does allow setting it in the Cargo.toml.

That being said, adding that functionality later is always a possibility. So if the decision of the team is to accept an RFC that only discusses config, I would not object. Leaving Cargo.toml for later is a two way door.

[dpaoliello]

I had missed an important piece of the conversation. Specifically that @joshtriplett's use case is not hypothetical. The second paragraph here is his actual use case. He has two different workspaces and regularly needs to construct path dependencies from one to the other. (I would like to apologize again, and publicly, for the rude thing I said when calling for a non-hypothetical user.) This leads me to being comfortable merging a version of this RFC that does allow setting it in the Cargo.toml.

That being said, adding that functionality later is always a possibility. So if the decision of the team is to accept an RFC that only discusses config, I would not object. Leaving Cargo.toml for later is a two way door.

I agree that this is a valid use case for this feature, but it does not require path bases in the manifest - they could be specified in the configuration instead. It might be more convenient to have them in the manifest, but given all of the complexity surrounding how to handle workspace vs member manifests I'd rather defer that for now.

[epage]

If we feel the config use case is important on its own, an important reason for splitting out manifest support is we don't block on the extra questions that introduces (workspace vs package, implicit vs explicit inheritance, etc)

[epage]

Let's move this to Future Possibilities and call out the open questions on workspace vs package, implicit vs explicit inheritance, etc.

[epage]

Suggested change

	# Unresolved questions
	[unresolved-questions]: #unresolved-questions
	# Unresolved questions
	[unresolved-questions]: #unresolved-questions
	- What exact names we should use for the table (`path-bases`) and field names (`base`)

I feel uncomfortable with the names but don't think we need to block approval of the RFC on it.

[epage]

Can you add a config/manifest summary here for easy reference on what the proposal is?

[epage]

Suggested change

	Cargo can also provide built-in base paths, for example `workspace` to point to
	the root directory of the workspace. This allows workspace members to reference
	each other without first needing to `../` their way back to the workspace root.
	Cargo will also provide built-in base paths, for example `workspace` to point to
	the root directory of the workspace. This allows workspace members to reference
	each other without first needing to `../` their way back to the workspace root.

The current wording makes it sound like a future possibility

[epage]

What about a package base that is the default value for base?

While this won't necessarily be a value users specify explicitly, I think it can help in modeling this to talk about how this feature works generally and not just in this specific case.

I am fine with this being an Unresolved Question that is left for stabilization.

[epage]

Hmm, if including config, maybe current-dir would be a better name

[epage]

Suggested change

	For example, if the Cargo.toml contains:
	For example, if the `Cargo.toml` contains:

[epage]

Suggested change

	then this path base is defined as the parent directory of the root Cargo.toml of
	then this path base is defined as the parent directory of the root `Cargo.toml` of

[epage]

@epage I've removed support for defining this in the manifest, can you please resolve the need-for-manifest concern?

There is a little more nuance I want to dig into. Will need to get back to this later (soon, I'm back from vacation)

[epage]

Suggested change

	This RFC proposes a mechanism to specify path bases in `config.toml` or
	`Cargo.toml` files which can be used to prepend `path` dependencies. This allows
	This RFC proposes a mechanism to specify path bases in `config.toml` which can be used to prepend `path` dependencies. This allows

As Cargo.toml is deferred out

[epage]

As this is adding a new field to the detailed dependency table, we need to specify how it interacts with workspace inheritance

• Can it be specified in workspace.dependencies? Yes
• Can it be specified in dependencies with workspace = true? No
• How does specifying both of the above interact? N/A

[epage]

Should cargo add be updated to handle this? If so, what would that look like?

[epage]

I would assume the biggest issue for monorepos is scaling the builds. If you use a registry, features like rust-lang/cargo#5931 could help. We have no plans in site for any kind of monorepo caching scheme. This makes me wonder how viable Cargo is more generally for monorepos which makes me wonder how important this feature is.

[epage]

With how intertwined dependencies and patches are in Cargo, this should probably be called out in the main section of the RFC that patches are not intended to be supported.

[epage]

We talked about this in today's Cargo meeting and lean towards going ahead and supporting patches now.

• In theory, we'll get support for nearly free. We'll need to test various corner cases and maybe something will be uncovered that needs development
• Otherwise, we'd have to add explicit error checks and tests for the various corner cases to ensure the error check happens

[epage]

@rfcbot concern comments

[epage]

@rfcbot resolved need-for-manifest

[joshtriplett]

Suggested change

	path base is prepended to the `path` value to produce the actual location where
	path base is prepended to the `path` value (along with a path separator if necessary) to produce the actual location where

(This ensures a base of xyz and a path of abc produces xyz/abc rather than xyzabc.)

[joshtriplett]

Suggested change

	You can also use `base` along with `path` when specifying a `[patch]`. Specifying a `path` and `base` on a `[patch]` is equivalent to specifying just a `path` containing the full path including the prepended base.

[joshtriplett]

Suggested change

	## Support patches
	It may also be useful to be able to use path bases in `patch` and `path` tables.

(Along with the suggestion above adding it.)