Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390753005:
Double Builds for Blimp Integration Test Infrastructure on FYI bot (Closed)
|
Created:
4 years, 2 months ago by shenghuazhang Modified:
4 years, 2 months ago CC:
mikecase (-- gone --), chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionDouble Builds for Blimp Integration Test Infrastructure on FYI bot
master.cfg - Add builder 'b_chromium_blimp_android_tester' on chromium.fyi
slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI'
blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration'
BUG=653175
Committed: https://chromium.googlesource.com/chromium/tools/build/+/de7929ec12983a232fcb750c7a6d8045e0ee1046
Patch Set 1 #Patch Set 2 : part of recipe - gn args building #
Total comments: 11
Patch Set 3 : Dirk comment #
Total comments: 1
Patch Set 4 : Dirk comment #
Total comments: 12
Patch Set 5 : recipe config #
Total comments: 1
Patch Set 6 : restore pgo related json #Patch Set 7 : slight change #
Messages
Total messages: 24 (11 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
This is the rough CL for double builds - gn gen args building part. I want to send this out for reviewing, to ensure it's at least in the right track.
https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:114: 'Blimp Android Tester', nit: this name is too similar to "Blimp Android Client". It ought to make clear that this is testing both the client and the engine. https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:1308: b_chromium_blimp_andoird_tester, s/andoird/android/ https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... File masters/master.chromium.fyi/slaves.cfg (right): https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... masters/master.chromium.fyi/slaves.cfg:145: 'hostname': 'slave141-c1', Where was this allocated? https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:58: ANDROID_BUILD = api.path['slave_build'].join('src', 'out-android', 'Debug') ANDROID_BUILD and LINUX_BUILD should not be capitalized https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:61: buildername, bot_config = api.chromium.configure_bot(BUILDERS) After looking at api.chromium.configure_bot, I'm not sure that this will work w/ the configuration split between client_config and engine_config. https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:64: api.chromium.run_mb(mastername="chromium.fyi", nit: separate the android build and the linux build. run mb for linux, then build linux, then run mb for android, then build android. https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:70: api.chromium.compile(targets=['blimp', 'chrome_public_apk'], out_dir=ANDROID_BUILD) 80 chars, please
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
See my comments in https://codereview.chromium.org/2391343002/#msg6 (the mb_config.pyl change) for the way I'd prefer to handle this, MB-wise.
+ This patch set 3 is mainly sent out for reviewing 'pgo/api.py' and related code. - No need to look around 'blimp/integration.py' this time. Sorry about the mess. https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2390753005/diff/20001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:114: 'Blimp Android Tester', On 2016/10/06 13:34:30, jbudorick wrote: > nit: this name is too similar to "Blimp Android Client". It ought to make clear > that this is testing both the client and the engine. Done. https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:58: ANDROID_BUILD = api.path['slave_build'].join('src', 'out-android', 'Debug') On 2016/10/06 13:34:30, jbudorick wrote: > ANDROID_BUILD and LINUX_BUILD should not be capitalized Done. https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:70: api.chromium.compile(targets=['blimp', 'chrome_public_apk'], out_dir=ANDROID_BUILD) On 2016/10/06 13:34:30, jbudorick wrote: > 80 chars, please Done.
Description was changed from ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_andoird_tester' on chromium.fyi slaves.cfg - Add 'Blimp Android Tester' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Android Tester' BUG=653175 ========== to ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_andoird_tester' on chromium.fyi slaves.cfg - Add 'Blimp Android Tester' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Android Tester' BUG=653175 ==========
https://codereview.chromium.org/2390753005/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/pgo/api.py (right): https://codereview.chromium.org/2390753005/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:59: phase='pgo_phase_1') See my comments in https://codereview.chromium.org/2391343002/ about how we can't easily change the phase arg for the official builders, and so we should probably leave these alone as well.
https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/20001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:64: api.chromium.run_mb(mastername="chromium.fyi", On 2016/10/06 13:34:30, jbudorick wrote: > nit: separate the android build and the linux build. run mb for linux, then > build linux, then run mb for android, then build android. Done. https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/pgo/api.py (right): https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:59: phase=0) The 'win_pgo' config keys are set to be '0', '1'. And the phase type defaults to be 'str'. So set the phase '0', '1' instead of '1', '2'. Does this correct? https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:98: phase=1) Relates here
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
Looks great ^_^ https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:941: 'factory': m_remote_run_chromium_src('chromium'), I think you want the recipe arg to be 'blimp/integration' and not 'chromium' 'chromium' is for things that will run with the chromium.py recipe which I think are mostly bots with their configs in the 'scripts/slave/recipe_modules/chromium_tests/' https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:41: 'engine_config': { It doesnt look like these configs are being used. I could be wrong. https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:61: buildername, bot_config = api.chromium.configure_bot(BUILDERS) woah, cool. Didn't know this API existed.
https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:941: 'factory': m_remote_run_chromium_src('chromium'), On 2016/10/10 18:15:53, mikecase wrote: > I think you want the recipe arg to be 'blimp/integration' and not 'chromium' > > 'chromium' is for things that will run with the chromium.py recipe which I think > are mostly bots with their configs in the > 'scripts/slave/recipe_modules/chromium_tests/' Yup, this needs to be 'blimp/integration'. https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/pgo/api.py (right): https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:59: phase=0) On 2016/10/08 00:21:08, shenghuazhang1 wrote: > The 'win_pgo' config keys are set to be '0', '1'. And the phase type defaults to > be 'str'. So set the phase '0', '1' instead of '1', '2'. > Does this correct? Nope; this'd break the old branches, i.e., this module needs to not change at all. Fix this client-side instead as I noted in my comments on the other CL just now. (And sorry for the back-and-forth on this). https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:98: phase=1) On 2016/10/08 00:21:08, shenghuazhang1 wrote: > Relates here same answer :). https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:41: 'engine_config': { On 2016/10/10 18:15:53, mikecase wrote: > It doesnt look like these configs are being used. I could be wrong. Right, I don't think you can nest configs like this. Just use the client_config values, as any android checkout can also do a linux build.
Description was changed from ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_andoird_tester' on chromium.fyi slaves.cfg - Add 'Blimp Android Tester' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Android Tester' BUG=653175 ========== to ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_andoird_tester' on chromium.fyi slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration' BUG=653175 ==========
https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2390753005/diff/60001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:941: 'factory': m_remote_run_chromium_src('chromium'), On 2016/10/11 00:18:23, Dirk Pranke wrote: > On 2016/10/10 18:15:53, mikecase wrote: > > I think you want the recipe arg to be 'blimp/integration' and not 'chromium' > > > > 'chromium' is for things that will run with the chromium.py recipe which I > think > > are mostly bots with their configs in the > > 'scripts/slave/recipe_modules/chromium_tests/' > > Yup, this needs to be 'blimp/integration'. Done. https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/pgo/api.py (right): https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:59: phase=0) On 2016/10/11 00:18:23, Dirk Pranke wrote: > On 2016/10/08 00:21:08, shenghuazhang1 wrote: > > The 'win_pgo' config keys are set to be '0', '1'. And the phase type defaults > to > > be 'str'. So set the phase '0', '1' instead of '1', '2'. > > Does this correct? > > Nope; this'd break the old branches, i.e., this module needs to not change at > all. > > Fix this client-side instead as I noted in my comments on the other CL just now. > > (And sorry for the back-and-forth on this). Oh yeah I see. This is very clear, I will change it back to the initial way. Thanks for the explanation :) https://codereview.chromium.org/2390753005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/pgo/api.py:98: phase=1) On 2016/10/11 00:18:23, Dirk Pranke wrote: > On 2016/10/08 00:21:08, shenghuazhang1 wrote: > > Relates here > > same answer :). Done. https://codereview.chromium.org/2390753005/diff/80001/scripts/slave/recipes/b... File scripts/slave/recipes/blimp/integration.py (right): https://codereview.chromium.org/2390753005/diff/80001/scripts/slave/recipes/b... scripts/slave/recipes/blimp/integration.py:48: api.chromium_android.configure_from_properties( For the chromium android recipe config here, I copy/paste the way that chromedriver.py uses: https://codesearch.chromium.org/chromium/build/scripts/slave/recipes/chromedr...
Description was changed from ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_andoird_tester' on chromium.fyi slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration' BUG=653175 ========== to ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_android_tester' on chromium.fyi slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration' BUG=653175 ==========
lgtm
The CQ bit was checked by shenghuazhang@chromium.org
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 shenghuazhang@chromium.org
The CQ bit was checked by shenghuazhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2390753005/#ps120001 (title: "slight change")
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 ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_android_tester' on chromium.fyi slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration' BUG=653175 ========== to ========== Double Builds for Blimp Integration Test Infrastructure on FYI bot master.cfg - Add builder 'b_chromium_blimp_android_tester' on chromium.fyi slaves.cfg - Add 'Blimp Client Engine Integration' for master 'ChromiumFYI' blimp/integration.py (new created directory 'scripts/slave/recipes/blimp') - recipe with running steps for 'Blimp Client Engine Integration' BUG=653175 Committed: https://chromium.googlesource.com/chromium/tools/build/+/de7929ec12983a232fcb... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/tools/build/+/de7929ec12983a232fcb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
