search

zephyrproject-rtos / ci-tools

CI Tools and Scripts (obsolete)
Apache License 2.0
9 stars 18 forks source link

CI Checkpatch warnings should be made obvious in PRs #63

Closed aescolar closed 5 years ago

aescolar commented 5 years ago

Is your enhancement proposal related to a problem? Please describe. Currently checkpatch is run in CI. If an error is reported, the whole checkpatch output is added as a comment to the PR.

But if checkpatch only reports warnings, these are not reported to the PR. This leads to checkpatch warnings being missed during reviews.

Describe the solution you'd like The CI checkpatch check would still add as a comment to the PR the warning output, even though it would not block the PR. This way the author or reviewers are made aware of them and can act.

Additional context For ex. see https://github.com/zephyrproject-rtos/zephyr/pull/16091#issuecomment-491747807

marc-hb commented 5 years ago

Another issue that may or may not be related, I can't tell yet. Please ignore if not related.

check_compliance.py doesn't use checkpatch's --git range argument. When checking a large PR with multiple commits, check_compliance.py squashes the entire PR and gives that to checkpatch instead. This loses checkpatch's ability to inspect commit messages. This loses checkpatch's ability point directly at specific commits. This may also fail to catch issues between commits. Etc.

At first this seems like a one-line fix, so I tried to fix it. Unfortunately this makes checkpatch returns a zero (success) exit status. It's like errors become warnings? Because of the --mailback option? I tried other checkpatch versions from the Linux kernel and the results were inconclusive.

Here are a couple test cases I found in Zephyr's history, useful for any checkpatch work:

scripts/check_compliance.py -m checkpatch -c 1f96f55183~1..57923185472b # 193 commits
scripts/check_compliance.py -m checkpatch -c 1f96f55183..521f4778a149 # 2 commit
marc-hb commented 5 years ago

Cc: @ulfalizer (dunno how to Cc: without "assigning")

aescolar commented 5 years ago

@marc-hb About commit messages, Isn't that checked by gitlint in CI? (the part with checkpatch errors being added and removed in a series of PR commits would be a different thing, but seems relatively minor, or?)

marc-hb commented 5 years ago

About commit messages, Isn't that checked by gitlint in CI?

Yes, gitlint check the commit messages but maybe looks at different things? Or at the exact same things, and then it's a waste of time and code to duplicate what checkpatch could do "for free".

the part with checkpatch errors being added and removed in a series of PR commits would be a different thing, but seems relatively minor, or?

Yes minor as long as people don't care about checkpatch on other branches.

ulfalizer commented 5 years ago

I could look at this next week maybe. Thinking there could be an info() function for adding output from a test without failing it.

Might take some refactoring though. Tests really ought to only generate strings as output, with conversion to whatever format as a separate step afterwards.