Presubmit include guard test warns on generated third_party code |
||||
Issue descriptionThe presubmit include guard test is firing on third party code, even generated files. Therefore it is necessary to use NOTRY=true to land a change that changes the generated files. For example, in https://chromium-review.googlesource.com/c/chromium/src/+/1044347 After regenerating ia2_api_all.h there were errors that indicate the need for #define __REQUIRED_RPCNDR_H_, which was not not auto-generated, and it seems to be correct as is. In fact I don't see the expected __REQUIRED_RPCNDR_H_ used anywhere, only __REQUIRED_RPCNDR_H_VERSION__ It appears this presubmit was added fairly recently in https://chromium-review.googlesource.com/c/chromium/src/+/931761, and it should not fire for third-party code (according to thakis).
,
May 23 2018
You get/got warnings if you have non standard guard names, but that does not block any integrations as far as I know.
There were 2 warnings and 1 error in your presubmit check (run 2018-05-11)
Warning #1: Style of include guards (changed after that run to not care about third_party)
Warning #2: Request to run git cl format.
Error #1: Missing LGTM.
** Presubmit Messages **
If this change has an associated bug, add Bug: [bug number].
** Presubmit Warnings **
Missing "#define __REQUIRED_RPCNDR_H_" for include guard
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.h:24
***************
Expected: u'#define __REQUIRED_RPCNDR_H_'
Got: u'#define __REQUIRED_RPCNDR_H_VERSION__ 475'
***************
Missing "#define __REQUIRED_RPCNDR_H_" for include guard
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.h:24
***************
Expected: u'#define __REQUIRED_RPCNDR_H_'
Got: u'#define __REQUIRED_RPCNDR_H_VERSION__ 475'
***************
The src directory requires source formatting. Please run: git cl format
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/app/strings/content_strings.grd
third_party/closure_compiler/externs/automation.js
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.tlb
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.tlb
ui/accessibility/ax_enums.mojom
Missing LGTM from an OWNER for these files:
content/app/strings/content_strings.grd
third_party/closure_compiler/externs/automation.js
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.tlb
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.tlb
ui/accessibility/ax_enums.mojom
,
May 30 2018
This is still a problem. See presubmit errors when trying to land https://chromium-review.googlesource.com/c/chromium/src/+/1078928
,
May 30 2018
I'll take a look at why the warnings are there tomorrow, but your problem has has never been the warnings but the presubmit errors:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x64/ia2_api_all.tlb
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.h
third_party/win_build_output/midl/third_party/iaccessible2/x86/ia2_api_all.tlb
,
May 30 2018
The include guard warning is still incorrectly printed for these files though. It's not blocking the bot, but still incorrect.
,
May 30 2018
Ok, maybe it isn't blocking, my mistake. It also just fools me when I try to git cl upload upload. It says a git cl format which, when I do it, isn't enough. It wants another git cl format at that point. Doing those formats and uploading causes it to not compile on the trybots (although for some reason it compiled locally). I wish the git cl format was bypassed for these generated files entirely.
,
May 31 2018
I've looked at why the guard presubmit check still triggers for the win_build_output headers and it's not because they have the wrong guard names as I initially thought but because they have no include guards at all. Or rather they have many small include guards that cover parts but not all of the time. I suspect those headers are not safe against double inclusion and that could cause problems in Chromium code in the future but since they are out of our control it's no value in warning. I'll upload a patch that disables more of the include guard check. I've also noticed that sometimes you have to run git cl format more than once to reach a fix point, but most of the time it gets it right.
,
May 31 2018
@bratell, when I run git cl format and upload the changes, they no longer compile.
,
May 31 2018
Doh. That is bad. You should file a bug on cl format, bug I'm not sure who maintains it.
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a75baef68b117b0b53245d2dc450588c25d4a4f commit 6a75baef68b117b0b53245d2dc450588c25d4a4f Author: Daniel Bratell <bratell@opera.com> Date: Mon Jun 04 10:04:45 2018 Disable all of the include guard presubmit check for third_party The style check for include guards in third_party had already been disabled but there are also headers in third_party with no normal include guards at all so we better disable all of the check. (blink is not considered third_party). Bug: 845584 Change-Id: I801022a18daec38e9e6788c313e6bac6ed8a150b Reviewed-on: https://chromium-review.googlesource.com/1080511 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Daniel Bratell <bratell@opera.com> Cr-Commit-Position: refs/heads/master@{#564045} [modify] https://crrev.com/6a75baef68b117b0b53245d2dc450588c25d4a4f/PRESUBMIT.py
,
Jun 4 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by thakis@chromium.org
, May 22 2018