New issue
Advanced search Search tips

Issue 814776 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Presubmit check for missing or broken include guards

Project Member Reported by brat...@opera.com, Feb 22 2018

Issue description

Jumbo 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.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ba52722ce6a940fa96221f415c23dec331d6313

commit 8ba52722ce6a940fa96221f415c23dec331d6313
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Mar 02 16:06:14 2018

Add presubmit check for include guards

Missing include guards tend to break jumbo builds and makes
everyone involved sad so better to have a presubmit check for it.

There is a risk that this triggers on correct code so it includes
a way to disable it per source file. Include the string
"no-include-guard-because-multiply-included" and there won't
be any warning for that file.

By popular demand there will also be a warning if the name of
the include guard doesn't follow the coding standards, but since
mistakes are common, that check is only enabled for new files.

Bug:  814776 
Change-Id: Id18b3d43288b9a064e537460d667957b355bbd33
Reviewed-on: https://chromium-review.googlesource.com/931761
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540526}
[modify] https://crrev.com/8ba52722ce6a940fa96221f415c23dec331d6313/PRESUBMIT.py
[modify] https://crrev.com/8ba52722ce6a940fa96221f415c23dec331d6313/PRESUBMIT_test.py

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 

Comment 3 by brat...@opera.com, Mar 12 2018

Status: Fixed (was: Started)
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)
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by gan...@chromium.org, 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_'
***************

Comment 6 by gan...@chromium.org, May 16 2018

Status: Untriaged (was: Fixed)

Comment 7 by brat...@opera.com, May 16 2018

Cc: gan...@chromium.org
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?

Comment 8 by thakis@chromium.org, 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.)

Comment 9 by brat...@opera.com, May 16 2018

Were those cases also in third_party/win_build_output or was there some other pattern that identified them as special?
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.

Comment 11 by brat...@opera.com, 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.

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Comment 13 by brat...@opera.com, May 17 2018

Status: Fixed (was: Untriaged)

Sign in to add a comment