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

Add json output flags to our CI tool #13187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Brezak
Copy link
Contributor

@Brezak Brezak commented May 2, 2024

This is part 1 of what I'm planing to be a 3 part project to improve how our CI reports errors.

  • Part 1 (this) will introduce a way to output structured error messages which other tools may ingest.
  • Part 2 will be a refactor of how we handle commands so they can do more than invoke the shell.
  • Part 3 will be a command that ingests the structured output from other commands and spits out GitHub workflow commands for our CI.

Objective

Add a way for our CI tool to output messages in a structured form usable by other tools.

Solution

Most everything we currently use in our tools can be configured to output [json](https://doc.rust-lang.org/cargo/reference/external-tools.html#json-messages). So we just need to process it a bit and output effectively that.

The only thing that can't do json are our compile fail tests and formatting. Formatting can't do it because the check flag for rustfmt doesn't support json output. The compile fail tests can't do it because they use a custom test harness. We'd maybe be able to switch them to the standard cargo harness. We would likely loose the ability to show stderr diffs which is less than ideal but they don't work right now so it isn't much of a blocker.

The result is that everything that supports json output gets a emit_json flag and I've refactored how we invoke our commands so we can parse the json output they now output.

Testing

The code was linted through our CI tool using the clippy subcommand using it with and without the emit_json flag,

cc @BD103

@Brezak
Copy link
Contributor Author

Brezak commented May 2, 2024

I'd hold of on merging this until part 2 is ready as this currently removes the colored messages from our CI logs. From personal experience, this makes skimming them for error messages about 3 times as annoying. Part 2 should fix this.

@alice-i-cecile alice-i-cecile requested review from BD103 and mockersf May 2, 2024 20:30
@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from a31623c to b243706 Compare May 2, 2024 20:30
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Build-System Related to build systems or continuous integration S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 2, 2024
@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from b243706 to 348f18a Compare May 2, 2024 23:23
@Brezak
Copy link
Contributor Author

Brezak commented May 2, 2024

Update. We don't loose color. We use a env variable to force cargo to always output color.

tools/ci/out.txt Outdated Show resolved Hide resolved
@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from 348f18a to 1f4a102 Compare May 3, 2024 08:22
@Brezak Brezak requested a review from BD103 May 3, 2024 19:34
@BD103 BD103 removed the S-Blocked This cannot move forward until something else changes label May 5, 2024
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

This works, but it gives me the feeling that the current structure we are using is flawed for this kind of logic.

I'm going to approve this with a few nits on comments, but I look forward to further refactors. (Also yay better error handling!)

tools/ci/src/ci.rs Show resolved Hide resolved
tools/ci/src/ci.rs Show resolved Hide resolved
tools/ci/src/json.rs Show resolved Hide resolved
tools/ci/src/json.rs Show resolved Hide resolved
tools/ci/src/json.rs Show resolved Hide resolved
tools/ci/src/prepare.rs Show resolved Hide resolved
@BD103
Copy link
Member

BD103 commented May 5, 2024

Oh and also make sure to mark the PR as ready for review, since you cannot merge a draft. :)

@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from 1f4a102 to 6a12558 Compare May 5, 2024 15:10
@Brezak Brezak marked this pull request as ready for review May 5, 2024 15:32
@BD103 BD103 mentioned this pull request May 10, 2024
@janhohenheim janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 17, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from 6a12558 to 71c886b Compare December 24, 2024 13:52
@Brezak Brezak force-pushed the nicer-ci-errors-part-1 branch from 71c886b to 0193ed0 Compare December 24, 2024 17:50
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants