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

Allow build scripts to report error messages through cargo::error #14435

Closed
wants to merge 5 commits into from

Conversation

torhovland
Copy link
Contributor

@torhovland torhovland commented Aug 21, 2024

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.

@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2024

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2024
@torhovland torhovland marked this pull request as draft August 21, 2024 12:13
@torhovland torhovland marked this pull request as ready for review August 21, 2024 12:18
@torhovland torhovland marked this pull request as draft August 22, 2024 10:15
@torhovland torhovland force-pushed the build-script-errors branch from 8c0028a to da2e8eb Compare August 22, 2024 11:27
@torhovland torhovland marked this pull request as ready for review August 22, 2024 11:27
@torhovland torhovland force-pushed the build-script-errors branch from da2e8eb to 02eb2a8 Compare August 22, 2024 11:44
@torhovland
Copy link
Contributor Author

@epage This is ready for another look.

@torhovland torhovland force-pushed the build-script-errors branch from 02eb2a8 to ee7edd2 Compare August 23, 2024 06:42
@torhovland
Copy link
Contributor Author

@epage This is ready for another look.

@@ -319,6 +319,8 @@ to set the shared library version or the runtime-path.
The `error` instruction tells Cargo to display an error after the build script
has finished running, and then fail the build.

> **MSRV:** Respected as of 1.82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping a comment open on this so we remember to update it if this slips

@epage epage added the T-cargo Team: Cargo label Aug 23, 2024
@epage
Copy link
Contributor

epage commented Aug 23, 2024

@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 (#10159 (comment)): 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 (#10159 (comment)). 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.

torhovland and others added 2 commits August 26, 2024 09:12
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
@torhovland torhovland force-pushed the build-script-errors branch from 2b1c7a9 to f94b442 Compare August 26, 2024 07:13
@epage epage changed the title Build script errors Allow build scripts to report error messages through cargo::error Aug 26, 2024
@kornelski
Copy link
Contributor

kornelski commented Aug 28, 2024

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.

@ehuss
Copy link
Contributor

ehuss commented Aug 28, 2024

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?

@kornelski
Copy link
Contributor

@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.

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Sep 17, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 17, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Sep 27, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 27, 2024

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.

@epage
Copy link
Contributor

epage commented Sep 27, 2024

Thanks for the work on this @torhovland !

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2024

📌 Commit f94b442 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2024
bors added a commit that referenced this pull request Sep 27, 2024
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.
@bors
Copy link
Contributor

bors commented Sep 27, 2024

⌛ Testing commit f94b442 with merge 3395029...

@bors
Copy link
Contributor

bors commented Sep 27, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 27, 2024
@epage
Copy link
Contributor

epage commented Sep 27, 2024

@torhovland looks like some other changes cause the output in the new tests to change and the commits need to be updated.

bors added a commit that referenced this pull request Oct 29, 2024
Allow build scripts to report error messages through `cargo::error`

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.

This is a repost of #14435 with the tests updated, a note added to the documentation about using this in a library, and updating the MSRV.

Closes #10159
@bors
Copy link
Contributor

bors commented Oct 29, 2024

☔ The latest upstream changes (presumably #14743) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Oct 29, 2024

Closing in favor of #14743.

@ehuss ehuss closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-documenting-cargo-itself Area: Cargo's documentation disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants