Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223893002:
Add Linux bots to mb_config.pyl. (Closed)
|
Created:
4 years, 4 months ago by ehmaldonado_webrtc Modified:
4 years, 4 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd Linux bots to mb_config.pyl.
BUG=589510
NOTRY=True
Patch Set 1 #Patch Set 2 : Rebase CL. #
Total comments: 6
Patch Set 3 : Add openh264 to libfuzzer. #Patch Set 4 : Adressed some comments. #Patch Set 5 : Disable goma for iOS. #
Total comments: 2
Messages
Total messages: 19 (7 generated)
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Can you rebase this as the other CL has now landed? It'll make review easier.
Description was changed from ========== Add Linux bots to mb_config.pyl. BUG=589510 ========== to ========== Add Linux bots to mb_config.pyl. BUG=589510 ==========
On 2016/08/08 14:01:15, kjellander_webrtc wrote: > Can you rebase this as the other CL has now landed? It'll make review easier. How can I do that?
On 2016/08/08 14:06:03, ehmaldonado_webrtc wrote: > On 2016/08/08 14:01:15, kjellander_webrtc wrote: > > Can you rebase this as the other CL has now landed? It'll make review easier. > > How can I do that? Done. I ran 'cl upload origin/master'
This is a good start, just some minor changes needed. https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.pyl File webrtc/build/mb_config.pyl (right): https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:33: 'Linux32 ARM': 'linux_gyp_release_bot_arm', Let's add tryserver.webrtc entries for these too https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:94: 'linux_gyp_release_bot_arm': [ Let's try to follow the conventions in Chromium's mb_config.pyl as much as possible: 1. Don't use platform names in the config names unless necessary (iOS above is one such exception but Linux/Mac/Windows can probably be the same) 2. The ordering convention seems to be to put {debug,release}_bot at the end in the name and the list of configs. https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:179: 'disable_nacl': { Remove disable_nacl since it won't have any effect for a WebRTC stand-alone build. https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:221: 'mixins': ['release', 'static'], Let's add 'goma' too, so we can keep having distributed compilation for our bots: https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl?rcl=0&l=2335 https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:223: For the tryserver configs, add a 'release_trybot': { 'mixins': ['release_bot', 'dcheck_always_on'], }, Similar for debug.
On 2016/08/08 14:47:08, kjellander_webrtc wrote: > This is a good start, just some minor changes needed. > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.pyl > File webrtc/build/mb_config.pyl (right): > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:33: 'Linux32 ARM': 'linux_gyp_release_bot_arm', > Let's add tryserver.webrtc entries for these too > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:94: 'linux_gyp_release_bot_arm': [ > Let's try to follow the conventions in Chromium's mb_config.pyl as much as > possible: > 1. Don't use platform names in the config names unless necessary (iOS above is > one such exception but Linux/Mac/Windows can probably be the same) > 2. The ordering convention seems to be to put {debug,release}_bot at the end in > the name and the list of configs. > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:179: 'disable_nacl': { > Remove disable_nacl since it won't have any effect for a WebRTC stand-alone > build. > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:221: 'mixins': ['release', 'static'], > Let's add 'goma' too, so we can keep having distributed compilation for our > bots: https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl?rcl=0&l=2335 > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:223: > For the tryserver configs, add a > 'release_trybot': { > 'mixins': ['release_bot', 'dcheck_always_on'], > }, > > Similar for debug. Done. PTAL.
Description was changed from ========== Add Linux bots to mb_config.pyl. BUG=589510 ========== to ========== Add Linux bots to mb_config.pyl. BUG=589510 NOTRY=True ==========
lgtm, I added NOTRY=True for you and fired off the iOS trybots (but they're actually not using the MB config I believe, see separate e-mail I sent to smut@). https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.pyl File webrtc/build/mb_config.pyl (right): https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:94: 'linux_gyp_release_bot_arm': [ On 2016/08/08 14:47:08, kjellander_webrtc wrote: > Let's try to follow the conventions in Chromium's mb_config.pyl as much as > possible: > 1. Don't use platform names in the config names unless necessary (iOS above is > one such exception but Linux/Mac/Windows can probably be the same) > 2. The ordering convention seems to be to put {debug,release}_bot at the end in > the name and the list of configs. I didn't get this right - when there is an architecture in the Chromium config, it's at the end. So your CL is correct and my comment was wrong :-o See https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl?rcl=0&l=964 for examples.
On 2016/08/08 19:35:30, kjellander_webrtc wrote: > lgtm, I added NOTRY=True for you and fired off the iOS trybots (but they're > actually not using the MB config I believe, see separate e-mail I sent to > smut@). I stand corrected - it seems they actually are using the MB config, since they're failing on Goma not being started (see discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=617541 - we could actually extend the ios recipe module now to start and stop goma, since they made a recipe module for Goma with those capabilities now: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/goma/api.py). For this CL - let's just exclude Goma for iOS to unblock progress. > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.pyl > File webrtc/build/mb_config.pyl (right): > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > webrtc/build/mb_config.pyl:94: 'linux_gyp_release_bot_arm': [ > On 2016/08/08 14:47:08, kjellander_webrtc wrote: > > Let's try to follow the conventions in Chromium's mb_config.pyl as much as > > possible: > > 1. Don't use platform names in the config names unless necessary (iOS above is > > one such exception but Linux/Mac/Windows can probably be the same) > > 2. The ordering convention seems to be to put {debug,release}_bot at the end > in > > the name and the list of configs. > > I didn't get this right - when there is an architecture in the Chromium config, > it's at the end. So your CL is correct and my comment was wrong :-o > See https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl?rcl=0&l=964 for > examples.
On 2016/08/08 19:40:13, kjellander_webrtc wrote: > On 2016/08/08 19:35:30, kjellander_webrtc wrote: > > lgtm, I added NOTRY=True for you and fired off the iOS trybots (but they're > > actually not using the MB config I believe, see separate e-mail I sent to > > smut@). > > I stand corrected - it seems they actually are using the MB config, since > they're failing on Goma not being started (see discussion in > https://bugs.chromium.org/p/chromium/issues/detail?id=617541 - we could actually > extend the ios recipe module now to start and stop goma, since they made a > recipe module for Goma with those capabilities now: > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/goma/api.py). > > For this CL - let's just exclude Goma for iOS to unblock progress. > > > > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.pyl > > File webrtc/build/mb_config.pyl (right): > > > > > https://codereview.webrtc.org/2223893002/diff/20001/webrtc/build/mb_config.py... > > webrtc/build/mb_config.pyl:94: 'linux_gyp_release_bot_arm': [ > > On 2016/08/08 14:47:08, kjellander_webrtc wrote: > > > Let's try to follow the conventions in Chromium's mb_config.pyl as much as > > > possible: > > > 1. Don't use platform names in the config names unless necessary (iOS above > is > > > one such exception but Linux/Mac/Windows can probably be the same) > > > 2. The ordering convention seems to be to put {debug,release}_bot at the end > > in > > > the name and the list of configs. > > > > I didn't get this right - when there is an architecture in the Chromium > config, > > it's at the end. So your CL is correct and my comment was wrong :-o > > See https://cs.chromium.org/chromium/src/tools/mb/mb_config.pyl?rcl=0&l=964 > for > > examples. Ok. So, do I leave the configs names as they are?
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2223893002/#ps80001 (title: "Disable goma for iOS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add Linux bots to mb_config.pyl. BUG=589510 NOTRY=True ========== to ========== Add Linux bots to mb_config.pyl. BUG=589510 NOTRY=True Committed: https://crrev.com/09abaa0c7dcd53e3883c855fe3e6ee189be5c551 Cr-Commit-Position: refs/heads/master@{#13689} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/09abaa0c7dcd53e3883c855fe3e6ee189be5c551 Cr-Commit-Position: refs/heads/master@{#13689}
Message was sent while issue was closed.
Description was changed from ========== Add Linux bots to mb_config.pyl. BUG=589510 NOTRY=True Committed: https://crrev.com/09abaa0c7dcd53e3883c855fe3e6ee189be5c551 Cr-Commit-Position: refs/heads/master@{#13689} ========== to ========== Add Linux bots to mb_config.pyl. BUG=589510 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
I found an error and a redundancy. Can you fix in a follow-up CL? Sorry for not finding in initial review. https://codereview.webrtc.org/2223893002/diff/80001/webrtc/build/mb_config.pyl File webrtc/build/mb_config.pyl (right): https://codereview.webrtc.org/2223893002/diff/80001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:47: 'Linux64 Release (Libfuzzer)': Let's remove the libfuzzer bot config since it's already using GN (libfuzzer only works with GN). https://codereview.webrtc.org/2223893002/diff/80001/webrtc/build/mb_config.py... webrtc/build/mb_config.pyl:74: 'linux_ubsan_clang_vptr': 'gyp_ubsan_vptr_clang_release_trybot_x64', I missed this during review: this name should be 'linux_ubsan_vptr' Can you fix in a follow up-CL? |
