Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
64723003:
Remove Win Aura bots from the buildbot and tryservers since they're now redundant as trunk Windows … (Closed)
|
Created:
7 years, 1 month ago by jam Modified:
7 years, 1 month ago CC:
chromium-reviews, cmp-cc_chromium.org, ilevy-cc_chromium.org, xusydoc+watch_chromium.org, kjellander+cc_chromium.org Visibility:
Public. |
DescriptionRemove Win Aura bots from the buildbot and tryservers since they're now redundant as trunk Windows is using Aura.
BUG=316199
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=234145
Patch Set 1 #
Total comments: 12
Patch Set 2 : review comments #Patch Set 3 : take out lkgr change #
Total comments: 1
Patch Set 4 : fix presubmit #
Messages
Total messages: 14 (0 generated)
lgtm https://chromiumcodereview.appspot.com/64723003/diff/1/scripts/tools/lkgr_fin... File scripts/tools/lkgr_finder.py (right): https://chromiumcodereview.appspot.com/64723003/diff/1/scripts/tools/lkgr_fin... scripts/tools/lkgr_finder.py:99: 'ash_unittests', I'm not as familiar with lkgr_finder, so I'd appreciate Paweł's review as well
Hey John, some comments. This is normal when working with buildbot, sorry about that. Pro tip: please run presubmit - I think it should detect some of the issues. If you did run it and it didn't detect, it'd look like a bug. https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... File masters/master.chromium.win/master_win_cfg.py (left): https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... masters/master.chromium.win/master_win_cfg.py:524: B('Win Aura Builder', 'dbg_aura', 'compile|windows', 'win_dbg', This removes bots - I'm pretty sure you'll need to modify slaves.cfg here and in the tryserver. https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... File masters/master.chromium.win/master_win_cfg.py (right): https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... masters/master.chromium.win/master_win_cfg.py:527: build_url=dbg_aura_archive, Please also update this line. With this patch dbg_aura_archive no longer exists. https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... File masters/master.tryserver.chromium/master.cfg (right): https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... masters/master.tryserver.chromium/master.cfg:706: 'keyboard_unittests', I think this needs buildrunner (_br) here. I noticed this is how it used to be before, but it looks suspicious. Maybe a candidate for TODO? I'll let Mike comment on this. https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py File scripts/tools/lkgr_finder.py (right): https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py#... scripts/tools/lkgr_finder.py:99: 'ash_unittests', On 2013/11/07 22:14:48, stip wrote: > I'm not as familiar with lkgr_finder, so I'd appreciate Paweł's review as well Provided the additions are correct, fine. One landing advice though: land removals together with the change, but move additions to a separate CL. The additions should be landed after several cycles of the new bots to avoid holding up LKGR. It can also be tested locally (happy to explain more) before committing live. https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py#... scripts/tools/lkgr_finder.py:145: # 'Interactive Tests (dbg)': [ While you're here, let's also remove this.
https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... File masters/master.tryserver.chromium/master.cfg (right): https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... masters/master.tryserver.chromium/master.cfg:706: 'keyboard_unittests', On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > I think this needs buildrunner (_br) here. > > I noticed this is how it used to be before, but it looks suspicious. Maybe a > candidate for TODO? I'll let Mike comment on this. it doesn't need _br, as it was added as a native buildrunner test
On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > Hey John, some comments. This is normal when working with buildbot, sorry about > that. > > Pro tip: please run presubmit - I think it should detect some of the issues. If > you did run it and it didn't detect, it'd look like a bug. The presubmit checks don't work on Windows. Previously I was told to just let the CQ handle that since they don't run for me. https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... File masters/master.chromium.win/master_win_cfg.py (left): https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... masters/master.chromium.win/master_win_cfg.py:524: B('Win Aura Builder', 'dbg_aura', 'compile|windows', 'win_dbg', On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > This removes bots - I'm pretty sure you'll need to modify slaves.cfg here Thanks, updated slaves.cfg in this directory. > and in the tryserver. Which file exactly do you mean? https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... File masters/master.chromium.win/master_win_cfg.py (right): https://codereview.chromium.org/64723003/diff/1/masters/master.chromium.win/m... masters/master.chromium.win/master_win_cfg.py:527: build_url=dbg_aura_archive, On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > Please also update this line. With this patch dbg_aura_archive no longer exists. Done. https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... File masters/master.tryserver.chromium/master.cfg (right): https://codereview.chromium.org/64723003/diff/1/masters/master.tryserver.chro... masters/master.tryserver.chromium/master.cfg:706: 'keyboard_unittests', (not doing anything here per stip's comment) https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py File scripts/tools/lkgr_finder.py (right): https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py#... scripts/tools/lkgr_finder.py:99: 'ash_unittests', On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > On 2013/11/07 22:14:48, stip wrote: > > I'm not as familiar with lkgr_finder, so I'd appreciate Paweł's review as well > > Provided the additions are correct, fine. > > One landing advice though: land removals together with the change, but move > additions to a separate CL. The additions should be landed after several cycles > of the new bots to avoid holding up LKGR. It can also be tested locally (happy > to explain more) before committing live. Good idea, I'll split off the removals into another cl that I can land later (https://codereview.chromium.org/66063002). I think that'll also help the tricky landing wrt to CQ/try server. https://codereview.chromium.org/64723003/diff/1/scripts/tools/lkgr_finder.py#... scripts/tools/lkgr_finder.py:145: # 'Interactive Tests (dbg)': [ On 2013/11/07 22:22:16, Paweł Hajdan Jr. wrote: > While you're here, let's also remove this. Done.
Could you move lkgr change to a separate CL? Everything else looks good. :)
On 2013/11/08 22:27:40, Paweł Hajdan Jr. wrote: > Could you move lkgr change to a separate CL? Everything else looks good. :) done, thanks
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jam@chromium.org/64723003/160001
Presubmit check for 64723003-160001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
tests/masters_cfg_test.py failed
FAILURE The following master.cfg files did not load:
/b/commit-queue/workdir/tools/build/masters/master.tryserver.chromium
> Traceback (most recent call last):
> File "/b/commit-queue/workdir/tools/build/scripts/slave/runbuild.py", line
403, in <module>
> sys.exit(main())
> File "/b/commit-queue/workdir/tools/build/scripts/slave/runbuild.py", line
389, in main
> retcode = execute(opts)
> File "/b/commit-queue/workdir/tools/build/scripts/slave/runbuild.py", line
283, in execute
> execute_builder(builder, mastername, options)
> File "/b/commit-queue/workdir/tools/build/scripts/slave/runbuild.py", line
325, in execute_builder
> commands = builder_utils.GetCommands(steplist)
> File "/b/commit-queue/workdir/tools/build/scripts/slave/builder_utils.py",
line 303, in GetCommands
> StripBuildrunnerIgnore(step)
> File "/b/commit-queue/workdir/tools/build/scripts/slave/builder_utils.py",
line 289, in StripBuildrunnerIgnore
> step.name.rstrip('_buildrunner_ignore_1'), step.build.builder.name))
> AssertionError: Duplicate buildrunner step views_unittests not allowed in
win_rel
Parsed 51 masters successfully, 1 failed, 1 skipped in 9.2s.
Presubmit checks took 40.1s to calculate.
https://codereview.chromium.org/64723003/diff/160001/masters/master.tryserver... File masters/master.tryserver.chromium/master.cfg (right): https://codereview.chromium.org/64723003/diff/160001/masters/master.tryserver... masters/master.tryserver.chromium/master.cfg:716: 'views_br', remove this line. views_br is already added via unit_br. see line 700 of https://chromium.googlesource.com/chromium/tools/build/+/dfd1ca5287205350f768.... Note that there is a win32 check there, does that still make sense?
On 2013/11/09 01:03:15, stip wrote: > https://codereview.chromium.org/64723003/diff/160001/masters/master.tryserver... > File masters/master.tryserver.chromium/master.cfg (right): > > https://codereview.chromium.org/64723003/diff/160001/masters/master.tryserver... > masters/master.tryserver.chromium/master.cfg:716: 'views_br', > remove this line. views_br is already added via unit_br. see line 700 of > https://chromium.googlesource.com/chromium/tools/build/+/dfd1ca5287205350f768.... > > Note that there is a win32 check there, does that still make sense? Thanks! I didn't know that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jam@chromium.org/64723003/240001
Message was sent while issue was closed.
Change committed as 234145 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
