Skip to content

Export diff check results to a zip archive #5827

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Export diff check results to a zip archive #5827

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71c3f68
write rustfmt diff check output to diff-check.zip
ytmimi Jul 11, 2023
a1ec8b7
Build the rustfmt binary and feature binary in quiet mode
ytmimi Jul 12, 2023
78545bc
update diff-check logging
ytmimi Jul 12, 2023
c940aef
Add a README.md to the diff-check.zip archive
ytmimi Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/check_diff.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,9 @@ jobs:

- name: check diff
run: bash ${GITHUB_WORKSPACE}/ci/check_diff.sh ${{ github.event.inputs.clone_url }} ${{ github.event.inputs.branch_name }} ${{ github.event.inputs.commit_hash || github.event.inputs.branch_name }} ${{ github.event.inputs.rustfmt_configs }}

- name: Archive Diff Check Report
uses: actions/upload-artifact@v3
with:
name: diff-check-report
path: diff-check.zip
30 changes: 29 additions & 1 deletion ci/check_diff.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

create_diff $RUSFMT_BIN rustfmt_diff.txt
Expand All @@ -89,10 +90,18 @@ function check_diff() {
--unified=0 --no-index rustfmt_diff.txt feature_diff.txt 2>&1 | tail -n +6 | cut -c 2-
)

# COPY diffs into output dir
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
Comment on lines +94 to +97
Copy link
Member

Choose a reason for hiding this comment

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

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


if [ -z "$diff" ]; then
echo "no diff detected between rustfmt and the feture branch"
return 0
else
echo "Copying diffs between rustfmt and feature branch to $OUTPUT_DIR/$1/diff.txt"
echo "$diff" >> $OUTPUT_DIR/$1/diff.txt
echo "$diff"
Comment on lines +103 to 105
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

return 1
fi
Expand Down Expand Up @@ -160,11 +169,27 @@ function check_repo() {
cd $WORKDIR
}

# Zip up all the diff changes detected by the script
#
# Globlas:
Copy link
Member

Choose a reason for hiding this comment

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

Typo here, though for some reason GitHub isn't giving me ability to suggest line-level changes at the moment

Suggested change
# Globlas:
# Globals:

# $OUTPUT_DIR: Output directory where all `*diif.txt` files are written to. Set in `main`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# $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`.

# $CURRENT_DIR: The directory where the script was run from. Set in `main`.
function zip_up_diffs() {
cd $OUTPUT_DIR

# Just to clean things up we'll make sure to remove empty files and directories
find . -type f -empty -delete
find . -type d -empty -delete
zip -q -r $CURRENT_DIR/diff-check .
}

function main() {
CURRENT_DIR=$(pwd)
tmp_dir=$(mktemp -d -t rustfmt-XXXXXXXX)
echo Created tmp_dir $tmp_dir

compile_rustfmt $tmp_dir
OUTPUT_DIR=$(mktemp -d -t diff-output-XXX)

# run checks
check_repo "https://github.com/rust-lang/rust.git" rust-lang-rust
Expand All @@ -191,9 +216,12 @@ function main() {
check_repo "https://github.com/actix/actix.git" actix
check_repo "https://github.com/denoland/deno.git" denoland_deno

zip_up_diffs

# cleanup temp dir
echo removing tmp_dir $tmp_dir
echo removing tmp_dir $tmp_dir and $OUTPUT_DIR
rm -rf $tmp_dir
rm -rf $OUTPUT_DIR

# figure out the exit code
for status in ${STATUSES[@]}
Expand Down