Add validation for example target paths with clearer error messages #16329
Conversation
tests/testsuite/bad_config.rs
Outdated
| .build(); | ||
| p.cargo("check --example fuzz") | ||
| .with_status(101) | ||
| .with_stderr_contains("[ERROR] couldn't read `examples/fuzz`: Is a directory (os error 21)") |
There was a problem hiding this comment.
I'm assuming there isn't a reason to use a _contains test here. Where possible, we prefer sing our snapshot tests (see above for str![]). This makes it easy to update the output on changes
| // Validate if the path has a .rs extension | ||
| if let Some(extension) = path.extension() { | ||
| if extension != "rs" { | ||
| anyhow::bail!( | ||
| "{} `{}` has path `{}` which does not have a `.rs` extension", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } | ||
| } else { | ||
| anyhow::bail!( | ||
| "{} `{}` has path `{}` which has no file extension (expected `.rs`)", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
What happens today when you have an extension-less file or a non-.rs extension? If it works, then we need to have the conversation about what the intended behavior should be, whether that is a bug or not (and should likely be moved out of this PR to not block on that). If not, we should have tests to compare with to see how the error changed.
There was a problem hiding this comment.
today it works if the example and its path is explicitly mentioned in Cargo.toml. It however does not work if there is an extensionless file, say fuzz in the examples directory and i try cargo check --example fuzz which on hindsight makes sense.
| if !path.exists() { | ||
| anyhow::bail!( | ||
| "can't find {} `{}` at path `{}`", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should have a test for this case as well. This might also represent a breaking change.
| let target_name = name_or_panic(&toml); | ||
|
|
||
| validate_target_path_as_source_file(&path, target_name, TARGET_KIND_HUMAN_EXAMPLE)?; |
There was a problem hiding this comment.
As I mentioned on #10173, we shouldn't limit ourselves to examples
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
What if we talked the directory to report concrete lib.rs and main.rss they could point to?
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
A message like this should start with help: and start with a lowercase letter per rustc's style guide
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
While Consider isn't a question, I feel like it falls into a similar category as questions in the rustc style guide
|
Thanks for your work on this! |
|
Closing this for now. I'll open a new one later with a more comprehensive rework for all target types. Thanks for being patient with me! |
2 participants
What does this PR try to resolve?
Addresses #10173
When directory paths are specified for example targets in Cargo.toml, cargo emits unhelpful os errors:
This PR fixes this by adding a validation function that checks if the target path is a valid source file. If not, emits an instructive error message: