Implement path-bases (RFC 3529) 2/n: support for nested packages #14416
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-manifest
I believe the existing behavior is what is "expected" and this PR is a regression in Cargo. EDIT: To expand on this, configuration is environmental configuration and not project configuration. The latter is what |
Ah, ok, #2930 is what I was tripping over when writing my test: I'll adjust the test and revert the regression. I'll also add a note to the docs to mention this: if you depend on a package that uses a path base, then that path base must be defined in the configuration loaded for the root package. |
|
Interesting timing since I called this out in https://blog.rust-lang.org/inside-rust/2024/08/15/this-development-cycle-in-cargo-1.81.html#path-bases as a case that can't be supported
(it was as I was writing up that section that I connected the dots on this) |
| write_config_at( | ||
| paths::home().join(".cargo/config.toml"), | ||
| r#" | ||
| [path-bases] | ||
| submodules = '../dep1/submods' | ||
| "#, | ||
| ); |
There was a problem hiding this comment.
imo writing path-bases on behalf of git dependencies seems very questionable
- Constructing the right absolute path is a mess
- Constructing the right relative path is a mess
If you want to test RecursivePathSource, https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#paths-overrides is another user of that tyoe
| let workspace_root_cell: LazyCell<PathBuf> = LazyCell::new(); | ||
|
|
||
| for (p, base) in nested.iter() { | ||
| let p = if let Some(base) = base { | ||
| let workspace_root = || { | ||
| workspace_root_cell | ||
| .try_borrow_with(|| { | ||
| find_workspace_root(&manifest_path, gctx)? | ||
| .ok_or_else(|| anyhow!("failed to find a workspace root")) | ||
| }) | ||
| .map(|p| p.as_path()) | ||
| }; | ||
| // Pass in `None` for the `cargo-features` not to skip verification: when the | ||
| // package is loaded as a dependency, then it will be checked. | ||
| match lookup_path_base(base, gctx, &workspace_root, None) { | ||
| Ok(base) => Cow::Owned(base.join(p)), | ||
| Err(err) => { | ||
| errors.push(err); | ||
| continue; | ||
| } | ||
| } | ||
| } else { | ||
| Cow::Borrowed(p) | ||
| }; | ||
| let path = paths::normalize_path(&path.join(p.as_path())); |
There was a problem hiding this comment.
Why is this needed? Aren't we operating on a normalized package and don't we resolve path-bases during normaization?
For either of us to verify that is one of the reasons I recommend splitting commits into
- Test showing the existing behavior
- Fix/feature with test updates to show how the behavior changed
|
Looks like this change isn't needed: #2930 really confused me. |
Cargo has the ability to discover "nested packages" when loading the dependencies of a package. For example, if a git dependency has a path dependency pointing to somewhere else in the repo, or a submodule, then Cargo needs to discover those dependencies as well.
This change makes the "nested packages" loading code aware of the "path bases" feature so that it can build the correct manifest path for any path dependencies that may be using a
base.RFC: rust-lang/rfcs#3529
Tracking Issue: #14355