Skip to content

Swap travis and jenkins checkpatch roles #1988

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

Closed
marc-hb wants to merge 2 commits into from
Closed

Swap travis and jenkins checkpatch roles #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73615ba
.travis.yml: swap Travis and Jenkins check patch --strict roles.
marc-hb Apr 7, 2020
4b3606e
Fake check patch CHK to test previous commit
marc-hb Apr 7, 2020
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
3 changes: 2 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ jobs:
include:
- if: type = pull_request

@dbaluta dbaluta Apr 8, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use DO NOT MERGE in the subject.
e.g DO NOT MERGE: Fkae checkpatch ...

name: checkpatch
script:
script: # FIXME: replace with TRAVIS_COMMIT_RANGE or similiar

@kv2019i kv2019i Apr 7, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@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

@paulstelian97 paulstelian97 Apr 7, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 on that, I'm not aware of any such word and "similar" is the correct form.

@marc-hb marc-hb Apr 7, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

WARNING the second commit is a checkpatch test NOT meant to be merged.

:-)

- 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

@plbossart plbossart Apr 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@marc-hb marc-hb Apr 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

  1. "these two branches (sof-dev and sof-dev-rebase) should again be identical."
  2. More importantly, this wouldn't be swapping Travis and Jenkins anymore, this would make strict-only failures green everywhere.

Let me know your preference.

@plbossart plbossart Apr 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@marc-hb marc-hb Apr 7, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 :-)

@plbossart plbossart Apr 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, but that process was for developers. This is an infrastructure patch so we can be flexible. Same as for the .yaml thing

@marc-hb marc-hb Apr 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@plbossart plbossart Apr 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@kv2019i kv2019i Apr 8, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@marc-hb marc-hb Apr 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@plbossart plbossart Apr 8, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should really revisit the whole thing. IIRC Travis has different - stricter - compilation options than Jenkins.

- scripts/checkpatch.pl --strict --codespell -g $SHA_MAIN..$SHA_PR
- name: "Code Scan"
script:
Expand Down
2 changes: 1 addition & 1 deletion scripts/checkpatch.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2904,7 +2904,7 @@ sub process {
my $typo_fix = $spelling_fix{lc($typo)};
$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
my $msg_level = \&WARN;
my $msg_level = \&CHK;
$msg_level = \&CHK if ($file);
if (&{$msg_level}("TYPO_SPELLING",
"'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
Expand Down