Add GN (the build tool) to the chromium.linux buildbots and try jobs

scripts/slave/recipes/chromium_trybot.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 DEPS = [
   'chromium',
   'gclient',
   'itertools',
   'json',
   'path',
@@ skipping 225 matching lines @@
     yield api.gclient.checkout(revert=True)
   yield (
     api.rietveld.apply_issue(),
     api.json.read(
       'read test spec',
       api.path['checkout'].join('testing', 'buildbot', 'chromium_trybot.json'),
       step_test_data=lambda: api.json.test_api.output([])),
     api.chromium.runhooks(),
   )
 
+  # TODO(dpranke): crbug.com/353690. Remove the gn-specific steps from this
+  # recipe and stand up a dedicated GN bot when the GN steps take up enough
+  # resources to be worth it. For now, we run GN and generate files into a new
+  # Debug_gn / build in that dir, and run gn_unittests from that dir (w/o
+  # uploading any test data or parsing the annotations).
+  gn_output_dir = api.path['slave_build'].join(api.chromium.c.BUILD_CONFIG +
+                                               '_gn')
+  should_run_gn = api.properties.get('buildername') in ('linux', 'linux_rel')

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

This recipe is not running on any of these builders. This should probably be
linux_chromium_rel.

Actually, given the questions about its stability, can we create dedicated
tryserver builders for this? They can still use the large pool of Linux slaves.

[Dirk Pranke, 2014/03/26 16:52:28]

Right! Will fix.
No, we can't. That's the whole point of this ... we can't currently afford to
double the load of the try jobs (twice the checkout space in particular) for
something that takes less than a minute to run the new steps.

Also, as Robbie pointed out earlier, since we're building into a new builddir,
we can't reuse the pool of slaves either.
 

+
   test_spec = api.step_history['read test spec'].json.output
   test_spec = [s.encode('utf-8') for s in test_spec]
 
   tests = []
   tests.append(CheckdepsTest())
   tests.append(Deps2GitTest())
   for name in GTEST_TESTS + test_spec:
     tests.append(GTestTest(name))
   tests.append(NaclIntegrationTest())
 
   compile_targets = list(api.itertools.chain(
                              *[t.compile_targets() for t in tests]))
   yield api.chromium.compile(compile_targets,
                              name='compile (with patch)',
                              abort_on_failure=False,
                              can_fail_build=False)
-  if api.step_history['compile (with patch)'].retcode != 0:
+  if should_run_gn:
+    yield api.chromium.run_gn(gn_output_dir)
+    yield api.chromium.compile_with_ninja('compile (gn with patch)',

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

Shouldn't this _replace_ api.chromium.compile for GN bots?

[Dirk Pranke, 2014/03/26 16:52:28]

No. This and other comments make me think that you didn't understand what I'm
doing in this patch. The try job needs to do *two* compiles, the normal one and
the gn one, into two different directories.

+                                          gn_output_dir)

[iannucci, 2014/03/26 09:38:49]

we may want to set 'can_fail_build=False, abort_on_failure=False' for these,
until we're sure they should be green (maybe we're sure now?) Same for the clone
of these steps below.

[Dirk Pranke, 2014/03/26 16:52:28]

Good point, will do.

[Dirk Pranke, 2014/03/26 19:16:35]

Oh, I misread this. No, they should be green.

+
+  if ((api.step_history['compile (with patch)'].retcode != 0) or
+      (should_run_gn and
+       api.step_history['compile (gn with patch)'].retcode != 0)):
     # Only use LKCR when compile fails. Note that requested specific revision
     # can still override this.
     api.gclient.set_config('chromium_lkcr')
 
     # Since we're likely to switch to an earlier revision, revert the patch,
     # sync with the new config, and apply issue again.
     yield api.gclient.checkout(revert=True)
     yield (
       api.rietveld.apply_issue(),
       api.chromium.compile(compile_targets,
                            name='compile (with patch, lkcr, clobber)',
                            force_clobber=True)
     )
 
+    if should_run_gn:
+      yield api.path.rmcontents('slave gn build directory', gn_output_dir)

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

Instead of this, could you add force_clobber to the api.chromium methods you're
adding?

This would be consistent with api.chromium.compile, and maybe run_gn and
compile_with_ninja can be just one method.

[Dirk Pranke, 2014/03/26 16:52:28]

Nope :(. force_clobber doesn't support custom output dirs, and I don't think we
should add the param to that method (and the others) for something that should
be a relatively-short-lived thing.
See above. 

It's also not clear to me if run_gn and compile_with_ninja should even be one
step at this point, since we will definitely want to see the failures
differently in the job.

+      yield api.chromium.run_gn(gn_output_dir)
+      yield api.chromium.compile_with_ninja(
+          'compile (gn with patch, lkcr, clobber)', gn_output_dir)
+
   # Do not run tests if the build is already in a failed state.
   if api.step_history.failed:
     return
 
   if recipe_config['compile_only']:
     return
 
+  # TODO(dpranke): crbug.com/353690. We can't use runtest() for gn_unittests
+  # on the trybots because of the different output directory; this means
+  # we don't get annotations and don't get retry of the tests for free :( .
+  # For now we'll hope that this is short-lived until we get dedicated GN
+  # trybots, and assume that making runtest() work w/ a differe output dir

[iannucci, 2014/03/26 09:38:49]

*different

Though it would be /best/ if we could avoid runtest.py entirely and just
implement the bits and pieces which are actually necessary. I'm assuming the
annotations you're referring to are the test case summaries in step_text?

[Dirk Pranke, 2014/03/26 16:52:28]

Yes, and I agree that we should probably try to bypass runtest completely and
just get the annotations.

+  # is not worth it.
+  if should_run_gn:
+    yield api.step(name='gn_unittests',
+                   cmd=[gn_output_dir.join('gn_unittests',

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

Sorry, all new code added here MUST support retries without patch.

We've had to add pre-existing telemetry tests below, and they're causing
annoying flakes, even though they run on the main waterfall.

[Dirk Pranke, 2014/03/26 16:52:28]

I forgot to add a TODO -- and will do so -- but I thought we discussed this
yesterday and agreed that adding a TODO a la the telemetry tests would be good
enough?

There's very few gn_unittests, almost no one will be working on them, and they
complete in less than a second. They should not be flaky, I don't think it will
be easy to distinguish a flaky failure from a patch-induced failure, and I will
be perfectly fine w/ flakes being considered failures for now (and I believe
Brett will be, too).

I don't want to start out assuming they will be flaky and writing extra code to
handle it, especially considering that we can't use the normal retry logic for
this (another good reason to make this part of its own recipe).

The analogy to telemetry isn't very good; telemetry is a far more complex set of
tests than gn_unittests.

How does that sound?

[Dirk Pranke, 2014/03/26 19:16:35]

Actually, per further discussion w/ Robbie, I'm going to remove gn_unittests
from this change so that this problem goes away, and
then add a separate change so that we can discuss how best to handle testing for
the GN-based binaries there.

+                                           platform_ext={'win': '.exe',})])
+
   # TODO(phajdan.jr): Make it possible to retry telemtry tests (add JSON).
   yield (
     api.chromium.run_telemetry_unittests(),
     api.chromium.run_telemetry_perf_unittests(),
   )
 
   def deapply_patch_fn(failing_tests):
     yield (
       api.gclient.revert(),
       api.chromium.runhooks(),
@@ skipping 258 matching lines @@
     )
   )
 
   yield (
     api.test('compile_failure') +
     props() +
     api.platform.name('win') +
     api.step_data('compile (with patch)', retcode=1) +
     api.step_data('compile (with patch, lkcr, clobber)', retcode=1)
   )
+
+  # TODO(dpranke): crbug.com/353690.
+  # Remove this when we make GN a standalone recipe.
+  yield (
+    api.test('unittest_should_run_gn') +
+    api.properties.tryserver(buildername='linux', build_config='Debug') +
+    api.platform.name('linux') +
+    api.step_data('compile (gn with patch)', api.json.output(None)) +

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

These shouldn't be needed after my latest changes. :)

[Dirk Pranke, 2014/03/26 16:52:28]

Why not?

[Dirk Pranke, 2014/03/26 19:16:35]

Never mind, I think I've got it now.

+    api.step_data('gclient revert') +
+    api.step_data('checkdeps (with patch)', api.json.output(None)) +
+    api.override_step_data('deps2git (with patch)', api.json.output(None)) +
+    api.step_data('nacl_integration (with patch)', api.json.output(None)) +
+    reduce(
+      lambda a, b: a + b,

[iannucci, 2014/03/26 09:38:49]

isn't this the same as sum?

[Dirk Pranke, 2014/03/26 16:52:28]

Think so :). I was just cutting and pasting. Will take a look.

[Dirk Pranke, 2014/03/26 19:16:35]

cleaned up on merge.

+      (api.step_data('%s (with patch)' % name, canned_test(passing=True))
+       for name in GTEST_TESTS)
+    )
+  )
+
+  yield (
+    api.test('unittest_should_run_gn_compile_failure') +
+    api.properties.tryserver(buildername='linux', build_config='Debug') +
+    api.platform.name('linux') +
+    api.step_data('compile (gn with patch)', retcode=1) +
+    api.step_data('gclient revert') +

[Paweł Hajdan Jr., 2014/03/26 11:44:59]

This is likely a no-op.

[Dirk Pranke, 2014/03/26 16:52:28]

I don't know what that means; are you saying I don't need the line?

[Dirk Pranke, 2014/03/26 19:16:35]

cleaned up on merge.

+    api.step_data('compile (gn with patch, lkcr, clobber)', retcode=1)
+  )