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

reporting diagnostics from build scripts #12394

Open
OmarTawfik opened this issue Jul 24, 2023 · 3 comments
Open

reporting diagnostics from build scripts #12394

OmarTawfik opened this issue Jul 24, 2023 · 3 comments
Labels
A-build-scripts Area: build.rs scripts C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@OmarTawfik
Copy link

OmarTawfik commented Jul 24, 2023

Problem

The Rust ecosystem is developing nicely, with more and more projects making use of Cargo build scripts and Cargo workspace binaries to automate a lot of their workflows. The editor integration with Rust Analyzer makes that a breeze to work with, where disk contents and in-editor diagnostics/intellisense are updated with every file save. It all works perfectly, until it doesn't!

When build scripts fail, they can only panic, which sends the user hunting for the root cause through backtraces on the terminal, or in rust-analyzer output channels. Instead of leveraging these diagnostics to guide the user and improve his workflow, they are painful to work with.

I believe it is one of the biggest pain points of working with Cargo build scripts. It wildly contrasts with the top-notch diagnostics experience provided by rustc, as scripts have no way of informing the user of what went wrong, where it went wrong, or how to fix it!

Proposed Solution

Build scripts are already able to report diagnostics, by reusing the exact same data format that rustc is using. They can just use serde_json to serialize instances of cargo_metadata::Message, which is already understood by both Cargo (to render in the terminal), and Rust Analyzer (to render in the editor).

What is remaining is to make sure that when Cargo invokes the build script, it parses any JSON messages in the build script output, and print it along the ones from rustc. I already enabled the necessary API in oli-obk/cargo_metadata#220

This empowers even the simplest scripts to provide the same incredible diagnostics experience that rustc provides. It has great potential for build script authors to preprocess any kind of input, and iteratively work with the user to validate/fix it in their editor, instead of combing through panics and stack traces in the terminal/output window when a build script fails.

Not sure if that would be a breaking change, but if needed, we can also avoid that by adding a new cargo:json-message={} instruction to preserve any legacy/corner case behaviours.

Notes

Unfortunately, text directives like cargo:warning are not enough (even if we add cargo:error), as they don't carry additional metadata like file path or line/column numbers, and they cannot be parsed by other clients like Cargo or Rust Analyzer.

Open Questions

  1. When invoked in the terminal, can Cargo render a nice error report for these messages (including the source file preview), as if it was produced from rustc itself?
  2. Can it allow file locations other than *.rs for these diagnostics? in case the build file is using a custom file format as input. This is important for build scripts that read metadata from files like Cargo.toml, or JSON/YAML configuration files for external tools.

If someone can provide tips or code pointers, it would be absolutely fantastic. I'm not sure how involving the change is. Thanks for considering!

@OmarTawfik OmarTawfik 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 24, 2023
@epage
Copy link
Contributor

epage commented Jul 24, 2023

imo cargo::warning and cargo::error are the first steps for any kind of diagnostics.

Challenges we have beyond that

  • We can't interpret stdout lines as rustc-like messages as that would be a breaking change
  • This means we need a new directive which is blocked on Cannot extend build directives within cargo #11461
  • Adding rustc diagnostic message support is putting a large burden on users to get the minimum required content correct (cargo_metadata blindly added a builder without any enforcement of what content is required)
  • Adding rustc diagnostic message support would require cargo to support multiple versions of the messages. Today, we only have to worry about supporting the version that ships with cargo (with some give in that for making development easier)

Personally, I worry about people making complex enough of build scripts that rustc diagnostics are needed. There are pros and cons to build scripts but the cons have led me to only use them when I care about the host environment we are building for.

@epage epage added the A-build-scripts Area: build.rs scripts label Jul 24, 2023
@OmarTawfik
Copy link
Author

Thanks for the tips!

We can't interpret stdout lines as rustc-like messages as that would be a breaking change. This means we need a new directive which is blocked on #11461

Ah! Makes sense then to wait for that first.

Adding rustc diagnostic message support is putting a large burden on users to get the minimum required content correct (cargo_metadata blindly added a builder without any enforcement of what content is required)

Since cargo-metadata is being recommended by the official Cargo reference, I think its maintainers (any contributors) would be happy to keep it up to date and enforce the required fields/data. We can also think about ways to automate that.

Adding rustc diagnostic message support would require cargo to support multiple versions of the messages. Today, we only have to worry about supporting the version that ships with cargo (with some give in that for making development easier)

While I understand that a full formal specification of that data format is not available, it is a fact that, today, Cargo users need to understand and work with that format to process cargo check --message-format json, along with dealing with breaking changes and versioning issues. IIUC, build scripts reusing it won't introduce more issues. Authors can also pin a specific rust-version to avoid unexpected changes.

@epage
Copy link
Contributor

epage commented Jul 26, 2023

Since cargo-metadata is being recommended by the official Cargo reference, I think its maintainers (any contributors) would be happy to keep it up to date and enforce the required fields/data. We can also think about ways to automate that.

I'd recommend checking with them before committing them to that. When you contributed builder support, it was all automatic. This would require it to all be written by hand.

We also already have a problem with rustc, cargo, and cargo-metadata staying in sync when changes are made.

While I understand that a full formal specification of that data format is not available, it is a fact that, today, Cargo users need to understand and work with that format to process cargo check --message-format json, along with dealing with breaking changes and versioning issues. IIUC, build scripts reusing it won't introduce more issues. Authors can also pin a specific rust-version to avoid unexpected changes.

Reading a format and writing a format are very different.

@ehuss ehuss added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Nov 22, 2023
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 C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

3 participants