Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071793002:
Add linux64 mb configs. (Closed)
|
Created:
4 years, 6 months ago by Michael Achenbach Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAdd linux64 mb configs.
Depends on https://codereview.chromium.org/2071583003/ to
skip redundant clang=1 configs.
BUG=chromium:616035
NOTRY=true
Committed: https://crrev.com/9347cae9982732422df4823d0b2e9e13b2661731
Cr-Commit-Position: refs/heads/master@{#37050}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix slow dcheck default #
Total comments: 5
Messages
Total messages: 19 (8 generated)
Description was changed from ========== Add linux64 mb configs. BUG= ========== to ========== Add linux64 mb configs. BUG=616035 ==========
PTAL. This prepares for switching these bots to MB. The switch flag is flipped on the infra side in a follow up. Please no rubber stamping on this CL. One tiny misconfiguration changes the behavior of a whole bot and might go by undetected (e.g. switching static to shared lib by mistake). I'll also do a post-review after switching to MB. Then we can compare the infra-side gyp_defines of the runhooks step in prod with the client-side gyp_defines of the mb step. PS. I've added a bunch of comments in patch 1 to ease review. Needed to add a tiny fix in patch 2 though and am too lazy to move all the comments. https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:12: # Linux64. x64 bots in prod: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20builder/bu... https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20debug%20bu... https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20custom%20s... https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20internal%2... https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:20: 'tryserver.v8': { Trybots in prod: https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/7... https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/bui... https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_dbg/builds... https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:31: 'gyp', 'debug_bot', 'swarming', 'valgrind', 'x64'], The debug bot in prod has also jsfunfuzz=1. This is not needed by gyp but by one of the other download hooks. Therefore, it needs to stay as an infra-side gyp_defines, but is not needed on the client-side for mb. https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:44: 'mixins': { Some mixins ported from: https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:120: # This is the default in gn for debug. See: https://cs.chromium.org/chromium/src/v8/gni/v8.gni?l=9 https://cs.chromium.org/chromium/src/v8/gni/v8.gni?l=52 https://codereview.chromium.org/2071793002/diff/1/infra/mb/mb_config.pyl#newc... infra/mb/mb_config.pyl:127: 'gyp_defines': 'embed_script=../test/mjsunit/mjsunit.js', This used to be an absolute path on the bots, precalculated by recipes. Here we can't use absolute paths. Tested locally that this works with a path relative to src. Doesn't matter in which dir gyp is called...
machenbach@chromium.org changed reviewers: + jochen@chromium.org, kjellander@chromium.org, vogelheim@chromium.org
PTAL. See comments of previous message. Forgot to add reviewers there.
Description was changed from ========== Add linux64 mb configs. BUG=616035 ========== to ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=616035 ==========
What about the v8_target_arch variable? https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:57: 'debug', 'static', 'goma', 'v8_enable_slow_dchecks', I thought the default way was static for Release and shared for Debug, but maybe that's just Chrome (we use that too in WebRTC). https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:109: # TODO(machenbach): Add this to gn. What do you mean with this TODO?
On 2016/06/16 12:45:38, kjellander_chromium wrote: > What about the v8_target_arch variable? Well spotted. Forgot to comment about it. I intend to skip it where it's equal to target_arch as the configs are redundant. See: https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=static+fil... https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:57: 'debug', 'static', 'goma', 'v8_enable_slow_dchecks', On 2016/06/16 12:45:38, kjellander_chromium wrote: > I thought the default way was static for Release and shared for Debug, but maybe > that's just Chrome (we use that too in WebRTC). We use static everywhere unless specified otherwise: https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=static+fil... There is not much link performance gain for v8 and all test executables are statically linked anyways. https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:109: # TODO(machenbach): Add this to gn. On 2016/06/16 12:45:38, kjellander_chromium wrote: > What do you mean with this TODO? There is no has_valgrind gn arg yet. We'll need it as soon as the bot switches to gn. I'll add it probably in the CL that switches the bot.
Description was changed from ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=616035 ========== to ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 ==========
Description was changed from ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 ========== to ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 NOTRY=true ==========
lgtm https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl File infra/mb/mb_config.pyl (right): https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... infra/mb/mb_config.pyl:109: # TODO(machenbach): Add this to gn. On 2016/06/16 12:53:13, Michael Achenbach wrote: > On 2016/06/16 12:45:38, kjellander_chromium wrote: > > What do you mean with this TODO? > > There is no has_valgrind gn arg yet. We'll need it as soon as the bot switches > to gn. I'll add it probably in the CL that switches the bot. OK. Be aware that GN will throw an error if you try to specify a variable that doesn't exist (unlike GYP).
On 2016/06/17 07:22:37, kjellander_chromium wrote: > lgtm > > https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl > File infra/mb/mb_config.pyl (right): > > https://codereview.chromium.org/2071793002/diff/20001/infra/mb/mb_config.pyl#... > infra/mb/mb_config.pyl:109: # TODO(machenbach): Add this to gn. > On 2016/06/16 12:53:13, Michael Achenbach wrote: > > On 2016/06/16 12:45:38, kjellander_chromium wrote: > > > What do you mean with this TODO? > > > > There is no has_valgrind gn arg yet. We'll need it as soon as the bot switches > > to gn. I'll add it probably in the CL that switches the bot. > > OK. Be aware that GN will throw an error if you try to specify a variable that > doesn't exist (unlike GYP). Sure. Now all bots are still on gyp. Will first start with gn flips once all mb flips are done...
I'll land this now - but a second pair of eyes is still welcome. I'll also make a post review of what is in prod.
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071793002/20001
Message was sent while issue was closed.
Description was changed from ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 NOTRY=true ========== to ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 NOTRY=true ========== to ========== Add linux64 mb configs. Depends on https://codereview.chromium.org/2071583003/ to skip redundant clang=1 configs. BUG=chromium:616035 NOTRY=true Committed: https://crrev.com/9347cae9982732422df4823d0b2e9e13b2661731 Cr-Commit-Position: refs/heads/master@{#37050} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9347cae9982732422df4823d0b2e9e13b2661731 Cr-Commit-Position: refs/heads/master@{#37050}
Message was sent while issue was closed.
Follow up fix: https://codereview.chromium.org/2072833004/ |
