cargo
cargo copied to clipboard
rust-lang
Allow build scripts to report error messages through `cargo::error`
Related to #10159.
Adds the cargo::error build script directive. It is similar to cargo::warning, but it emits an error message and it also fails the build.
r? @ehuss
rustbot has assigned @ehuss. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
@rfcbot fcp merge
@torhovland is proposing we insta-stabilize cargo::error= build directive which was requested in #10159.
This is following the semantics proposed in a cargo team meeting (https://github.com/rust-lang/cargo/issues/10159#issuecomment-988104375): cargo::error= forces the build script to fail, independent of whether its local or not
@kornelski counters that this means we have two ways to cause a build script to error when we should only have one. They suggest warning if an error appears without a failure (https://github.com/rust-lang/cargo/issues/10159#issuecomment-2305139091). However, that does mean that we have error messages that might not be tied to a failure which can cause user confusion and I think we should keep the Cargo team's previous proposal on this.
This is only supported with cargo:: syntax and not cargo: syntax. Use of cargo:error would create an error metadata entry. As cargo:: is strictly checked, this will require an MSRV bump to use which is good in this case if people are relying on cargo::error causing a bad exit code. Because cargo::error will fail on previous versions of Cargo, no transition period should be needed for this.
As for insta-stabiliaze, I can understand that it seems like a lot to create an unstable feature just for this build directive. We don't have anything like -Zunstable-options for build directives. This is relatively small in scope and low level / mechanism focused, that besides @kornelski's counter proposal, I'm not seeing much we can learn from an extended testing period. I also don't expect much nightly testing on this.
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [ ] @Muscraft
- [x] @arlosi
- [x] @ehuss
- [x] @epage
- [x] @joshtriplett
- [ ] @weihanglo
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
The primary motivation for this feature was #10159, where the main problem was noisy output from pkg_config. However, I don't see how to integrate cargo::error with the pkg_config crate ;(
By default pkg_config prints cargo metadata as a side effect. But this metadata must not contain cargo::error, because the API returns a Result and many crates try a fallback after that. pkg_config can't stuff cargo::error prefix in Display of pkg_config::Error either, because unwrap() prints it to stderr, not stdout (where it's supposed to be), plus some crates have printf("cargo:warning={pkg_config_error}"); and that would nest syntax, or cargo::error after a newline would fail the build instead of warning.
I would assume that the caller of pkg-config would be responsible for printing cargo::error with the appropriate error if they want to. Can you explain why that might not work?
@ehuss The downside is that instead of improving one crate to benefit the whole ecosystem, it'll be necessary to change 750 downstream crates.
pkg_config instructs users to .unwrap() it: https://docs.rs/pkg-config/latest/pkg_config/#example
system-deps too.
which means these crates can't adopt cargo:error on their own, in their current API, to automatically provide a good baseline to all current users who unwrap() their errors.
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
:pushpin: Commit f94b4427ad3223787abd523339a911576b867148 has been approved by epage
It is now in the queue for this repository.
:hourglass: Testing commit f94b4427ad3223787abd523339a911576b867148 with merge 3395029873112bda5e95ee1a6b5b5d57a0e18c8b...
@torhovland looks like some other changes cause the output in the new tests to change and the commits need to be updated.
:umbrella: The latest upstream changes (presumably #14743) made this pull request unmergeable. Please resolve the merge conflicts.