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

More helpful error message when a stray Cargo.toml is present #13904

Open
kpreid opened this issue May 11, 2024 · 15 comments
Open

More helpful error message when a stray Cargo.toml is present #13904

kpreid opened this issue May 11, 2024 · 15 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-easy Experience: Easy S-triage Status: This issue is waiting on initial triage.

Comments

@kpreid
Copy link
Contributor

kpreid commented May 11, 2024

Problem

Newcomers to Cargo occasionally end up with a Cargo.toml file that is irrelevant to the project they're trying to run. (It is often in their home directory; perhaps they ran cargo init not understanding what it does, but that's speculation.) When this happens, they get an error message that doesn't really explain what is going on. An example from today:

$ cargo run
error: failed to parse manifest at '/home/user/Cargo.toml'

Caused by:
  this virtual manifest specifies a 'dependencies' section, which is not allowed

New users don't know what a “virtual manifest” is, and are dealing with too many new concepts already to notice that /home/user/Cargo.toml isn't their project directory, nor do they know that it doesn't make sense to have one there. (And this message doesn't even contain the word “workspace”!)

Proposed Solution

Two parts:

  1. When Cargo produces an error, the error is related to an invalid workspace manifest, and the purported workspace manifest path was found in a parent directory (rather than being the direct output of the locate-project algorithm), produce a message which prints the paths of both manifests rather than only the workspace manifest, with some explanation that these are two different manifests and how each relates to the situation.

  2. If the purported workspace manifest is at least two levels up from the project manifest, or is located in the user's home directory, produce an additional help message which speculates that the workspace manifest may be extraneous and should then be either deleted or relocated to avoid nesting unrelated projects.

Notes

@rustbot label +A-diagnostics +A-workspaces +E-easy

@kpreid kpreid 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 May 11, 2024
@rustbot rustbot added A-diagnostics Area: Error and warning messages generated by Cargo itself. A-workspaces Area: workspaces E-easy Experience: Easy labels May 11, 2024
@epage
Copy link
Contributor

epage commented May 29, 2024

I just tried to reproduce this and didn't get any errors. Could you provide more concrete reproduction steps and use cases that lead to these mistaken workspace manifests so we can better consider how the user gets into these situations so we understand what errors to look at and how to improve them?

@kpreid
Copy link
Contributor Author

kpreid commented May 29, 2024

I can't provide concrete steps for the original example, because the user that had that problem didn't tell us how they got there. In general, issues of this type come from users that don't remember what they did, or don't have enough model of Cargo to say what the relevant things they did are. But here is an example sequence of steps which will produce a (different) broken configuration:

$ cargo init
    Creating binary (application) package
note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
$ rm -r src/ # we decided we didn't want this but didn't notice the other file
$ cargo new foo
    Creating binary (application) `foo` package
      Adding `foo` as member of workspace at `/<...>/project-structure-test`
warning: compiling this new package may not work due to invalid workspace configuration

failed to parse manifest at `/<...>/project-structure-test/Cargo.toml`

Caused by:
  no targets specified in the manifest
  either src/lib.rs, src/main.rs, a [lib] section, or [[bin]] section must be present
note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
$ # IDK what that warning means so I will ignore it
$ cd foo
$ cargo check
error: failed to parse manifest at `/<...>/project-structure-test/Cargo.toml`

Caused by:
  no targets specified in the manifest
  either src/lib.rs, src/main.rs, a [lib] section, or [[bin]] section must be present

