Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393863002:
[inspector] Add swarming support to inspector tests (Closed)
|
Created:
4 years, 2 months ago by Michael Achenbach Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Add swarming support to inspector tests
BUG=chromium:635948
Committed: https://crrev.com/3195f19e879c37d8f421defa3a2608e5c15f45df
Cr-Commit-Position: refs/heads/master@{#40194}
Patch Set 1 #Patch Set 2 : Missing file + switch inspector off on noi18nbot #Patch Set 3 : Format + fixed config when inspector is off #Patch Set 4 : Swarming + format #
Total comments: 2
Patch Set 5 : Fix swarming target when inspector is off #Patch Set 6 : Fix swarming #
Total comments: 3
Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Format (looking ugly) + remove gyp deps #Patch Set 9 : Detach from dependent patchset #
Messages
Total messages: 67 (51 generated)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
Description was changed from ========== [inspector] Add swarming support to inspector tests BUG= ========== to ========== [inspector] Add swarming support to inspector tests BUG=chromium:635948 ==========
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25728)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25910)
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25971)
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:387: 'gyp', 'release_bot', 'x64', 'swarming'], To fix last linker error on gyp linux release x64 bot we need to enable inspector in this configuration since we wouldn't enable inspector by default for gyps build in standalone.gypi.
https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:387: 'gyp', 'release_bot', 'x64', 'swarming'], On 2016/10/10 17:29:17, kozyatinskiy wrote: > To fix last linker error on gyp linux release x64 bot we need to enable > inspector in this configuration since we wouldn't enable inspector by default > for gyps build in standalone.gypi. I don't understand. My plan was to not enable inspector for any gyp bot to keep coverage of the non-inspector case. This bot is testing this case. What else can be done to make it work with inspector off?
machenbach@chromium.org changed reviewers: + yangguo@chromium.org
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26118)
On 2016/10/11 09:01:54, machenbach (slow) wrote: > https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl > File infra/mb/mb_config.pyl (right): > > https://codereview.chromium.org/2393863002/diff/60001/infra/mb/mb_config.pyl#... > infra/mb/mb_config.pyl:387: 'gyp', 'release_bot', 'x64', 'swarming'], > On 2016/10/10 17:29:17, kozyatinskiy wrote: > > To fix last linker error on gyp linux release x64 bot we need to enable > > inspector in this configuration since we wouldn't enable inspector by default > > for gyps build in standalone.gypi. > > I don't understand. My plan was to not enable inspector for any gyp bot to keep > coverage of the non-inspector case. This bot is testing this case. What else can > be done to make it work with inspector off? Patch 6 runs now on all bots except gyp. Will rerun 'git cl format' once that's fixed.
Sorry, but then I think that we need to include test/inspector.gyp in all.gyp under v8_inspector_enabled check. https://codereview.chromium.org/2393863002/diff/100001/gypfiles/all.gyp File gypfiles/all.gyp (right): https://codereview.chromium.org/2393863002/diff/100001/gypfiles/all.gyp#newco... gypfiles/all.gyp:33: '../test/inspector/inspector.gyp:*', I think that problem is here. It looks like we include inspector.gyp always regardless v8_inspector_enabled flag. This leads to linker error on linux gyp bot. I prepared separate CL during fixing linker error [1] to sync BUILD.gn and this file, but feel free to introduce this check right here. [1] https://codereview.chromium.org/2411743002/
On 2016/10/11 14:46:48, kozyatinskiy wrote: > Sorry, but then I think that we need to include test/inspector.gyp in all.gyp > under v8_inspector_enabled check. > > https://codereview.chromium.org/2393863002/diff/100001/gypfiles/all.gyp > File gypfiles/all.gyp (right): > > https://codereview.chromium.org/2393863002/diff/100001/gypfiles/all.gyp#newco... > gypfiles/all.gyp:33: '../test/inspector/inspector.gyp:*', > I think that problem is here. It looks like we include inspector.gyp always > regardless v8_inspector_enabled flag. This leads to linker error on linux gyp > bot. > I prepared separate CL during fixing linker error [1] to sync BUILD.gn and this > file, but feel free to introduce this check right here. > > [1] https://codereview.chromium.org/2411743002/ Makes sense. You can land the other CL or I will incorporate it here tomorrow.
https://codereview.chromium.org/2393863002/diff/100001/test/inspector/BUILD.gn File test/inspector/BUILD.gn (right): https://codereview.chromium.org/2393863002/diff/100001/test/inspector/BUILD.g... test/inspector/BUILD.gn:8: if (v8_enable_inspector_override) { We should maybe just guard the whole gyp target like I did for GN here. Though, if we have the guard at the deps in all.gyp, it's not really required. Only in GN, every target will be found on ninja's toplevel all target automatically.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
machenbach@chromium.org changed reviewers: + dgozman@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
On 2016/10/11 18:22:54, machenbach (slow) wrote: > PTAL Forgot to format. And it looks like I need an additional guard on the gyp bot.
https://codereview.chromium.org/2393863002/diff/100001/test/inspector/BUILD.gn File test/inspector/BUILD.gn (right): https://codereview.chromium.org/2393863002/diff/100001/test/inspector/BUILD.g... test/inspector/BUILD.gn:8: if (v8_enable_inspector_override) { On 2016/10/11 18:16:23, machenbach (slow) wrote: > We should maybe just guard the whole gyp target like I did for GN here. Though, > if we have the guard at the deps in all.gyp, it's not really required. Only in > GN, every target will be found on ninja's toplevel all target automatically. Sounds good to me. Could you wrap it in this CL? https://codereview.chromium.org/2393863002/diff/120001/gypfiles/all.gyp File gypfiles/all.gyp (right): https://codereview.chromium.org/2393863002/diff/120001/gypfiles/all.gyp#newco... gypfiles/all.gyp:38: '../test/inspector/inspector.gyp:*', please remove it or wrap whole inspector.gyp file and remove line 30.
https://codereview.chromium.org/2393863002/diff/120001/gypfiles/all.gyp File gypfiles/all.gyp (right): https://codereview.chromium.org/2393863002/diff/120001/gypfiles/all.gyp#newco... gypfiles/all.gyp:38: '../test/inspector/inspector.gyp:*', On 2016/10/11 19:05:57, kozyatinskiy wrote: > please remove it or wrap whole inspector.gyp file and remove line 30. Ah - right - rebase didn't remove this line. Maybe when I remove it, wrapping inspector.gyp isn't required then.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/11 21:18:10, kozyatinskiy wrote: > thanks! lgtm lgtm
Landing this now independent of the inspector-on default. It might break again though since we don't test it anywhere.
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kozyatinskiy@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2393863002/#ps160001 (title: "Detach from dependent patchset")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [inspector] Add swarming support to inspector tests BUG=chromium:635948 ========== to ========== [inspector] Add swarming support to inspector tests BUG=chromium:635948 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Add swarming support to inspector tests BUG=chromium:635948 ========== to ========== [inspector] Add swarming support to inspector tests BUG=chromium:635948 Committed: https://crrev.com/3195f19e879c37d8f421defa3a2608e5c15f45df Cr-Commit-Position: refs/heads/master@{#40194} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3195f19e879c37d8f421defa3a2608e5c15f45df Cr-Commit-Position: refs/heads/master@{#40194} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
