Export diff check results to a zip archive #5827
Conversation
|
Happy to break this up into smaller PRs if you think there's too much going on here |
|
I ran the updated diff-check job on my fork and if you'd like you can check out the results here. If you'd like, you can download and inspect the |
Not yet had a chance to really go through, but just wanted to check to confirm that the changes here are strictly additive and won't impact our ability to review the results without downloading and extracting an archive file? |
ci/check_diff.sh
Outdated
| echo "$diff" | ||
| echo "Copying diffs between rustfmt and feature branch to $OUTPUT_DIR/$1/diff.txt" | ||
| echo "$diff" >> $OUTPUT_DIR/$1/diff.txt |
There was a problem hiding this comment.
Not yet had a chance to really go through, but just wanted to check to confirm that the changes here are strictly additive and won't impact our ability to review the results without downloading and extracting an archive file?
@calebcartwright this change would result in the script no longer echoing the exact diff out. If you'd like I can add that back so you can still inspect the diff in the console. All other changes are additive.
The diff-check will continue to pass / fail as it did before.
There was a problem hiding this comment.
Thanks for the additional info. I want to make sure we don't make it more difficult (see: add too many extra steps) to use this in a standard PR context, so let's make sure we balance them
Now we have an archive that we can inspect after running the diff-check job. I believe this will be easier to inspect than looking at diff output in the github actions console.
This cleans up the diff check job output and makes it less noisy
These changes mostly improve logging out the cargo version and version of the two rustfmt binaries that are compiled. Some other minor logging changes were made as well to add some whitespace to improve visual clarity when looking at the logs in the GitHub Actions console.
The README provides details about how to interpret the diff-check output and some tips on how to start inspecting the output.
ytmimi
force-pushed
the
diff-check-export
branch
from
384851e
to
c940aef
Compare
| echo "Copying diffs between rustfmt and feature branch to $OUTPUT_DIR/$1/diff.txt" | ||
| echo "$diff" >> $OUTPUT_DIR/$1/diff.txt | ||
| echo "$diff" |
There was a problem hiding this comment.
@calebcartwright I've updated the code to continue outputting the diff in the logs so it'll be easier to check when we incorporate this in CI.
| @@ -71,6 71,7 @@ function create_diff() { | |||
| # $RUSFMT_BIN: Path to the rustfmt master binary. Created when running `compile_rustfmt` | |||
| # $FEATURE_BIN: Path to the rustfmt feature binary. Created when running `compile_rustfmt` | |||
| # $OPTIONAL_RUSTFMT_CONFIGS: Optional configs passed to the script from $4 | |||
| # $OUTPUT_DIR: Path to an output directory for storing the diff files. Set in `main` | |||
| function check_diff() { | |||
| echo "running rustfmt (master) on $1" | |||
There was a problem hiding this comment.
Just a point of personal preference in shell scripts, but in cases where the function gets long and has repeated usage of args, I like creating function local variables and assigning the value to the corresponding positional parameter.
Otherwise one can get a few (or even dozens) of lines into a function looking positional parameter references and trying to remember which is which, or bouncing back up to the signature/docs
| mkdir $OUTPUT_DIR/$1 | ||
| echo "Copying diffs to $OUTPUT_DIR/$1" | ||
| cp rustfmt_diff.txt $OUTPUT_DIR/$1/rustfmt_diff.txt | ||
| cp feature_diff.txt $OUTPUT_DIR/$1/feature_diff.txt |
There was a problem hiding this comment.
And just another nit, but we could similarly create a variable for the target output directory and reuse it instead of having multiple directory concat instances
|
|
||
| ## Summary | ||
|
|
||
| The Diff Check Job is used to validate rustfmts backwards compatability guarantees |
There was a problem hiding this comment.
| The Diff Check Job is used to validate rustfmts backwards compatability guarantees | |
| The Diff Check Job is used to validate rustfmt's backwards compatability guarantees |
| Some of the real world projects that rustfmt is tested against may not use rustfmt at all. | ||
| All the $diff_files files show is that using rustfmt on a given project would change some formatting. | ||
|
|
||
| If a $diff_file file is present for a given project then that indicates a failure to |
There was a problem hiding this comment.
Perhaps worth mention the version=Two / style_edition=vNext* is a scenario where formatting changes might actually be expected?
Or are we only running diffs against repositories that we know aren't using a vNext Style Edition?
| # Zip up all the diff changes detected by the script | ||
| # | ||
| # Globlas: | ||
| # $OUTPUT_DIR: Output directory where all `*diif.txt` files are written to. Set in `main`. |
There was a problem hiding this comment.
| # $OUTPUT_DIR: Output directory where all `*diif.txt` files are written to. Set in `main`. | |
| # $OUTPUT_DIR: Output directory where all `*diff.txt` files are written to. Set in `main`. |
|
|
||
| # Zip up all the diff changes detected by the script | ||
| # | ||
| # Globlas: |
There was a problem hiding this comment.
Typo here, though for some reason GitHub isn't giving me ability to suggest line-level changes at the moment
| # Globlas: | |
| # Globals: |
This PR enables the
diff-checkGitHub Actions workflow to export a zip archive containing the results of running the diff-check job. This archive includes all diffs produced when runningrustfmt --checkand additionally includes aREADME.md. TheREADME.mdprovides further information about the diff check job that was run and how to interpret and inspect thediff-checkresultsSome changes were made to the logging output of the diff-check job. For example, we no longer
catout the diffs produced when runningrustfmt --checksince we're now exporting those in the archive.r? @calebcartwright
I think this PR would be best reviewed commit by commit.