David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Add check-style for pre-commit hook ......................................................................
git-hooks: Add check-style for pre-commit hook
This imports check-style and some related lint tools from coreboot for the pre-commit hook: - Add check-style rule to Makefile - Replace the current pre-commit hook with the one from coreboot, omitting lint-stable for the time being. - Import checkpatch.pl, along with certain dependencies (spelling.txt and const_structs.checkpatch) and .checkpatch.conf. - Add .clang-format from CB:38673.
Change-Id: Ieb77aa1aa2911e081c69c93929a2f1c4430cd2b9 Signed-off-by: David Hendricks david.hendricks@gmail.com --- A .checkpatch.conf A .clang-format M Makefile M util/git-hooks/pre-commit A util/lint/check-style A util/lint/checkpatch.pl A util/lint/const_structs.checkpatch A util/lint/lint-007-checkpatch A util/lint/spelling.txt 9 files changed, 16,655 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/02/43602/1
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Add check-style for pre-commit hook ......................................................................
Patch Set 1:
This should go along with reformatting the tree (CB:42556). If I have some time in the near future I'll try using uncrustify for this, but for now clang-format seems to do a reasonably good job of linting patch submissions.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Add check-style for pre-commit hook ......................................................................
Patch Set 1: Code-Review-2
(4 comments)
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@11 PS1, Line 11: Add check-style rule to Makefile It would need to be added to meson in this commit as well. I think long term we should drop the Makefile in favour of just meson however i'll leave that discussion to another commit.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@14 PS1, Line 14: - Import checkpatch.pl, along with certain dependencies (spelling.txt : and const_structs.checkpatch) and .checkpatch.conf. I am not so keen, -2 not so keen, on having coreboot's unreasonbly slow pre-commit hook stuff land here as well.
If spelling and that sort of thing should be checked then a build target that is run by the bot and a report generated is more reasonable imho.
Regardless, in it's current form it isn't fit for purpose here yet. Can we take it out and just land a working clang-format we can all agree on and have that done please?
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@16 PS1, Line 16: Add .clang-format from CB:3 I am in favour of clang-format coming to Flashrom and it has been suggested a number of times now.
I don't recall precisely what Nico had to say but it was along the lines of Flashrom having sometimes longer lines than usual in some cases? Nico please correct me here.
In any case, I would be in favour of this part of the changeset and having it hooked up to the build bot as a target that is run as well as a pre-commit hook.
https://review.coreboot.org/c/flashrom/+/43602/1/util/lint/const_structs.che... File util/lint/const_structs.checkpatch:
https://review.coreboot.org/c/flashrom/+/43602/1/util/lint/const_structs.che... PS1, Line 1: cpi_dock_ops : address_space_operations : backlight_ops : block_device_operations : dentry_operations : dev_pm_ops : dma_map_ops : extent_io_ops : file_lock_operations : file_operations : hv_ops : ide_dma_ops : intel_dvo_dev_ops : item_operations : iwl_ops : kgdb_arch : kgdb_io : kset_uevent_ops : lock_manager_operations : microcode_ops : mtrr_ops : neigh_ops : nlmsvc_binding : of_device_id : pci_raw_ops : pipe_buf_operations : platform_hibernation_ops : platform_suspend_ops : proto_ops : rpc_pipe_ops : seq_operations : snd_ac97_build_ops : soc_pcmcia_socket_ops : stacktrace_ops : sysfs_ops : tty_operations : uart_ops : usb_mon_operations : wd_ops none of these have anything to do with flashrom
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Add check-style for pre-commit hook ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@11 PS1, Line 11: Add check-style rule to Makefile
It would need to be added to meson in this commit as well. […]
I'm not sure I understand... The pre-commit hook doesn't care about the build system. For now it just calls `make check-style` but could easily be modified to run the meson equivalent (which we should definitely add, I or someone just needs to figure out how).
The broader make vs. meson issue is another discussion, as you point out.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@14 PS1, Line 14: - Import checkpatch.pl, along with certain dependencies (spelling.txt : and const_structs.checkpatch) and .checkpatch.conf.
I am not so keen, -2 not so keen, on having coreboot's unreasonbly slow pre-commit hook stuff land here as well.
I did a few tests with this patch and `git commit -m` took around 0.4 seconds. I agree that all of coreboot's checks are slow, and I haven't looked into which ones are most worst offenders, but at least this seems pretty fast.
If spelling and that sort of thing should be checked then a build target that is run by the bot and a report generated is more reasonable imho.
I'm not entirely sure what you mean, but FWIW I don't think the spell checker currently works with this patch so we can try to leave it out (if possible). I actually didn't add it intentionally, checkpatch just gave me an error when I didn't have it available.
Regardless, in it's current form it isn't fit for purpose here yet. Can we take it out and just land a working clang-format we can all agree on and have that done please?
I'm not sure why it's not fit for purpose, but yes, if desired we can omit the checkpatch stuff and just have `make check-style` that runs clang-format.
BTW, here is sample output: https://paste.flashrom.org/view.php?id=3345
The `make check-style` part is the diff that is shown first, the checkpatch part is below where it says "Running checkpatch".
If we do as you suggest then the user will only see the diff, not the rules that provide rationale for the changes. I personally think the checkpatch output is nice to have, but am willing to remove it if you prefer it that way.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@16 PS1, Line 16: Add .clang-format from CB:3
I am in favour of clang-format coming to Flashrom and it has been suggested a number of times now.
Great! Feel free to add your thoughts to CB:38673. Also have a look at CB:42556 which uses uncrustify and has some comments about limitations I encountered with clang-format.
I don't recall precisely what Nico had to say but it was along the lines of Flashrom having sometimes longer lines than usual in some cases? Nico please correct me here.
There was a discussion about this on the mailing list a while back (maybe 2 years ago?) where it was decided that we'll allow up to 112 columns (tables can be longer). I more info on that to the wiki: https://flashrom.org/Development_Guidelines#Coding_style
In any case, I would be in favour of this part of the changeset and having it hooked up to the build bot as a target that is run as well as a pre-commit hook.
Agreed.
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit File util/git-hooks/pre-commit:
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit@2... PS1, Line 28: sleep 5 We can reduce or remove this if desired.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/43602
to look at the new patch set (#2).
Change subject: git-hooks: Import check-style from coreboot ......................................................................
git-hooks: Import check-style from coreboot
This imports check-style from coreboot. It will be modified and used in the follow-up patch.
Change-Id: Ieb77aa1aa2911e081c69c93929a2f1c4430cd2b9 Signed-off-by: David Hendricks david.hendricks@gmail.com --- A util/lint/check-style 1 file changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/02/43602/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Import check-style from coreboot ......................................................................
Patch Set 2:
(5 comments)
I tried simplifying this so that the check-style script is run directly from the pre-commit hook (i.e. no Makefile or Meson changes needed) and we can also omit the cruft you pointed out.
This patch now just imports check-style from coreboot, the follow-up makes flashrom-specific changes.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@11 PS1, Line 11: Add check-style rule to Makefile
I'm not sure I understand... The pre-commit hook doesn't care about the build system. […]
Removed Makefile rule in favor of calling check-style directly from pre-commit hook.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@14 PS1, Line 14: - Import checkpatch.pl, along with certain dependencies (spelling.txt : and const_structs.checkpatch) and .checkpatch.conf.
I am not so keen, -2 not so keen, on having coreboot's unreasonbly slow pre-commit hook stuff land […]
Removed checkpatch and spelling-related stuff.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@16 PS1, Line 16: Add .clang-format from CB:3
I am in favour of clang-format coming to Flashrom and it has been suggested a number of times now. […]
Moved to CB:44095
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit File util/git-hooks/pre-commit:
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit@2... PS1, Line 28: sleep 5
We can reduce or remove this if desired.
Removed.
https://review.coreboot.org/c/flashrom/+/43602/1/util/lint/const_structs.che... File util/lint/const_structs.checkpatch:
https://review.coreboot.org/c/flashrom/+/43602/1/util/lint/const_structs.che... PS1, Line 1: cpi_dock_ops : address_space_operations : backlight_ops : block_device_operations : dentry_operations : dev_pm_ops : dma_map_ops : extent_io_ops : file_lock_operations : file_operations : hv_ops : ide_dma_ops : intel_dvo_dev_ops : item_operations : iwl_ops : kgdb_arch : kgdb_io : kset_uevent_ops : lock_manager_operations : microcode_ops : mtrr_ops : neigh_ops : nlmsvc_binding : of_device_id : pci_raw_ops : pipe_buf_operations : platform_hibernation_ops : platform_suspend_ops : proto_ops : rpc_pipe_ops : seq_operations : snd_ac97_build_ops : soc_pcmcia_socket_ops : stacktrace_ops : sysfs_ops : tty_operations : uart_ops : usb_mon_operations : wd_ops
none of these have anything to do with flashrom
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Import check-style from coreboot ......................................................................
Patch Set 2: -Code-Review
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Import check-style from coreboot ......................................................................
Patch Set 2: Code-Review+1
-
Angel Pons (Code Review) -
David Hendricks (Code Review)
-
Edward O'Callaghan (Code Review)