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

fix(clang-tidy): make clang-tidy workflow fail only when it reports errors #306

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

veqcc
Copy link
Contributor

@veqcc veqcc commented Jun 28, 2024

Description

Related PR:

The current clang-tidy fails when there exists a fixes.yaml.
It leads to the CI failure even when clang-tidy reports only warnings.
I have changed it so that the CI fails only when it reports errors.

@xmfcx
The way I used whether clang-tidy reports an errors is the following:

if [ -n "$(grep ": error:" /tmp/clang-tidy-result/report.txt)" ];

I'm not sure there is a better way, so please review it carefully. Thank you in advance!

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Interface changes

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@veqcc veqcc self-assigned this Jun 28, 2024
@veqcc veqcc requested a review from xmfcx June 28, 2024 02:26
@veqcc
Copy link
Contributor Author

veqcc commented Jun 28, 2024

@xmfcx
Do you have any idea how to test this change before being merged?

@xmfcx
Copy link
Contributor

xmfcx commented Jun 28, 2024

I'll create a branch and point here in universe to test 👍

@xmfcx
Copy link
Contributor

xmfcx commented Jun 28, 2024

Could you make it so that it will still export the fixes with the -export-fixes /tmp/clang-tidy-result/fixes.yaml command and upload these artifacts as usual?

It can still check for the existence of your new report file.

The fixes file can be used to apply fixes and provides a structured output.

@xmfcx
Copy link
Contributor

xmfcx commented Jun 28, 2024

https://github.com/autowarefoundation/autoware.universe/actions/runs/9697046266/job/26761095747?pr=7729#step:9:438

Here you can download the artifact.

Maybe we could use the fixes file and search for Level: Warning or error? to look for errors.

@veqcc
Copy link
Contributor Author

veqcc commented Jun 28, 2024

Maybe we could use the fixes file and search for Level: Warning or error? to look for errors.

I'm not sure so need some investigations how the followings are denoted in yaml file:

  • errors
  • warnings which is specified as warnings-as-errors
  • warnings

@veqcc
Copy link
Contributor Author

veqcc commented Jun 28, 2024

Maybe we could use the fixes file and search for Level: Warning or error? to look for errors.

Quick investigation revealed that the warnings with warnings-as-errors also have Level: Warning in yaml file.
So we cannot use fixes.yaml for deciding exit code 😭

Signed-off-by: veqcc <[email protected]>
@veqcc
Copy link
Contributor Author

veqcc commented Jun 28, 2024

@xmfcx

Could you make it so that it will still export the fixes with the -export-fixes /tmp/clang-tidy-result/fixes.yaml command and upload these artifacts as usual?
It can still check for the existence of your new report file.
The fixes file can be used to apply fixes and provides a structured output.

done in 90a0f2a

@xmfcx xmfcx merged commit a1960b0 into main Jul 1, 2024
13 checks passed
@xmfcx xmfcx deleted the fix/clang-tidy branch July 1, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants