Rework how bots get the version of Blink.

Rework how bots get the version of Blink.

The current buildbot code uses a number of hacks to try and get the
correct version of Blink, and has a number of bugs. This patch completely
reworks things.

We will now set a 'blink_config' factory property to determine which
version of blink to use, and pass that to the GClient object. If 'blink_config'
is set to 'chromium' (or None, or anything other than 'blink'), we'll use the
stock gclient spec and the version of Blink as set in the DEPS file.

If 'blink_config' is set to 'blink', then we will use a custom version
of Blink. If the patch contains a "trunk" branch, then we use the revision
in the patch (as the change came from the Blink poller). Otherwise,
we will set the custom "webkit_revision" gclient variable to "", which
is equivalent to HEAD (meaning we'll get the tip-of-tree for Blink).
This replaces the way we used "WK_REV" previously.

The code previously also supported a means to specify a fixed version of
Blink via the revision specified in the patch as uploaded through the
svn poller to Rietveld (in trychange and git-wktry). This code path was
also a hack and we're currently discouraging git-wktry altogether, so
I'm removing this support for now (although I think trychange should still
work properly).

A lot of trouble was caused by us using "inheritance" in different
ChromiumWebKit* helper functions in ChromiumFactory. We should've been using
a parameter instead. This change removes all of the *ChromiumWebKit*
methods and updates all of the bot configs accordingly to use the
"blink_config" parameter as described above.

The net result of all of this is that a bot will now be either a "Blink
tip of tree" bot, or not, and that is independent of the patch being
tried or applied.

R=szager@chromium.org, iannucci@chromium.org
BUG=233412
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=196054

[iannucci, 2013-04-24 02:36:46]

https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
File scripts/master/chromium_step.py (right):

https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
scripts/master/chromium_step.py:71: # should figure out how to generalize this
to sub-repos somehow.
This could conceivably be a {custom_var: value} dictionary. Then you could
represent:
  Deps version -> no entry
  HEAD version -> var: 'HEAD'
  LKGR version -> var: rev

This would require reworking how gclient_spec is passed around (it's passed
around as a raw string, which makes manipulating it at the point where we
actually run the gclient commands difficult). Alternately, it may be possible to
compute all this at the point where the current gclient_spec is stringified
(edit: Looks like not so much. This is because the gclient_spec belongs to the
_Factory_ and not to the Build. That would have to be altered (so that the
gclient spec was computed in the factory method instead of the factory
constructor, or allowing overrides to be passed to
BuildGclientSpec()/GetSpec())).

That said, it would be a larger change, and would be better to do as a follow-on
patch than in this CL.

https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
scripts/master/chromium_step.py:153:
'"custom_vars":{"webkit_revision":"%s","webkit_trunk"' % wk_revision)
Hm... this would break in mysterious ways if there were ever any custom_vars
which sorted before webkit_trunk...

[Dirk Pranke, 2013-04-24 02:55:37]

On 2013/04/24 02:36:46, iannucci wrote:
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> File scripts/master/chromium_step.py (right):
> 
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> scripts/master/chromium_step.py:71: # should figure out how to generalize this
> to sub-repos somehow.
> This could conceivably be a {custom_var: value} dictionary. Then you could
> represent:
>   Deps version -> no entry
>   HEAD version -> var: 'HEAD'
>   LKGR version -> var: rev
> 
> This would require reworking how gclient_spec is passed around (it's passed
> around as a raw string, which makes manipulating it at the point where we
> actually run the gclient commands difficult). Alternately, it may be possible
to
> compute all this at the point where the current gclient_spec is stringified
> (edit: Looks like not so much. This is because the gclient_spec belongs to the
> _Factory_ and not to the Build. That would have to be altered (so that the
> gclient spec was computed in the factory method instead of the factory
> constructor, or allowing overrides to be passed to
> BuildGclientSpec()/GetSpec())).
> 

Yup.

> That said, it would be a larger change, and would be better to do as a
follow-on
> patch than in this CL.
>

Yup. This patch changes enough as it is.
 
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> scripts/master/chromium_step.py:153:
> '"custom_vars":{"webkit_revision":"%s","webkit_trunk"' % wk_revision)
> Hm... this would break in mysterious ways if there were ever any custom_vars
> which sorted before webkit_trunk...

Well, yeah, except that we're not actually sorting keys now, we already are
hard-coding "custom_vars":{"webkit_trunk" ...

I think we need to fix this as part of the other non-stringifying changes
discussed above, which I think we'll probably want to do (or need to do) to get
a blink lkgr configuration to work in a non-insane way.

That said, if you have a particularly better idea for how to insert the
webkit_revision field here, I'm open to it. As discussed in person, I'm trying
to move away from the string interpolation that we had before.

[iannucci, 2013-04-24 04:16:30]

On 2013/04/24 02:55:37, Dirk Pranke wrote:
> On 2013/04/24 02:36:46, iannucci wrote:
> >
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> > File scripts/master/chromium_step.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> > scripts/master/chromium_step.py:71: # should figure out how to generalize
this
> > to sub-repos somehow.
> > This could conceivably be a {custom_var: value} dictionary. Then you could
> > represent:
> >   Deps version -> no entry
> >   HEAD version -> var: 'HEAD'
> >   LKGR version -> var: rev
> > 
> > This would require reworking how gclient_spec is passed around (it's passed
> > around as a raw string, which makes manipulating it at the point where we
> > actually run the gclient commands difficult). Alternately, it may be
possible
> to
> > compute all this at the point where the current gclient_spec is stringified
> > (edit: Looks like not so much. This is because the gclient_spec belongs to
the
> > _Factory_ and not to the Build. That would have to be altered (so that the
> > gclient spec was computed in the factory method instead of the factory
> > constructor, or allowing overrides to be passed to
> > BuildGclientSpec()/GetSpec())).
> > 
> 
> Yup.
> 
> > That said, it would be a larger change, and would be better to do as a
> follow-on
> > patch than in this CL.
> >
> 
> Yup. This patch changes enough as it is.
>  
> >
>
https://chromiumcodereview.appspot.com/14440005/diff/4001/scripts/master/chro...
> > scripts/master/chromium_step.py:153:
> > '"custom_vars":{"webkit_revision":"%s","webkit_trunk"' % wk_revision)
> > Hm... this would break in mysterious ways if there were ever any custom_vars
> > which sorted before webkit_trunk...
> 
> Well, yeah, except that we're not actually sorting keys now, we already are
> hard-coding "custom_vars":{"webkit_trunk" ...
> 
> I think we need to fix this as part of the other non-stringifying changes
> discussed above, which I think we'll probably want to do (or need to do) to
get
> a blink lkgr configuration to work in a non-insane way.
> 
> That said, if you have a particularly better idea for how to insert the
> webkit_revision field here, I'm open to it. As discussed in person, I'm trying
> to move away from the string interpolation that we had before.

I would even be more comfortable finding '"webkit_trunk"' and replacing that...
I'm worried about webkit_trunk appearing as something other than the first item,
and I guess I'm not seeing where we're currently replacing based on
'"custom_vars":{"webkit_trunk"'

[Dirk Pranke, 2013-04-24 04:55:56]

ok. please take another look?

[iannucci, 2013-04-24 05:32:40]

lgtm

[commit-bot: I haz the power, 2013-04-24 06:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/14440005/11001

[commit-bot: I haz the power, 2013-04-24 06:22:22]

Presubmit check for 14440005-11001 failed and returned exit status 1.

INFO:root:Found 22 file(s).
INFO:PRESUBMIT:Running pylint on 323 files
Running presubmit commit checks ...
Running /b/commit-queue/workdir/tools/build/PRESUBMIT.py
Running test/slaves_cfg_test.py
Running test/masters_test.py
Running test/tryserver_mail_test.py
Running test/masters_cfg_test.py
Running scripts/master/unittests/failures_notifier_test.py
Running scripts/master/unittests/gatekeeper_test.py
Running scripts/master/unittests/failures_history_test.py
Running scripts/master/unittests/chromium_utils_test.py
Running scripts/master/unittests/try_job_base_test.py
Running scripts/master/unittests/chromium_notifier_test.py
Running scripts/master/unittests/perf_count_notifier_test.py
Running scripts/master/unittests/gclient_source_test.py
Running scripts/master/unittests/process_log_test.py
Running scripts/master/unittests/chromium_commands_test.py
Running scripts/master/unittests/annotator_test.py
Running scripts/slave/unittests/get_swarm_results_test.py
Running scripts/slave/unittests/runisolatedtest_test.py
Running scripts/slave/unittests/extract_build_test.py
Running scripts/slave/unittests/recipe_util_test.py
Running scripts/slave/unittests/telemetry_test.py
Running scripts/slave/unittests/buildrunner_test.py
Running scripts/slave/unittests/runtest_annotator_test.py
Running scripts/slave/unittests/cleanup_old_files_test.py
Running scripts/slave/unittests/results_dashboard_test.py
Running scripts/common/unittests/gtest_utils_test.py
Running scripts/common/unittests/annotator_test.py

** Presubmit ERRORS **
test/masters_test.py failed
1 masters failed:
master.devtools
48 masters succeeded, 1 failed, 12 skipped in 8.5s.

=== Error running master.devtools === 
2013-04-23 23:22:07-0700 [-] Applying patch for
http://twistedmatrix.com/trac/ticket/5079
2013-04-23 23:22:07-0700 [-] Creating BuildMaster -- buildbot.version: 0.8.4p1
2013-04-23 23:22:07-0700 [-] loading configuration from
/b/commit-queue/workdir/tools/build/masters/master.devtools/master.cfg
2013-04-23 23:22:07-0700 [-] configuration update started
2013-04-23 23:22:07-0700 [-] error while parsing config file
2013-04-23 23:22:07-0700 [-] Unhandled Error
	Traceback (most recent call last):
	  File
"/b/commit-queue/workdir/tools/build/third_party/buildbot_8_4p1/buildbot/master.py",
line 248, in loadTheConfigFile
	    d = self.loadConfig(f)
	  File
"/b/commit-queue/workdir/tools/build/third_party/buildbot_8_4p1/buildbot/master.py",
line 602, in loadConfig
	    d.addCallback(do_load)
	  File
"/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/internet/defer.py",
line 297, in addCallback
	    callbackKeywords=kw)
	  File
"/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/internet/defer.py",
line 286, in addCallbacks
	    self._runCallbacks()
	--- <exception caught here> ---
	  File
"/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/internet/defer.py",
line 542, in _runCallbacks
	    current.result = callback(current.result, *args, **kw)
	  File
"/b/commit-queue/workdir/tools/build/third_party/buildbot_8_4p1/buildbot/master.py",
line 277, in do_load
	    exec f in localDict
	exceptions.SyntaxError: invalid syntax (master.cfg, line 71)
	



test/masters_cfg_test.py failed
error while parsing
/b/commit-queue/workdir/tools/build/masters/master.devtools/master.cfg: 
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/build/scripts/common/master_cfg_utils.py",
line 76, in LoadConfig
    localdict = ExecuteConfig(canonical_config)
  File "/b/commit-queue/workdir/tools/build/scripts/common/master_cfg_utils.py",
line 50, in ExecuteConfig
    exec f in localDict
  File "/b/commit-queue/workdir/tools/build/masters/master.devtools/master.cfg",
line 71
    'blink_config': 'blink',
                 ^
SyntaxError: invalid syntax

build error encountered! aborting build
perf_id: chromium-dbg-linux-try
perf_subdir: linux-try-debug
url: http://build.chromium.org/buildbot/coverage/linux-try-debug/total_coverage
perf_id: chromium-dbg-linux
perf_subdir: linux-debug
url: http://build.chromium.org/buildbot/coverage/linux-debug/total_coverage
Skipping chromium.swarm, fix and enable in masters_cfg_test.py!
Skipping chromium.chromebot, fix and enable in masters_cfg_test.py!
Skipping chromiumos.tryserver, fix and enable in masters_cfg_test.py!
Skipping client.nacl.chrome, fix and enable in masters_cfg_test.py!
The following master.cfgs did not load:
  devtools: /b/commit-queue/workdir/tools/build/masters/master.devtools


Presubmit checks took 51.0s to calculate.

[commit-bot: I haz the power, 2013-04-24 06:25:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/14440005/18001

[commit-bot: I haz the power, 2013-04-24 06:26:47]

Change committed as 196054