Repository navigation
Conversation
checkpatch has a --subjective (a.k.a --strict) option that enables a few additional "checks". These additional "--strict checks" are desired but - as expected from their name - their output is likely to be dismissed more frequently than other checkpatch issues. For that reason, a relatively complex post-processing Jenkins change was made on the 22nd November 2019. After that change, --strict checks were still run by Jenkins however --strict-only failures were "downgraded" to a green success. For a good example look at sof-ci/jenkins/pr-checkpatch and continuous-integration/travis-ci/pr details in PR thesofproject#1967. This small Travis PR is meant to be merged immediately after the REMOVAL of that complex checkpatch post-processing in Jenkins, swapping who dismisses strict-only failures and who doesn't. This swap is not just about replacing about 50 lines of internal Jenkins code with a couple of lines of visible code in travis and checkpatch: after this swap, a green continuous-integration/travis-ci/pr success and a red sof-ci/jenkins/pr-checkpatch failure can have only one meaning: a strict-only failure. Before this swap, no combination on the github page ever identifies a strict-only failure. Of course, it would be easier to have a real "yellow" color instead of running checkpatch twice and creatively mixing one green and one red. Unfortunately, neither Travis nor github seem to support anytihng besides red and green: travis-ci/travis-ci#4730 https://developer.github.com/v3/repos/statuses/ https://github-api.kohsuke.org/apidocs/org/kohsuke/ github/GHCommitState.html Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Just a test. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
The https://travis-ci.org/github/thesofproject/linux/jobs/671951585 checkpatch is green despite the failed --strict CHK and the log there also shows this PR works as intended. I'll keep this as a draft as long as the november 2019 post processing in Jenkins hasn't been removed. The topic/sof-dev...marc-hb:checkpatch-strict-not-fatal commit should probably be moved to a branch of |
| - if: type = pull_request | ||
| name: checkpatch | ||
| script: # FIXME: replace with TRAVIS_COMMIT_RANGE or similar | ||
| script: # FIXME: replace with TRAVIS_COMMIT_RANGE or similiar |
There was a problem hiding this comment.
@marc-hb I I don't have comment on actual functionality, but I object to this spelling change :) My brain just emits a lot of warnings for "similiar" and Collins agrees:
https://www.collinsdictionary.com/dictionary/english/similar
There was a problem hiding this comment.
+1 on that, I'm not aware of any such word and "similar" is the correct form.
There was a problem hiding this comment.
WARNING the second commit is a checkpatch test NOT meant to be merged.
:-)
you created a new signal combination to make the strict-only failure stands out, but I'm not sure if this learning curve is OK with developers. Previously, Travis CI and SOF CI operate independently. |
Good point, thanks for the feedback. I think this combination is more intuitive even when you have no idea it exists because a strict-only checkpatch failure will appear on the github PR front page with a red "checkpatch" instead of a green checkpatch and red Travis. No need to drill down in Travis. It will also be much easier to learn about this new combination in case someone wants to learn because the entire, two lines of logic of it will be visible in a couple of logs line (see example in https://travis-ci.org/github/thesofproject/linux/jobs/671951585) as opposed to Jenkins configuration most people can't find.
This combination already exists, for instance you can look already look at past PR #1967 and from its colors tell it's a strict-only failure. I'm only swapping it to make it more obvious on the github front page so you don't have to open Travis results to understand what happened. |
| - if: type = pull_request | ||
| name: checkpatch | ||
| script: | ||
| script: # FIXME: replace with TRAVIS_COMMIT_RANGE or similar |
There was a problem hiding this comment.
?? why is this relevant for this specific PR - or what did you mean to day? Looks like a different issue,
There was a problem hiding this comment.
It is a different issue but it is (or at least: was) a bit related because you don't want to checkpatch the cherry-pick. I'll remove it.
| script: # FIXME: replace with TRAVIS_COMMIT_RANGE or similar | ||
| - SHA_PR=`git log --oneline -1 | sed -rn "s/.*Merge (.*) into.*/\1/p"` | ||
| - SHA_MAIN=`git log --oneline -1 | sed -rn "s/.*Merge .* into (.*)/\1/p"` | ||
| - git fetch https://github.com/marc-hb/linux checkpatch-strict-not-fatal && git cherry-pick FETCH_HEAD |
There was a problem hiding this comment.
why not add an SOF-specific script in the SOF tree, and avoid dependencies something we don't see and can't make PRs on.
There was a problem hiding this comment.
As I mentioned in the first github comment, this location wasn't meant to be final.
I can add this two lines long checkpatch commit as a .patch in the sof-dev branch and then git apply it from this same .travis.yml location.
Earlier I suggested a dedicated, thesofproject/linux/ branch different from sof-dev to store these two lines because https://thesofproject.github.io/latest/contribute/linux/development_tree.html#upstream-merge-rebase says:
After the two separate operations of merge and rebase on the two branches, these two branches (sof-dev and sof-dev-rebase) should again be identical.
I don't think you want me to edit scripts/checkpatch.pl in sof-dev directly because
- "these two branches (sof-dev and sof-dev-rebase) should again be identical."
- More importantly, this wouldn't be swapping Travis and Jenkins anymore, this would make strict-only failures green everywhere.
Let me know your preference.
There was a problem hiding this comment.
What I meant was ADDING a new scripts/sof-checkpatch-strict.pl, and we update this file on each kernel revision to align with checkpatch.pl/ Then you can change line 22 of the .yaml file below to use the strict file
There was a problem hiding this comment.
OK, no problem I can add scripts/sof-checkpatch-strict-not-fatal.pl. After reading https://thesofproject.github.io/latest/contribute/linux/development_tree.html#upstream-merge-rebase that seemed like more maintenance than applying a 2-lines long patch on the fly but you are maintaining these branches and I'm not :-)
There was a problem hiding this comment.
yes, but that process was for developers. This is an infrastructure patch so we can be flexible. Same as for the .yaml thing
There was a problem hiding this comment.
This is an infrastructure patch so we can be flexible.
If a somewhat "impure" sof-dev branch is acceptable, then I have a better checkpatch option than duplicating and diverging the entire script. See topic/sof-dev...marc-hb:checkpatch-no-strict-fails
Demo:
./scripts/checkpatch.pl -g b943f7980113 # $? = 0
./scripts/checkpatch.pl -g b943f7980113 --strict # $? = 1
./scripts/checkpatch.pl -g b943f7980113 --strict --no-strict-fails # $? = 0
It is a bit longer than the 2 lines and just 1 hunk of topic/sof-dev...marc-hb:checkpatch-strict-not-fatal so the risk of git conflict is a bit higher but barely: 5 lines and 3 hunks. Smaller than I expected.
There was a problem hiding this comment.
I honestly prefer a different file to avoid merge conflicts which may or may not work and result in CI flagging false positive or not flagging true issues. We'd know it's our own thing.
There was a problem hiding this comment.
@plbossart Hmm, but if we anyways need to take a look at checkpatch changes at every rebase, why not maintain a patch that modifies checkpatch.pl directly in sof-dev? Anyways us maintainers need to take a look at the file at each rebase, so why not let git help us. When we upload a new rebase PR, we'll get results whether we broke the CI checks not (i.e. whether we screwed up the checkpatch rebase or not).
There was a problem hiding this comment.
Of course an even simpler solution that doesn't require any checkpatch change at all is to simply drop --strict in .travis.yml. I mean is there really a need to duplicate the display of --strict failures in Travis (hidden behind a green mark that most people will never open...) once those same --strict failures will be visible again on the PR main page under a red line named "jenkins/checkpatch"?
PS: pushing code first and then discussing "requirements" later deep down this PR is not ideal but was intentional. I could have opened a github issue to discuss requirements first but: it's generally hard to get code reviews, it's harder to get CI code reviews and IMHO it's nearly impossible to get reviews of micro CI requirements like this one. Why I opted for the "send patches" + demo approach.
There was a problem hiding this comment.
we should really revisit the whole thing. IIRC Travis has different - stricter - compilation options than Jenkins.
checkpatch --strict --no-strict-fails prints "subjective" warnings without them causing to exit with a non-zero status. Without --strict, --[no]-strict-fails has no effect. See thesofproject#1988 for the long story. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
| @@ -15,7 +15,7 @@ jobs: | |||
| include: | |||
| - if: type = pull_request | |||
There was a problem hiding this comment.
Use DO NOT MERGE in the subject.
e.g DO NOT MERGE: Fkae checkpatch ...
|
I don't recall if there was any agreement so let's close. |
Let's try to run checkpatch in an open-source and fast way while leveraging Github's very good user interface. Run checkpatch both with and without --strict; this has been a source of confusion in the past, see thesofproject/linux#1988 Note this attempt does not rely on git merge-bases which has been found to be not compatible with shallow cloning: thesofproject/linux#2556 Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Let's try to run checkpatch in an open-source and fast way while leveraging Github's very good user interface. Run checkpatch both with and without --strict; this has been a source of confusion in the past, see thesofproject/linux#1988 Note this attempt does not rely on git merge-bases which has been found to be not compatible with shallow cloning: thesofproject/linux#2556 Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Let's try to run checkpatch in an open-source and fast way while leveraging Github's very good user interface. Run checkpatch both with and without --strict; this has been a source of confusion in the past, see thesofproject/linux#1988 Note this attempt does not rely on git merge-bases which has been found to be not compatible with shallow cloning: thesofproject/linux#2556 Signed-off-by: Marc Herbert <marc.herbert@intel.com>
WARNING the second commit is a checkpatch test NOT meant to be merged.
checkpatch has a --subjective (a.k.a --strict) option that enables a
few additional "checks". These additional "--strict checks" are desired
but - as expected from their name - their output is likely to be
dismissed more frequently than other checkpatch issues. For that reason,
a relatively complex post-processing Jenkins change was made on the 22nd
November 2019. After that change, --strict checks were still run by
Jenkins however --strict-only failures were "downgraded" to a green
success. For a good example look at sof-ci/jenkins/pr-checkpatch and
continuous-integration/travis-ci/pr details in PR #1967.
This small Travis PR is meant to be merged immediately after the REMOVAL
of that complex checkpatch post-processing in Jenkins, swapping who
dismisses strict-only failures and who doesn't.
This swap is not just about replacing about 50 lines of internal Jenkins
code with a couple of lines of visible code in travis and checkpatch:
after this swap, a green continuous-integration/travis-ci/pr success and
a red sof-ci/jenkins/pr-checkpatch failure can have only one meaning: a
strict-only failure. Before this swap, no combination on the github page
ever identifies a strict-only failure.
Of course, it would be easier to have a real "yellow" color instead of
running checkpatch twice and creatively mixing one green and one
red. Unfortunately, neither Travis nor github seem to support anytihng
besides red and green:
travis-ci/travis-ci#4730
https://developer.github.com/v3/repos/statuses/
https://github-api.kohsuke.org/apidocs/org/kohsuke/github/GHCommitState.html