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

Refactor CI tool and drop xshell dependency. #13279

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Brezak
Copy link
Contributor

@Brezak Brezak commented May 7, 2024

This is part 2 of my attempt at improving the errors CI spits out. See part 1 (#13187) for the action plan.

Problem

The current structure of the CI tool means we can't really execute anything other than shell scripts.

Solution

  • Remove the PreparedCommand struct and Prepare trait.
  • Run sub commands directly.
  • Introduce helpers for invoking cargo.

Testing

  1. Run cargo run -p ci -- compile-check.
  2. Check if everything is outputting correctly.
  3. The CI uses this code extensively. Double check it.

Probably should have waited for #13187 to merge. But oh well. Might as well get some feedback on my approach here.
Most of the command code isn't very human friendly. It was written to be copy-pastable as writing all that boiler plate by hand
would have turned my brain to mush.

cc @BD103

@Brezak
Copy link
Contributor Author

Brezak commented May 7, 2024

I'm sorry if the description isn't very coherent. It's very late here. I'm going to sleep. I'll fix everything tomorrow.

@BD103 BD103 self-requested a review May 7, 2024 23:10
@BD103 BD103 added C-Feature A new feature, making something new possible A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change X-Uncontroversial This work is generally agreed upon labels May 7, 2024
@BD103
Copy link
Member

BD103 commented May 8, 2024

I created Brezak#2 with just a few small refactors. I think it's possible to still use a trait instead of having inconsistent Command::run function signatures, but I'll make a demo for that tomorrow.

tools/ci/src/commands/mod.rs Outdated Show resolved Hide resolved
@Brezak
Copy link
Contributor Author

Brezak commented May 9, 2024

This is work for a future PR but several commands could be flags on other commands. For example both the test-check and cfg-check commands could be flags on the compile-check command.

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.

It needs work, but I think that's for future PRs. Looks like a good first start!

@BD103 BD103 mentioned this pull request May 10, 2024
@Brezak Brezak force-pushed the nicer-ci-errors-part-2 branch 3 times, most recently from c24ce17 to e312441 Compare December 24, 2024 21:23
@Brezak Brezak force-pushed the nicer-ci-errors-part-2 branch from e312441 to 17c667e Compare December 24, 2024 21:43
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-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants