New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Verified
Owner:
Last visit 15 days ago
Closed: Dec 20
Components:
HW: ----
NextAction: ----
OS: ----
Priority: 2
Type: Bug



Sign in to add a comment
link

Issue 8482: V8's presubmit in CI should run without cpplint cache

Reported by machenb...@chromium.org, Nov 21 Project Member

Issue description

This is to prevent problems like:
https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Presubmit/454

- tldr

The cache is very useful when running locally. In CI however, we get bots from a pool, resulting in very different cache states from build to build. We should add a flag allowing to run without cache and pass that flag on the bots.

- details

In the given case above, a header was added in some CL A. The corresponding cc file never included this header and didn't have to. CL A didn't touch the cc file, hence the pre-commit presubmit didn't check this.

The post-commit "presubmit" (yes, we should find a better name) should have failed immediately on CL A. But the cc file was in the cache of the respective run. A later run on innocent CL B used a bot with a different cache and ran the lint check on the cc file, discovering that it now needs this header.
 

Comment 1 by serg...@chromium.org, Nov 21

> The post-commit "presubmit"
We call it "push hook" or "presubmit on commit" and the other one is called "upload hook" or "presubmit on upload".

AFAIK, the both presubmits pass a list of the modified files to cpplint.py. IIUC both CL A and CL B did not modify the cc file - why would cpplint.py check it? If the CL B did modify the cc file, then perhaps we should teach cpplint.py to always validate the cc file when the header file is modified and vice versa.

Comment 2 by machenb...@chromium.org, Nov 22

The real pre-commit presubmit would not detect this problem anyways. But when deleting the cache, the post-commit presubmit would at least blame the right CL.

Comment 3 by machenb...@chromium.org, Dec 4

Owner: tm...@chromium.org
Status: Assigned (was: Available)
PTAL

Comment 4 by tm...@chromium.org, Dec 10

I've submitted https://chromium-review.googlesource.com/c/v8/v8/+/1370029 to solve this, I'll enable it in the CI recipes once its landed

Comment 5 by bugdroid1@chromium.org, Dec 18

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/11abc5ecdc8f87c5524dc335b8d1781ec47376b0

commit 11abc5ecdc8f87c5524dc335b8d1781ec47376b0
Author: Tamer Tas <tmrts@chromium.org>
Date: Tue Dec 18 13:53:35 2018

[test] create a CacheableSourceFileProcessor superclass for changed files

Added tests for the existing FileContentsCache, and created a superclass
that removes the duplicated code from Torque and CPP linters

R=machenbach@chromium.org,sergiyb@chromium.org
CC=​​​​yangguo@chromium.org
NOTRY=true

Bug:  v8:8482 
Change-Id: Ic7a0b3d58c64f395e790d4ff668fa804c05478be
Reviewed-on: https://chromium-review.googlesource.com/c/1369949
Commit-Queue: Tamer Tas <tmrts@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58321}
[add] https://crrev.com/11abc5ecdc8f87c5524dc335b8d1781ec47376b0/tools/unittests/v8_presubmit_test.py
[modify] https://crrev.com/11abc5ecdc8f87c5524dc335b8d1781ec47376b0/tools/v8_presubmit.py

Comment 6 by bugdroid1@chromium.org, Dec 18

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a5a381bd96d7b2309860a09cd70801d0019b91d5

commit a5a381bd96d7b2309860a09cd70801d0019b91d5
Author: Tamer Tas <tmrts@chromium.org>
Date: Tue Dec 18 15:37:09 2018

[test] add an option for disabling linter cache in the pre_submit check

Adds a flag to specify whether to disable the linter caching.

R=machenbach@chromium.org,sergiyb@chromium.org
CC=​​yangguo@chromium.org

Bug:  v8:8482 
Change-Id: I62a9b7cffb3adb50b136659568ad52078675ca4b
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1370029
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Tamer Tas <tmrts@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58329}
[modify] https://crrev.com/a5a381bd96d7b2309860a09cd70801d0019b91d5/tools/unittests/v8_presubmit_test.py
[modify] https://crrev.com/a5a381bd96d7b2309860a09cd70801d0019b91d5/tools/v8_presubmit.py

Comment 7 by bugdroid1@chromium.org, Dec 20

Project Member
Labels: merge-merged-7.1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6916c59c77a876d9702595ed21a4f31cf2682dc1

commit 6916c59c77a876d9702595ed21a4f31cf2682dc1
Author: Tamer Tas <tmrts@chromium.org>
Date: Thu Dec 20 12:39:22 2018

Merged: Squashed multiple commits.

Merged: [test] make python recognize tools and tools/unittests as packages
Revision: 956c8a50ce

Merged: [test] create a CacheableSourceFileProcessor superclass for changed files
Revision: 11abc5ecdc

Merged: [test] add an option for disabling linter cache in the pre_submit check
Revision: a5a381bd96

BUG= v8:8482 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=machenbach@chromium.org

Change-Id: I21f668c556248eb300aa9be66e05e31f13819d32
Reviewed-on: https://chromium-review.googlesource.com/c/1386489
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Tamer Tas <tmrts@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.1@{#67}
Cr-Branched-From: f70aaa8ab2e8815505a6145c745e50d8328cd28c-refs/heads/7.1.302@{#1}
Cr-Branched-From: 1dbcc78efa17a9047f7e923958087ef9eec43066-refs/heads/master@{#56462}
[add] https://crrev.com/6916c59c77a876d9702595ed21a4f31cf2682dc1/tools/__init__.py
[add] https://crrev.com/6916c59c77a876d9702595ed21a4f31cf2682dc1/tools/unittests/__init__.py
[add] https://crrev.com/6916c59c77a876d9702595ed21a4f31cf2682dc1/tools/unittests/v8_presubmit_test.py
[modify] https://crrev.com/6916c59c77a876d9702595ed21a4f31cf2682dc1/tools/v8_presubmit.py

Comment 8 by bugdroid1@chromium.org, Dec 20

Project Member
Labels: merge-merged-7.2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fe1da91bae60887519118f00f0efce34ee733f68

commit fe1da91bae60887519118f00f0efce34ee733f68
Author: Tamer Tas <tmrts@chromium.org>
Date: Thu Dec 20 12:42:23 2018

Merged: Squashed multiple commits.

Merged: [test] make python recognize tools and tools/unittests as packages
Revision: 956c8a50ce

Merged: [test] create a CacheableSourceFileProcessor superclass for changed files
Revision: 11abc5ecdc

Merged: [test] add an option for disabling linter cache in the pre_submit check
Revision: a5a381bd96

BUG= v8:8482 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=machenbach@chromium.org

Change-Id: Ic5ff281ece512496308d1ed4d15d01d217261751
Reviewed-on: https://chromium-review.googlesource.com/c/1386490
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Tamer Tas <tmrts@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.2@{#31}
Cr-Branched-From: 6acd03c9b8a8232aee95f25fbf6ae822aaedae75-refs/heads/7.2.502@{#1}
Cr-Branched-From: b03041de094610ef24e0e4fb6bf4c700fa1553ed-refs/heads/master@{#57910}
[add] https://crrev.com/fe1da91bae60887519118f00f0efce34ee733f68/tools/__init__.py
[add] https://crrev.com/fe1da91bae60887519118f00f0efce34ee733f68/tools/unittests/__init__.py
[add] https://crrev.com/fe1da91bae60887519118f00f0efce34ee733f68/tools/unittests/v8_presubmit_test.py
[modify] https://crrev.com/fe1da91bae60887519118f00f0efce34ee733f68/tools/v8_presubmit.py

Comment 9 by bugdroid1@chromium.org, Dec 20

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/fb8bd272c70dd6fa5b1194ecfdd124e952280941

commit fb8bd272c70dd6fa5b1194ecfdd124e952280941
Author: Tamer Tas <tmrts@chromium.org>
Date: Thu Dec 20 12:54:41 2018

[V8] don't use linter caches for presubmit check in CI

The cache is very useful when running locally. In CI however, we get bots from a
pool, resulting in very different cache states from build to build. This
disables the cache in bots by using the newly added flag.

R=machenbach@chromium.org
CC=yangguo@chromium.org,sergiyb@chromium.org

Bug:  v8:8482 
Change-Id: I67a007232903b596d4541da6ccea6bb49d238ce2
Reviewed-on: https://chromium-review.googlesource.com/c/1382572
Reviewed-by: Sergiy Belozorov <sergiyb@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Tamer Tas <tmrts@chromium.org>

[modify] https://crrev.com/fb8bd272c70dd6fa5b1194ecfdd124e952280941/scripts/slave/recipes/v8/presubmit.expected/basic.json
[modify] https://crrev.com/fb8bd272c70dd6fa5b1194ecfdd124e952280941/scripts/slave/recipes/v8/presubmit.py

Comment 10 by tm...@chromium.org, Dec 20

Status: Verified (was: Assigned)
It works now, you can verify it by looking at the logs for the last 2 builds by the V8 Presubmit Builder. They both contain lint checks without caching

V8 Presubmit Builder: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Presubmit

Sign in to add a comment