[coreboot] WG: What is the best way to treat warnings reported by checkpatch.pl

Martin Roth gaumless at gmail.com
Thu Jul 28 00:06:22 CEST 2016


Hey Werner,

Thanks for bringing up this topic.  It's something we've discussed a
bunch of times, but hadn't done anything about until now.

Here's what I've done:

I've pushed the .checkpatch.conf from the Chrome OS coreboot tree
https://review.coreboot.org/#/c/15919/

I had initially pushed a patch to update the pre-commit git hook to
say "If this failure is invalid, bypass it with 'git commit -n'.", but
i decided I'd rather do that inside checkpatch.pl so everyone who has
already run make gitconfig will get the fix, so I abandoned that
change. It needed some additional fixes anyway.  I'll push another
commit shortly.

I've also added a jenkins build to run checkpatch on every commit, but
that doesn't give any feedback in the commit, so it doesn't seem super
useful right now.  You have to go into the console output for the
jenkins build to see the results.
Here's an example: https://qa.coreboot.org/job/coreboot-checkpatch/30/console


What other changes to checkpatch are needed?

Martin

On Tue, Jul 26, 2016 at 10:34 PM, Zeh, Werner <werner.zeh at siemens.com> wrote:
>>Does git commit --no-verify (or -n for short) allow you to commit?
>>
>
> Yes, one can go this way and I did it already earlier. But I just wanted to point it out here.
> We do not need a check script for every commit if we simply disable the check when it comes to issues.
> I wanted to start this discussion to improve the situation with this script we currently have.
>
>>I think we should try to do a little of option 1 within reason, not by forking the script but rather by disabling more check steps that don't match the coreboot code style (by >having our wrapper pass --ignore XXX flags to checkpatch) and possibly upstreaming checkpatch.pl patches with new features we need to the Linux kernel (to make the >detection more accurate or add a more specific --ignore flag we can turn on). In the Chromium fork of coreboot we're already using a bunch of those flags that we should >probably use in upstream coreboot as well:
>>
>>--ignore C99_COMMENTS
>>--ignore GLOBAL_INITIALISERS
>>--ignore INITIALISED_STATIC
>>--ignore LINE_SPACING
>>--ignore NEW_TYPEDEFS
>>--ignore PREFER_ALIGNED
>>--ignore PREFER_PACKED
>>--ignore PREFER_PRINTF
>>--ignore SPLIT_STRING
>>
>
> If we really did not touch the contents of the script, I totally agree with you. We can disable warnings that do not match our coding style while updating the script from its origin from time to time. If we have already started tailoring this script, than we should do it the right way and end this tailoring process to match our needs. I admit that the later one will end up in more work if one wants to synchronize this script with origin again one day.
>
> Werner
> --
> coreboot mailing list: coreboot at coreboot.org
> https://www.coreboot.org/mailman/listinfo/coreboot



More information about the coreboot mailing list