New issue
Advanced search Search tips

Issue 829979 link

Starred by 0 users

Issue metadata

Status: WontFix
Owner:
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

re-enable FindBadConstructs clang plugin for standalone v8 build

Project Member Reported by most...@vewd.com, Apr 6 2018

Issue description

In https://chromium-review.googlesource.com/c/chromium/src/+/970424 we turned on the FindBadConstruct clang plugin's no-realpath flag, which worked in chromium builds, but broke standalone V8 builds:

> Ahh, I see the problem my change caused: the "/v8/" pattern in
> banned_directories_ works when building the chromium tree because
> paths to v8 sources relative to the out dir look like "../../v8/foo",
> whereas in a standalone v8 build the equivalent path would be
> "../../foo".  

> Things just happen to work in the standalone v8 build because the
> checkout location is a directory named "v8" on the bots, so the
> absolute path contains this and then all sources are considered
> "banned".  I don't think the style checker plugin should make
> assumptions about the location of the root checkout dir, but I
> need to think some more about how this should be implemented.

So prevent this from blocking V8 rolls, the plugin was disabled in
standalone v8 builds:
https://chromium-review.googlesource.com/998153

To avoid more trouble, let's re-enable the plugin in standalone v8
builds in such a way that v8 code is not misclassified as "chromium"
code, so we still run the tests that chromium builds run on "third party"
code.
 
v8 code _is_ chromium_code; intentionally. It's not in the chromium repo, but it (intentionally) builds with gn's chromium_code config (which means "more warnings", essentially)

So I think disabling the plugin in v8 is actually the right fix, until the day v8 decides they want to enable the plugin.

Comment 2 by most...@vewd.com, Apr 6 2018

The plugin looks like it might runs some checks on "third party" code, which is how the chromium build treats v8.  If this is the case (I'm not 100% sure), then I think the standalone v8 build should attempt to run the same checks, to avoid delaying v8 rolls.

Otherwise I agree- no need to enable the plugin in v8 builds if it doesn't actually check anything.

Comment 3 by dcheng@chromium.org, Apr 12 2018

The only third_party directory that's checked is blink (and that's because it's not really third party anymore--and even then, it's on by default there yet). The plugin itself is really only intended to be run on code that lives in the chromium repo.
Should this be then closed as WontFix?

Comment 5 by most...@vewd.com, Apr 26 2018

Status: WontFix (was: Untriaged)
I think so- wontfix'ing.

Sign in to add a comment