Get the Check Diff rust binary working #6751
Open
+477
−110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ytmimi
force-pushed
the
update_rust_check_diff
branch
from
db0480a to
01e646c
Compare
ytmimi
force-pushed
the
update_rust_check_diff
branch
4 times, most recently
from
c3cd596 to
8bf953a
Compare
ytmimi
force-pushed
the
update_rust_check_diff
branch
from
8bf953a to
78fdb19
Compare
This is how cargo determines which environment variable to set when running `cargo run` and helps to allow the binary to run on more operating systems than just linux.
I think this makes the overall code better. Also removes an unecessary lifetime annotation.
in most cases I'd expect to get valid UTF-8, and in the rare event that we don't get valid UTF-8 including some replacement characters should be fine.
The `CheckDiffError` that was returned in the error case was too broad and I felt that the code could be simplified if we returned a type that specifically captured what went wrong.
The `CheckDiffError` that was previously returned in the error case was too broad and I felt that the code could be simplified if we returned a type that specifically captured what went wrong when trying to create a diff between two formatter.
This updates some logging output to be more descriptive and also adds some `debug!` and `error!` logging.
Now after we compile both rustfmt binaries we iterate through each repository and process each check in its own thread. This implementation more or less follows the behavior from `ci/check_diff.sh`, but it's faster since we're able to process each repository in parallel.
When rustfmt knows the file path it's able to skip formatting for files listed in the repo's rustfmt.toml `ignore` list. This helps when us process files in r-l/rust that have been explicitly skipped. Otherwise we'll try to format files that cause rustfmt to hang.
`i32` didn't seem like the right return type to me. Since we're just recording that any error happened `u8` seemed good enough for that.
This let's us controll the rust language edition used to parse source code and the rustfmt style edition used for code formatting.
ytmimi
force-pushed
the
update_rust_check_diff
branch
from
78fdb19 to
45967f5
Compare
ytmimi
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Diff Check Job is integral to making sure we don't introduce breaking formatting changes. An effort was started by a contributor a while ago to rewrite the tool in rust, but that stalled out. I'd like to cut over to this rewritten rust version, which I think is an improvement over the current shell script and I think it should should help us get things unstuck and allow us to complete the next subtree sync with confidence that we're not introducing any breaking changes.
The PR is broken up into smaller commits, and I'd recommend reviewing things one commit at a time, but also happy to break this up into smaller commits if that's preferred.
Note: I plan to update the
.github/workflows/check_diff.ymlin a follow PR after this landsr? @calebcartwright @jieyouxu @Manishearth