Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
util/lint/checkpatch.pl: Update line length to 96
Change-Id: I7f34c93c2af4a7f1928af28320ef2abd66dac0bb Signed-off-by: Angel Pons th3fanbus@gmail.com --- M util/lint/checkpatch.pl 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/34386/1
diff --git a/util/lint/checkpatch.pl b/util/lint/checkpatch.pl index 1affdb7..c6cbe69 100755 --- a/util/lint/checkpatch.pl +++ b/util/lint/checkpatch.pl @@ -51,7 +51,7 @@ my @exclude = (); #coreboot my $help = 0; my $configuration_file = ".checkpatch.conf"; -my $max_line_length = 80; +my $max_line_length = 96; my $ignore_perl_version = 0; my $minimum_perl_version = 5.10.0; my $min_conf_desc_length = 4;
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
Note: I have not tested this.
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
What's the rationale?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
What's the rationale?
coreboot's changing to a maximum length of 96 characters per line:
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/OWHSS... https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/2JLGM...
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96; I think we can just add --max-line-length 96 to the coreboot/checkpatch.conf file.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
I think we can just add --max-line-length 96 to the coreboot/checkpatch.conf file.
In fact it's already there. See CB:33405. And I can confirm that there've been no more warnings for 80 char violations recently, I'm pretty sure it works.
(BTW, do we have an ETA for when the code base will be reformatted? The current state is super annoying -- I keep having to find line length violations manually and ask people to please stick to the rest of the file's style until it is converted in full. We just went from having *some* enforced standard to no enforced standard at all, which I'm pretty sure is not what people wanted. You guys were very passionate about making this decision, now please actually implement it too.)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
In fact it's already there. See CB:33405. […]
Regarding the timeline, we wanted to do this post-release, and I messed up releasing coreboot 4.10. Sorry about that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
In fact it's already there. See CB:33405. […]
An issue with the old 80-char limit on checkpatch still being present was brought up on the leadership meeting by Werner Zeh. I did a quick search in checkpatch w.r.t. the old limit and found this. I guess he could elaborate more w.r.t. these issues.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
An issue with the old 80-char limit on checkpatch still being present was brought up on the leadersh […]
Hmmm... maybe I was wrong. It looks like we do still get the warning sometimes (e.g. https://review.coreboot.org/c/coreboot/+/33387/9/src/mainboard/emulation/qem... ). I could swear that I've seen violations that weren't pointed out elsewhere, but maybe I'm confused (or maybe that was in file types checkpatch doesn't cover or something).
Either way, then we should figure out why the way it's currently done doesn't work.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
Hmmm... maybe I was wrong. It looks like we do still get the warning sometimes (e.g. https://review. […]
That happened because that patchset was based on a commit that didn't have the 96 characters per line override yet: the tests are always executed from the tree that is under test. That generally doesn't matter too much, but we may have to revisit that approach.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
That happened because that patchset was based on a commit that didn't have the 96 characters per lin […]
I found this issue when we were working on https://review.coreboot.org/c/coreboot/+/34384/1. If I for instance take src/drivers/spi/winbond.c and move the call to ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT_MS); on a single line, this will end up being 81 characters. Adding this patch will then let checkpatch.pl complain about 80 characters:
WARNING: line over 80 characters #11: FILE: src/drivers/spi/winbond.c:329: + ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT_MS);
Though it is just a warning my expectation was that even the warning should not appear after increasing the length to 96 characters.
After searching around for a while I found that the change made in CB:33405 will only be active if one have updated the pre-commit-hook to the new style which uses 'util/lint/lint-007-checkpatch diff' now. My old pre-commit-hook still have the style like this:
#!/bin/sh make lint-stable git diff --cached | util/lint/checkpatch.pl --no-signoff -q -
And here is where the 96 characters are missing. This pretty much explains the issue I have.
And you cannot get the new hook if you still have the old in place as the gitconfig.sh script checks for an existing hook and will not update it if it is already there with the needed name.
Nevertheless, while I was looking for this issue I found the usage text in checkpatch.pl which says how to pass the argument for max-line-length to checkpatch.pl. Here it states:
--max-line-length=n set the maximum line length, if exceeded, warn
What CB:33405 introduced was:
opts="--max-line-length 96"
and
util/lint/checkpatch.pl --quiet --no-signoff $opts $args -
Am I mistaken or is there a missing '=' before 96?
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
I found this issue when we were working on https://review.coreboot.org/c/coreboot/+/34384/1. […]
Updating my pre-commit-hook solves the issue. So sorry for the noise. On the other hand: can we ensure that the hooks are updated if there was a relevant change, is there a way to do it?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
Updating my pre-commit-hook solves the issue. So sorry for the noise. […]
We changed the pre-commit hook last year to just call lint-007 instead of calling checkpatch by hand, so that issue should resolve itself over time.
More generally speaking, we don't want to change automatically running scripts to change without notice (otherwise we could probably just add a symlink to util/gitconfig/pre-commit to .git/hooks), and I'm not quite sure where we should put any kind of warning that the script was updated.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/34386/1/util/lint/checkpatch.pl@54 PS1, Line 54: my $max_line_length = 96;
We changed the pre-commit hook last year to just call lint-007 instead of calling checkpatch by hand […]
Sure, understand. From my point of view we can abandon this patch. Sorry for the nois, thanks Angel for digging into.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34386 )
Change subject: util/lint/checkpatch.pl: Update line length to 96 ......................................................................
Abandoned
Reached happy ending without this patch
-
Angel Pons (Code Review) -
Idwer Vollering (Code Review)
-
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Werner Zeh (Code Review)