Presubmit check for missing or broken include guards |
|||||
Issue descriptionJumbo builds have broken a couple of times (more than 3, less than 6) this last half year because of mistakes with header include guards. Often missing completely, once without the #define and I think there once was a header using the same guard as another header file. The right thing is to create a presubmit check to prevent it, though it's a little tricky since include guards don't always look like the coding standard suggest.
,
Mar 9 2018
I just ran into this, and it looks like the CL in #2 is printing "Expected" and "Got" in wrong order: https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=90930b84a060e1685307cb3b83045548eeae4108&l=2894
,
Mar 12 2018
Thanks for the notice! I've uploaded a fix in https://chromium-review.googlesource.com/c/chromium/src/+/958466 for that. I hope the presubmit check was useful for you. (Closing as fixed since I forgot to do that when the change was committed)
,
Mar 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00e1b9bc8f88495a5ce98419bb352b902758ad9c commit 00e1b9bc8f88495a5ce98419bb352b902758ad9c Author: Daniel Bratell <bratell@opera.com> Date: Mon Mar 12 13:11:12 2018 Fix mixup in expected/got in include guard presubmit check Bug: 814776 Change-Id: If68e4015c54a0b60f9a68b7941fb3e5d14b58502 Reviewed-on: https://chromium-review.googlesource.com/958466 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Daniel Bratell <bratell@opera.com> Cr-Commit-Position: refs/heads/master@{#542469} [modify] https://crrev.com/00e1b9bc8f88495a5ce98419bb352b902758ad9c/PRESUBMIT.py
,
May 16 2018
This presubmit added in https://chromium-review.googlesource.com/c/chromium/src/+/931761 should not be firing for third-party code, for example, checked-in generated files. Could you please fix? Sample output: Header using the wrong include guard name __REQUIRED_RPCNDR_H_ third_party\win_build_output\midl\chrome_service\x64\chrome_service_idl.h:23 *************** Expected: 'THIRD_PARTY_WIN_BUILD_OUTPUT_MIDL_CHROME_SERVICE_X64_CHROME_SERVICE_IDL_H_' Found: u'__REQUIRED_RPCNDR_H_' ***************
,
May 16 2018
,
May 16 2018
It's just a warning, right? We had a long discussion in the review about how strict it should be and I think we ended with the conclusion that it would not fire by accident very often and only in special cases. The presubmit contains both a part that checks that include guards exist and a part that checks the name. I think the existence part should be available all the time so I consider your request to only be about the name checking. @dcheng, what do you think considering this example of a real world warning?
,
May 16 2018
(fwiw I had two people ping me about this script confusing them for the checked-in midl files since this landed, and it's only been in for about 2 months. So something needs to change.)
,
May 16 2018
Were those cases also in third_party/win_build_output or was there some other pattern that identified them as special?
,
May 16 2018
They were in third_party/win_build_output, yes. I'd argue nothing in third_party except stuff on a whitelist (blink...) should be checked by this.
,
May 16 2018
Currently only files in third_party that are in the main repo I don't have any strong feelings here. The important part to me was to stop files with no include guards. The rest was feature creep. I'll throw up a CL and we can take it in the review.
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39b5b069371d60a8d4f47c4d7fb01d3493ebf1d0 commit 39b5b069371d60a8d4f47c4d7fb01d3493ebf1d0 Author: Daniel Bratell <bratell@opera.com> Date: Wed May 16 18:09:57 2018 Don't check header guard style in third_party The presubmit check warning for correct include guards would trigger for generated files in third_party/win_build_output. Since files in third_party is often not written with the Chromium style guide in mind that is over zealous so this patch stops checking their names (except for in blink). The presubmit code will still check the existence of a guard since lack of one can cause problems outside the module and there has been no reports of false warnings. This patch also removes the exceptions that used to be there for blink since blink has moved and been reformatted. Bug: 814776 Change-Id: Ic7f2bbba5b9bcafbb5bec15b79b04d5e81994b77 Reviewed-on: https://chromium-review.googlesource.com/1061698 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Daniel Bratell <bratell@opera.com> Cr-Commit-Position: refs/heads/master@{#559185} [modify] https://crrev.com/39b5b069371d60a8d4f47c4d7fb01d3493ebf1d0/PRESUBMIT.py [modify] https://crrev.com/39b5b069371d60a8d4f47c4d7fb01d3493ebf1d0/PRESUBMIT_test.py
,
May 17 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Mar 2 2018