Skip to content

ci: upload patches #934

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

Merged
merged 1 commit into from
Jan 12, 2024
Merged

ci: upload patches #934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions .github/workflows/regression-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ jobs:
run: |
cd regression-tests
bash run-tests.sh -c ${{ matrix.compiler }}
continue-on-error: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Prevents a workflow run from failing when a job fails. Set to true to allow a workflow run to pass when this job fails.

This will make the run pass. I think you need to make this upload a new job and have it run always. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds

Copy link
Contributor

Choose a reason for hiding this comment

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

Another issue unrelated to this PR, the job is called regression-tests-linux-mac but also runs Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I must have forgotten to update.
You can fix it in is this PR.

Copy link
Contributor Author

@JohelEGP JohelEGP Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we really use jobs.<job_id>.needs?
We need to access the output patch file of the job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess I don't know. I just know that the continue-on-error means ignore failures, so that won't do what we want.

Copy link
Contributor

@gregmarr gregmarr Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe the diff needs to be a job output, not sure how that works for matrix jobs. (Update: matrix info is in that section.)
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idoutputs

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a simpler solution. I am testing it now.

Copy link
Contributor

@gregmarr gregmarr Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is one solution in #935 . It would be good to compare solutions, I think.


- name: Run regression tests - Windows version
if: matrix.os == 'windows-latest'
Expand All @@ -41,3 +42,10 @@ jobs:
cd regression-tests && ^
bash run-tests.sh -c ${{ matrix.compiler }}
shell: cmd
continue-on-error: true

- name: Upload patch
uses: actions/upload-artifact@v4
with:
name: ${{ matrix.compiler }}-patch.diff
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

Suggested change
name: ${{ matrix.compiler }}-patch.diff
name: ${{ matrix.os }}-${{ matrix.compiler }}-patch.diff

I think we might have the same compiler run on different OSes (I have that in cooking). Comparing patches for the same compiler between different systems could be another check, also of the CI. Thoughts?

BTW do you what is the retention time of artefacts on GitHub is? I guess these will be used instantly or not at all so it probably isn't an issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://github.com/actions/upload-artifact#retention-period:

Artifacts are retained for 90 days by default.

path: regression-tests/${{ matrix.compiler }}-patch.diff
4 changes: 2 additions & 2 deletions regression-tests/run-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ check_file () {
# Add the file to the index to be able to diff it...
git add "$file"
# ... print the diff ...
git --no-pager diff HEAD -- "$file"
git --no-pager diff HEAD -- "$file" | tee -a "$cxx_compiler-patch.diff"
# ... and remove the file from the diff
git rm --cached -- "$file" > /dev/null 2>&1

Expand Down Expand Up @@ -195,7 +195,7 @@ for test_file in $tests; do
# The source is temporarily copied to avoid issues with bash paths in cl.exe
(cd $exec_out_dir; \
cp ../../$expected_src $generated_cpp_name;
$compiler_cmd"$test_bin" \
$compiler_cmd "$test_bin" \
$generated_cpp_name \
> $generated_cpp_name.output 2>&1)
compilation_result=$?
Expand Down