Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1168513006:
Make swarming work w/ GN (kinda) (Closed)
|
Created:
5 years, 6 months ago by Dirk Pranke Modified:
5 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Vadim Sh., scottmg, tfarina, Paweł Hajdan Jr. Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake swarming work w/ GN (kinda).
This patch implements the basic functionality needed to make swarming
and isolates work w/ GN. It relies on GN's existing functionality
for dumping the runtime dependencies needed for a target, and adds
a new command to MB that will take the runtime deps for a target,
compute the command line needed for the target, and then generate
the .isolate and .isolate.gen.json files needed for the
'isolate.py batcharchive' command.
We still need recipe-side work for swarming to actually work, however.
In addition, the way to manage the command lines for a target is a total
hack that will need to be cleaned up in subsequent patches.
R=maruel@chromium.org, brettw@chromium.org
BUG=480053
Committed: https://crrev.com/d811358b035563cc79af2dd836f2719d231a1e36
Cr-Commit-Position: refs/heads/master@{#333114}
Patch Set 1 #
Total comments: 13
Patch Set 2 : update w/ review feedback, gn fixes #
Total comments: 3
Patch Set 3 : review feedback #Messages
Total messages: 20 (6 generated)
Brett, please see the comment in //base/BUILD.gn for thoughts on handling
directories in data deps.
Marc-Antoine, please take a look at the code in mb.py; basically I'm imagining
that we change the isolate recipe module to call out to 'mb isolate' and pass it
a JSON file w/ a list of the targets to generate .isolate files for; that
command would be a no-op for gyp-based builders.
I tested this as follows:
% echo '//base:base_unittests' > deps_list
% echo '{"targets":["base_unittests"]}' > isolate_targets.json
% gn gen //out/Release --runtime-deps-list=deps_list
% tools/mb isolate -c gn_release_bot //out/Release.gn isolate_target.json
results.json
% tools/swarming_client/swarming.py batcharchive
--isolate-server=https://isolateserver.appspot.com
out/Release.gn/base_unittests.isolate.gen.json
% tools/swarming_client/swarming.py run --isolate-server
https://isolateserver.appspot.com --swarming=https://chromium-swarm.appspot.com
-d os "Ubuntu-14.04" -d gpu none <hash>
which seemed to run successfully.
We need to figure out how to handle directory data deps, and I need to look at
more types of isolates, and write some tests before this can land, but it's a
start.
https://codereview.chromium.org/1168513006/diff/1/.gn
File .gn (right):
https://codereview.chromium.org/1168513006/diff/1/.gn#newcode89
.gn:89: "//base/BUILD.gn",
See comments in base/BUILD.gn.
https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn
File base/BUILD.gn (right):
https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn#newcode1323
base/BUILD.gn:1323: "list lines")
This is a hack; it is the equivalent of specifying "test/data/" in
base_unittest.isolate.
At the moment GN requires data to be a list of source files, and gets really
confused if you hand it a directory. We could make this slightly less of a hack
by adding in a built-in glob() function, but it might be better to create a
separate 'data_dirs' key or change 'data' to be a list of paths instead.
Brett, thoughts?
https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py
File tools/mb/mb.py (right):
https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode457
tools/mb/mb.py:457: def GetIsolateCommand(self, target, vals):
This whole function is dodgy and needs to be replaced with
something more user friendly, but I don't know what that will
be (look like) until I can take a look at more .isolate files
and see what the appropriate set of command line templates
we need to support will be.
For now, I've only tested this w/ base_unittests on linux, and everything is
hardcoded, those in theory this *might* work as-is on win and mac (but probably
won't).
https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn#newcode1323 base/BUILD.gn:1323: "list lines") On 2015/06/03 02:24:54, Dirk Pranke wrote: > This is a hack; it is the equivalent of specifying "test/data/" in > base_unittest.isolate. > > At the moment GN requires data to be a list of source files, and gets really > confused if you hand it a directory. We could make this slightly less of a hack > by adding in a built-in glob() function, but it might be better to create a > separate 'data_dirs' key or change 'data' to be a list of paths instead. > > Brett, thoughts? isolate really needs to be passed a directory, because often the directory is in PRODUCT_DIR, so enumerating at GN time will not return the files, they do not exist yet. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode151 tools/mb/mb.py:151: return 0 # .isolated files are generated during the compile in GYP. No, the .isolated.gen.json files are generated during the compile in GYP https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode440 tools/mb/mb.py:440: def GetIsolateInput(self): ReadIsolateInput ? And it's not really an isolate, it's more LoadTargetsDescriptionFile() ? https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode462 tools/mb/mb.py:462: # TODO(dpranke): We should probably pull this from Yes but obviously this only means that it works when driven through a recipe, which is semi-fine. Is the recipe name/builder name available here? I guess not. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode467 tools/mb/mb.py:467: read_only = {}.get(target, 1) The only test that can't run read_only is unit_tests on Windows. https://code.google.com/p/chromium/issues/detail?id=342913 Once fixed, you don't need this as a variable anymore. Probably worth fixing (or disabling the test, whatever)
https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1168513006/diff/1/base/BUILD.gn#newcode1323 base/BUILD.gn:1323: "list lines") On 2015/06/03 15:47:18, M-A Ruel wrote: > On 2015/06/03 02:24:54, Dirk Pranke wrote: > > This is a hack; it is the equivalent of specifying "test/data/" in > > base_unittest.isolate. > > > > At the moment GN requires data to be a list of source files, and gets really > > confused if you hand it a directory. We could make this slightly less of a > hack > > by adding in a built-in glob() function, but it might be better to create a > > separate 'data_dirs' key or change 'data' to be a list of paths instead. > > > > Brett, thoughts? > > isolate really needs to be passed a directory, because often the directory is in > PRODUCT_DIR, so enumerating at GN time will not return the files, they do not > exist yet. Yeah, it occurred to me that it would be fragile even in the normal case, since we wouldn't necessarily know when to re-run GN if the list of files changed. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode151 tools/mb/mb.py:151: return 0 # .isolated files are generated during the compile in GYP. On 2015/06/03 15:47:18, M-A Ruel wrote: > No, the .isolated.gen.json files are generated during the compile in GYP Will update. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode440 tools/mb/mb.py:440: def GetIsolateInput(self): On 2015/06/03 15:47:19, M-A Ruel wrote: > ReadIsolateInput ? > And it's not really an isolate, it's more LoadTargetsDescriptionFile() ? It was called GetIsolateInput because we're getting the input to the 'isolate' command, but I can see the confusion. I'll see if I can come up w/ something clearer. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode462 tools/mb/mb.py:462: # TODO(dpranke): We should probably pull this from On 2015/06/03 15:47:18, M-A Ruel wrote: > Yes but obviously this only means that it works when driven through a recipe, > which is semi-fine. Is the recipe name/builder name available here? I guess not. Well, it would only work w/ things using the files in testing/buildbot, at least. So far, using recipes is a requirement. I'm not sure yet if that'll stick all the way through the migration. The recipe name is not available, but the mastername and buildername are. https://codereview.chromium.org/1168513006/diff/1/tools/mb/mb.py#newcode467 tools/mb/mb.py:467: read_only = {}.get(target, 1) On 2015/06/03 15:47:18, M-A Ruel wrote: > The only test that can't run read_only is unit_tests on Windows. > https://code.google.com/p/chromium/issues/detail?id=342913 > > Once fixed, you don't need this as a variable anymore. Probably worth fixing (or > disabling the test, whatever) Ok, I'll keep that in mind.
okay, apart from needing some unit tests, and the TODO I've commented on, I think this is pretty much ready to be reviewed. Please take a look? https://codereview.chromium.org/1168513006/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1168513006/diff/20001/base/BUILD.gn#newcode1323 base/BUILD.gn:1323: "$root_out_dir/icudtl.dat", patch for this posted to https://codereview.chromium.org/1167613003/ . It probably makes sense to land that first and roll ICU in before landing this.
lgtm as a first draft then we can figure out for the command line generation. https://codereview.chromium.org/1168513006/diff/20001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1168513006/diff/20001/tools/mb/mb.py#newcode101 tools/mb/mb.py:101: subp.add_argument('input_path', nargs=1, why type=str at line 99 but not others? I'd recommend to remove it everywhere.
https://codereview.chromium.org/1168513006/diff/20001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1168513006/diff/20001/tools/mb/mb.py#newcode101 tools/mb/mb.py:101: subp.add_argument('input_path', nargs=1, On 2015/06/04 15:52:21, M-A Ruel wrote: > why type=str at line 99 but not others? I'd recommend to remove it everywhere. no particular reason. Will remove.
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1168513006/#ps40001 (title: "review feedback")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1168513006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1168513006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1168513006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d811358b035563cc79af2dd836f2719d231a1e36 Cr-Commit-Position: refs/heads/master@{#333114} |
