Skip to content
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 new: option to suppress automatic addition to workspace #14501

Closed
j-n-f opened this issue Sep 5, 2024 · 5 comments
Closed

cargo new: option to suppress automatic addition to workspace #14501

j-n-f opened this issue Sep 5, 2024 · 5 comments
Labels
A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-new

Comments

@j-n-f
Copy link

j-n-f commented Sep 5, 2024

Problem

This is similar to but not quite the same as #13985

I'm building a tool which calls cargo new as a subprocess. That tool has a root Cargo.toml which specifies a few workspaces.

When testing the tool I do something like cargo run -- new ../some/path or cargo run -- new path. When I do so, cargo new runs as a subprocess and it indicates that it's adding the new package to the workspace list (even if the folder is outside of the project, i.e. "../"). Because I'm only testing, I want to suppress this behaviour.

Without a means to do this, I have to delete the folder, and edit the root Cargo.toml every time I test.

Proposed Solution

99% of the time it seems correct (and convenient), but I'd like a flag or environment variable to suppress this unexpected automatic behaviour.

Notes

No response

@j-n-f j-n-f added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Sep 5, 2024
@epage
Copy link
Contributor

epage commented Sep 5, 2024

If we're adding relative to current_dir instead of --path, then I'd consider that a bug.

Otherwise, we've previously decided that the command is intended for user interactions and not programmatic use and are unlikely to add features for it.

@j-n-f
Copy link
Author

j-n-f commented Sep 6, 2024

I'm not sure what the --path argument is, I don't see that in documentation.

Just to clarify, when you say "adding relative to current_dir", do you mean that updating Cargo.toml workspaces when adding outside of the current_dir/project root is a bug?

For example:

$ cargo new project-root
$ cd project-root
$ cargo new some-workspace

Update Cargo.toml:

[workspace]
members = [
    "some-workspace"
]
resolver = "2"

Then do:

$ cargo new ../outside-root
    Creating binary (application) `outside-root` package
      Adding `outside-root` as member of workspace at `/home/USER/project-root`
warning: compiling this new package may not work due to invalid workspace configuration

workspace member `/home/USER/outside-root/Cargo.toml` is not hierarchically below the workspace root `/home/USER/project-root/Cargo.toml`

And now Cargo.toml looks like:

[workspace]
members = [ "../outside-root",
    "some-workspace"
]
resolver = "2"

I understand the decision to prioritize interactive use, but according to what you've said maybe there's a bug here.

Version info

$ cargo -V
cargo 1.81.0 (2dbb1af80 2024-08-20)

@epage
Copy link
Contributor

epage commented Sep 6, 2024

Sorry, I meant the positional <path>, not --path (forgot it was positional).

Looks like the code is trying to find the workspace for the manifest but I think we aren't handling ../ correctly in that logic

let manifest_path = path.join("Cargo.toml");
if let Ok(root_manifest_path) = find_root_manifest_for_wd(&manifest_path) {

We've accepted user input, made it absolute, but never called paths::normalize_path on it.

epage added a commit to epage/cargo that referenced this issue Sep 6, 2024
We were correctly doing this for cases like `cargo new foo` or
`cargo new deeper/than/this/directory/foo` but not `cargo new ../foo`.

This came up when discussing rust-lang#14501
@epage
Copy link
Contributor

epage commented Sep 6, 2024

Posted #14505 with a fix for that situation.

epage added a commit to epage/cargo that referenced this issue Sep 6, 2024
We were correctly doing this for cases like `cargo new foo` or
`cargo new deeper/than/this/directory/foo` but not `cargo new ../foo`.

This came up when discussing rust-lang#14501
epage added a commit to epage/cargo that referenced this issue Sep 9, 2024
We were correctly doing this for cases like `cargo new foo` or
`cargo new deeper/than/this/directory/foo` but not `cargo new ../foo`.

This came up when discussing rust-lang#14501
bors added a commit that referenced this issue Sep 9, 2024
fix(new): Add to workspace relative to manifest, not current-dir

### What does this PR try to resolve?

We were correctly doing this for cases like `cargo new foo` or
`cargo new deeper/than/this/directory/foo` but not `cargo new ../foo`.

This came up when discussing #14501

### How should we test and review this PR?

### Additional information
dingxiangfei2009 pushed a commit to dingxiangfei2009/cargo that referenced this issue Sep 17, 2024
We were correctly doing this for cases like `cargo new foo` or
`cargo new deeper/than/this/directory/foo` but not `cargo new ../foo`.

This came up when discussing rust-lang#14501
@j-n-f
Copy link
Author

j-n-f commented Sep 25, 2024

I think #14505 resolves the issue. Thanks @epage.

@j-n-f j-n-f closed this as completed Sep 25, 2024
@weihanglo weihanglo added Command-new A-workspaces Area: workspaces and removed S-triage Status: This issue is waiting on initial triage. labels Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-new
Projects
None yet
Development

No branches or pull requests

3 participants