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

Warning for misspelling of cargo directives in build scripts #12353

Open
HKalbasi opened this issue Jul 12, 2023 · 7 comments
Open

Warning for misspelling of cargo directives in build scripts #12353

HKalbasi opened this issue Jul 12, 2023 · 7 comments
Labels
A-build-scripts Area: build.rs scripts A-diagnostics Area: Error and warning messages generated by Cargo itself. 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-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@HKalbasi
Copy link
Member

Problem

Misspelling of cargo directives in build scripts is a huge footgun. For example:

println!("cargo:rerun-if-changed=somefile.cpp");
println!("cargo:rerun-if-chagned=src/main.rs");

Here cargo will silently ignore the src/main.rs file (the typo is chagned) and one would wonder why their build script is not working. It happend in real world to me, in my case it was rust-link-lib instead of rustc-link-lib.

Proposed Solution

Cargo should warn against any unknown directive in build scripts. Either it is a misspelling, which a warning is very helpful, or it is a directive that newer cargo versions support, which a warning still has useful information, and if ignoring is the desired behavior, user can allow the warning by #12235

Notes

No response

@HKalbasi HKalbasi 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 Jul 12, 2023
@epage
Copy link
Contributor

epage commented Jul 12, 2023

The challenge with this is that there is a catch-all directive of cargo:KEY=VALUE, so we don't know for certain there is a typo.

Similarly, because of this catch-all, we can't actually add new directives... (#11461).

However, we have found a way forward as noted in that issue. We'll make a low-probability breaking change of disallowing KEY=VALUEs to start with : and then switch the build directive syntax to use cargo:: as a prefix. This will allow unrecognized build directives to be a hard error.

Work on #11461 was started in #12201 but we were concerned about people using the new syntax on old cargo and ... dropping building directives. So we are looking to have a 6 month or so transition period where cargo:: is a hard error and then we will support cargo:: as the new prefix.

@HKalbasi
Copy link
Member Author

That's unfortunate. New prefix seems good. Meanwhile I think a crate that provide functions for each directive probably helps. That crate could also check for other footguns, for example warn/error for rerun-if-chagned with non existent path.

@epage
Copy link
Contributor

epage commented Jul 12, 2023

Yes, there has been some interest in having an official Rust API for build scripts. Part of the problem is our processes don't scale up for more crates which we have been working on.

@Mrt3383x38

This comment was marked as off-topic.

@weihanglo weihanglo added A-build-scripts Area: build.rs scripts A-diagnostics Area: Error and warning messages generated by Cargo itself. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 16, 2023
@weihanglo
Copy link
Member

Label this as S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review , but it is currently blocked on #11461.

#12432 is related somewhat.

@weihanglo weihanglo added E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review labels Aug 16, 2023
@after-ephemera
Copy link

This issue is currently marked as S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review , but it appears that with the ongoing work on #12432 it may end up being irrelevant/part of the larger project? Are we waiting on a finalized API spec before proceeding with detection of unrecognized directives?

@epage
Copy link
Contributor

epage commented Jan 8, 2024

imo this warning would be independent of #12432. The API would help people avoid having typos but not everyone will use it once its available.

Hmm, I'm not too sure what is actually left for this after #11461. That PR makes it so misspellings are hard errors but only if you use the new syntax (cargo::). That just leaves the old syntax (cargo:). We could just do a similarity match and only warn if the similarity match succeeds. It would be unlikely to be a metadata field and it would only be a warning, so no one would be blocked if we were wrong. However, the further away we are from #11461 being released and more people migrate to it, the less value adding a warning would give.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-diagnostics Area: Error and warning messages generated by Cargo itself. 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-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

5 participants