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

Running 'cargo new' with 'workspace.package.name' set creates an invalid manifest #13258

Open
TheNeikos opened this issue Jan 7, 2024 · 7 comments
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug Command-new S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@TheNeikos
Copy link
Contributor

TheNeikos commented Jan 7, 2024

Problem

I have a workspace that has workspace.package.name set. I am aware that it is not a supported key.

When creating a new crate with cargo new it will set package.name.workspace = true. This is not supported, and will then fail to build the crate.

Steps

  1. Workspace with workspace.package.name set to a string
  2. cargo new
  3. Observe it fail to build

Possible Solution(s)

Special case 'name' to not be inherited, even if it is set in the workspace?

Notes

No response

Version

cargo 1.75.0 (1d8b05cdd 2023-11-20)
release: 1.75.0
commit-hash: 1d8b05cdd1287c64467306cf3ca2c8ac60c11eb0
commit-date: 2023-11-20
host: x86_64-unknown-linux-gnu
libgit2: 1.7.1 (sys:0.18.1 vendored)
libcurl: 8.4.0-DEV (sys:0.4.68+curl-8.4.0 vendored ssl:OpenSSL/1.1.1u)
ssl: OpenSSL 1.1.1u  30 May 2023
os: NixOS 24.5.0 [64-bit]
@TheNeikos TheNeikos added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jan 7, 2024
@TheNeikos
Copy link
Contributor Author

Oh I forgot to mention, but this may be a separate issue, but the error is cryptic to what the problem actually is:

error: failed to load manifest for workspace member `/home/neikos/projects/test/crates/builder`

Caused by:
  failed to parse manifest at `/home/neikos/projects/test/crates/builder/Cargo.toml`

Caused by:
  TOML parse error at line 1, column 1
    |
  1 | [package]
    | ^^^^^^^^^
  invalid type: map, expected a string

@Muscraft
Copy link
Member

Muscraft commented Jan 7, 2024

The reason this is happening is that cargo new copies [workspace.package] if it exists and then tries to add each key to [package]. In this case, workspace.package.name is being copied as it is set within workspace.package. Fixing this would require cargo to copy only certain keys, but keeping that list in sync with what can be inherited could be tough.

As for the unhelpful error message, #13172 might help some, but the underlying cause is due to how things are being deserialized. I am not sure if there is an issue tracking for this yet.

@TheNeikos
Copy link
Contributor Author

if key == "edition" && opts.edition.is_some() {
continue;
}

As an outsider, special casing 'name' seems correct? In so far as that I don't see it ever making sense to be inherited.

@epage
Copy link
Contributor

epage commented Jan 8, 2024

workspace.package.name is not a valid key. We should add "unused key" warnings to this table to help people identify these problems which will reduce the chance they will be set for cargo new to copy.

I'd prefer not to have an allowlist of keys to copy due to the risk of forgetting to update it.

@epage
Copy link
Contributor

epage commented Jan 8, 2024

#13263 confirmed we are warning on workspace.package.name. If you know the field is unused, is there a reason you are setting it such that you ran into the cargo new issue?

bors added a commit that referenced this issue Jan 8, 2024
test(manifest): Verify we warn on unused workspace.package fields

I assumed from #13258 that we didn't warn but apparently we do.  Figured it'd still be good to keep the test around.
@TheNeikos
Copy link
Contributor Author

I am setting it, as I am building the workspace as a whole in a nix derivation. Derivations usually have a short descriptive name, this is where it gets pulled from.

But I don't think that this is something cargo should care/worry about. Ultimately, a more descriptive error in the case that package.name.workspace fails to parse should be enough to understand why it broke.

@epage
Copy link
Contributor

epage commented Jan 8, 2024

I am setting it, as I am building the workspace as a whole in a nix derivation. Derivations usually have a short descriptive name, this is where it gets pulled from.

This is incorrect. We reserve the right to add fields that won't be compatible with how people might use them.

The place to add a custom field for a tool is workspace.metadata / package.metadata

stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this issue Feb 28, 2024
@weihanglo weihanglo added Command-new S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. A-workspace-inheritance Area: workspace inheritance RFC 2906 and removed S-triage Status: This issue is waiting on initial triage. labels Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug Command-new S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

4 participants