To be clear, this is an unreasonable sequence of things to do all at once. Imagine, in between the parts, a lot of poking around with file managers and/or editors, trying different cargo commands, and possibly even moving files to different directories (“oh, I don't want my source code here…”). New users don't know what matters and I'm proposing that, for manifest errors involving supposed workspace manifests, Cargo can do a better job of mentioning what things do matter.

@epage
Copy link
Contributor

epage commented May 30, 2024

So what you are wanting to see is something like

$ cargo check
-error: failed to parse manifest at `/<...>/project-structure-test/Cargo.toml`
+error: Could not parse workspace for ...
+
+Caused by:
+    failed to parse manifest at `/<...>/project-structure-test/Cargo.toml`

Caused by:
  no targets specified in the manifest
  either src/lib.rs, src/main.rs, a [lib] section, or [[bin]] section must be present

@epage
Copy link
Contributor

epage commented May 30, 2024

Experimented with this a bit and not thrilled with the results

failures:

---- workspaces::error_if_parent_cargo_toml_is_invalid stdout ----
running `/home/epage/src/personal/cargo/target/debug/cargo check`
thread 'workspaces::error_if_parent_cargo_toml_is_invalid' panicked at tests/testsuite/workspaces.rs:1420:10:

test failed running `/home/epage/src/personal/cargo/target/debug/cargo check`
error: stderr did not match:
1   1     error: expected `.`, `=`
2   2      --> ../Cargo.toml:1:9
3   3       |
4   4     1 | Totally not a TOML file
5   5       |         ^
6   6       |
    7    +error: could not parse workspace for /home/epage/src/personal/cargo/target/tmp/cit/t35/foo/bar/Cargo.toml


other output:


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- workspaces::new_warning_with_corrupt_ws stdout ----
running `/home/epage/src/personal/cargo/target/debug/cargo new bar`
thread 'workspaces::new_warning_with_corrupt_ws' panicked at tests/testsuite/workspaces.rs:1147:10:

test failed running `/home/epage/src/personal/cargo/target/debug/cargo new bar`
error: stderr did not match:
1   1         Creating binary (application) `bar` package
2   2     error: expected `.`, `=`
3   3      --> Cargo.toml:1:5
4   4       |
5   5     1 | asdf
6   6       |     ^
7   7       |
8   8     warning: compiling this new package may not work due to invalid workspace configuration
9   9
    10   +could not parse workspace for /home/epage/src/personal/cargo/target/tmp/cit/t58/foo/bar/Cargo.toml
10  11    note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

Part of the problem is that annotate-snippets reporting is handled very differently than other errors and its less ideal in this situation. We are also wanting to move as many errors to annotate-snippets as possible.

@kpreid
Copy link
Contributor Author

kpreid commented May 30, 2024

So what you are wanting to see is …

More like this:

error: failed to parse manifest at /<...>/project-structure-test/Cargo.toml

help: the current directory, /<...>/project-structure-test/foo/, contains a Cargo.toml manifest file, but is considered a workspace member due to the presence of a manifest file in the parent directory, but that file is currently not a valid workspace manifest. If foo is not intended to be part of a larger project, then move foo/ out of project-structure-test/ or remove the Cargo.toml file from project-structure-test/.

@epage
Copy link
Contributor

epage commented May 31, 2024

Whichever way, that runs into the same problems. More even since we can't add help: to regular errors, only annotate-snippet errors. We sometimes emulate it but the result isn't the best.

btw that help: is too long that it would just lose people.

@onkoe
Copy link

onkoe commented Jun 2, 2024

I'd enjoy something simple, like:

error: Failed to parse a package description in this workspace's Cargo.toml file.

Caused by:
	the surrounding workspace doesn't match its Cargo.toml file. 

That moves away from the "workspace manifest" terminology while remaining helpful for more-experienced users.

If there is a way to add a help section, and we can tell if there's some issue with the package location, I'd like to see something similar to this:

help: Try checking the placement of your Cargo.toml file and its packages. 

Also, if you do wish to continue using the manifest terminology, linking to the glossary may be neccessary: https://doc.rust-lang.org/cargo/appendix/glossary.html#manifest

In any case, if you have any advice for my input, please let me know! I'm looking to contribute more to Cargo and Rust in general. 😄

@dylanopen
Copy link

I stumbled across this thread trying to fix the same problem but for a different reason. For some reason, I'd accidentally deleted a character of the 'package' TOML header so it read 'packag'.

All fixed now, but I wanted to know why Cargo just blindly accepted [packag] as a heading instead of throwing an error / warning about an unrecognised keyword.

Furthermore, why does Cargo just assume it's a virtual workspace just because the [package] header is missing? Any answers are appreciated :)

[packag]
name = "li"
version = "0.0.2"
edition = "2021"

[dependencies]
bevy = { version = "0.14.0", features = ["dynamic_linking"] }
serde = "1.0.204"

...

opt-level = 3

@epage
Copy link
Contributor

epage commented Jul 16, 2024

I wanted to know why Cargo just blindly accepted [packag] as a heading instead of throwing an error / warning about an unrecognised keyword.

Instead of erroring on unused keys, Cargo warns. This is helpful when we are adding new functionality to Cargo.toml that doesn't otherwise require an MSRV bump, like adding [lints].

Furthermore, why does Cargo just assume it's a virtual workspace just because the [package] header is missing?

A package must have at least

  • [package] (or [project])
  • [workspace]

I can see improving the error if neither is present, rather than falling through to assuming its a virtual workspace.

@dylanopen
Copy link

@epage Thanks for the info!

@onkoe

This comment was marked as outdated.

epage added a commit to epage/cargo that referenced this issue Jul 17, 2024
@epage

This comment was marked as off-topic.

bors added a commit that referenced this issue Jul 17, 2024
fix(toml): Improve error on missing package and workspace

That the error was confused was brought up in #13904
@onkoe

This comment was marked as off-topic.

@onkoe

This comment was marked as off-topic.

@epage

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-easy Experience: Easy S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

5 participants