[android] Stop using isolate.py for data dependency management.

[android] Stop using isolate.py for data dependency management.

BUG=663110
Committed: https://crrev.com/634c87cf4af7ac11f2dbe7687a6d7904c2217ee3
Cr-Commit-Position: refs/heads/master@{#432940}

[jbudorick, 2016-11-11 02:58:40]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 02:59:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbudorick, 2016-11-11 03:05:03]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 03:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 04:04:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 04:04:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbudorick, 2016-11-11 16:30:49]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 16:31:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 16:39:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 16:39:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbudorick, 2016-11-11 18:06:26]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 18:06:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 18:40:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 18:40:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbudorick, 2016-11-14 22:03:33]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 22:05:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 23:26:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 23:26:07]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2016-11-14 23:27:21]

jbudorick@chromium.org changed reviewers:
+ agrieve@chromium.org

[agrieve, 2016-11-15 15:51:01]

Looks great overall! So much nicer to drop this dependency :)

lgtm

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
File build/android/pylib/utils/device_dependencies.py (right):

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
build/android/pylib/utils/device_dependencies.py:14: This returns the device
path as a list of joinable path compoents,
compoents

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
build/android/pylib/utils/device_dependencies.py:44: if
os.path.commonprefix([output_directory, host_path]) == output_directory:
commonprefix just does string comparison. So.. I think would be simpler as:
  host_path.startswith(output_directory)

https://codereview.chromium.org/2492123002/diff/80001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2492123002/diff/80001/build/android/test_runn...
build/android/test_runner.py:207: group.add_argument('--gn-variable-json')
unused?

[jbudorick, 2016-11-15 19:33:14]

jbudorick@chromium.org changed reviewers:
+ dpranke@chromium.org

[jbudorick, 2016-11-15 19:33:15]

+dpranke for //PRESUBMIT.py

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
File build/android/pylib/utils/device_dependencies.py (right):

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
build/android/pylib/utils/device_dependencies.py:14: This returns the device
path as a list of joinable path compoents,
On 2016/11/15 15:51:01, agrieve wrote:
> compoents

Doe

https://codereview.chromium.org/2492123002/diff/80001/build/android/pylib/uti...
build/android/pylib/utils/device_dependencies.py:44: if
os.path.commonprefix([output_directory, host_path]) == output_directory:
On 2016/11/15 15:51:01, agrieve wrote:
> commonprefix just does string comparison. So.. I think would be simpler as:
>   host_path.startswith(output_directory)

Done.

https://codereview.chromium.org/2492123002/diff/80001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2492123002/diff/80001/build/android/test_runn...
build/android/test_runner.py:207: group.add_argument('--gn-variable-json')
On 2016/11/15 15:51:01, agrieve wrote:
> unused?

oops, leftover from a previous sketch. Removed.

[jbudorick, 2016-11-15 19:36:32]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:37:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 21:03:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 21:03:35]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-11-15 22:33:06]

//PRESUBMIT.py lgtm.

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
File build/android/pylib/utils/device_dependencies.py (right):

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/device_dependencies.py:37: [None, 'icu_fake_dir',
'icudtl.dat']
I don't know any of the surrounding code, so don't take this comment as a
blocker, but this seems kind of strange that you can't distinguish between
something relative to //out and something relative to //out/Release. Why not map
everything to //out/Release?

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/device_dependencies.py:72: isolate_contents =
ast.literal_eval(isolate_file.read())
Where does the isolate_file_path come from? Can we change this to just read the
.runtime_deps file instead, so that there's no dependency on isolates
whatsoever?

[jbudorick, 2016-11-16 03:33:47]

Description was changed from

==========
[android] Stop using isolate.py for data dependency management.

Note that this still uses isolate *files*, it simply doesn't
use isolate.py to handle them.

BUG=663110
==========

to

==========
[android] Stop using isolate.py for data dependency management.

BUG=663110
==========

[jbudorick, 2016-11-16 03:33:51]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 03:34:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbudorick, 2016-11-16 03:46:54]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 03:47:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbudorick, 2016-11-16 03:47:51]

Now with 100% less isolate. ptal

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
File build/android/pylib/utils/device_dependencies.py (right):

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/device_dependencies.py:37: [None, 'icu_fake_dir',
'icudtl.dat']
On 2016/11/15 22:33:06, Dirk Pranke wrote:
> I don't know any of the surrounding code, so don't take this comment as a
> blocker, but this seems kind of strange that you can't distinguish between
> something relative to //out and something relative to //out/Release. Why not
map
> everything to //out/Release?

I'm not sure what you mean here, which leads me to believe that my comment may
be written confusingly. The distinction isn't between things in //out and
//out/Release; it's between things in //out/Release and things not in //out at
all.

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/device_dependencies.py:72: isolate_contents =
ast.literal_eval(isolate_file.read())
On 2016/11/15 22:33:05, Dirk Pranke wrote:
> Where does the isolate_file_path come from? Can we change this to just read
the
> .runtime_deps file instead, so that there's no dependency on isolates
> whatsoever?

Rewritten.

[commit-bot: I haz the power, 2016-11-16 04:58:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 04:58:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbudorick, 2016-11-16 06:32:27]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 06:32:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 07:51:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 07:51:58]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-11-16 14:55:50]

lgtm

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
File build/android/gn/generate_isolate.py (left):

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
build/android/gn/generate_isolate.py:1: #!/usr/bin/env python
Much nicer to not need the .isolate at all!

I may be tempted to put this script back at some point, as I one day hope to
make it a part of the wrapper script to run them on swarming. E.g.
bin/run_base_unittests --swarming.

Makes sense to delete for now though.

[jbudorick, 2016-11-16 15:00:52]

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
File build/android/gn/generate_isolate.py (left):

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
build/android/gn/generate_isolate.py:1: #!/usr/bin/env python
On 2016/11/16 14:55:50, agrieve wrote:
> Much nicer to not need the .isolate at all!
> 
> I may be tempted to put this script back at some point, as I one day hope to
> make it a part of the wrapper script to run them on swarming. E.g.
> bin/run_base_unittests --swarming.
> 
> Makes sense to delete for now though.

You should talk to stip; he's been mulling over a similar idea. In any event,
this shouldn't be necessary for that case.

[Dirk Pranke, 2016-11-17 03:05:45]

lgtm.

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
File build/android/pylib/utils/device_dependencies.py (right):

https://codereview.chromium.org/2492123002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/device_dependencies.py:37: [None, 'icu_fake_dir',
'icudtl.dat']
On 2016/11/16 03:47:51, jbudorick wrote:
> On 2016/11/15 22:33:06, Dirk Pranke wrote:
> > I don't know any of the surrounding code, so don't take this comment as a
> > blocker, but this seems kind of strange that you can't distinguish between
> > something relative to //out and something relative to //out/Release. Why not
> map
> > everything to //out/Release?
> 
> I'm not sure what you mean here, which leads me to believe that my comment may
> be written confusingly. The distinction isn't between things in //out and
> //out/Release; it's between things in //out/Release and things not in //out at
> all.

Ah, it doesn't help that I wrote "//out" when I meant to write "//". So I
contributed a fair
amount to the confusion.

That said, I think I just didn't understand what was going on. I now see that
this is specifying
the place to write *to*. I was thinking it was the place to read *from*.

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
File build/android/gn/generate_isolate.py (left):

https://codereview.chromium.org/2492123002/diff/140001/build/android/gn/gener...
build/android/gn/generate_isolate.py:1: #!/usr/bin/env python
On 2016/11/16 15:00:52, jbudorick wrote:
> On 2016/11/16 14:55:50, agrieve wrote:
> > Much nicer to not need the .isolate at all!
> > 
> > I may be tempted to put this script back at some point, as I one day hope to
> > make it a part of the wrapper script to run them on swarming. E.g.
> > bin/run_base_unittests --swarming.

I also want this to work at some point.

[jbudorick, 2016-11-17 14:29:17]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2016-11-17 14:29:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 16:04:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 16:04:57]

Failed to apply patch for build/android/pylib/base/test_instance_factory.py:
While running git apply --index -p1;
  error: patch failed: build/android/pylib/base/test_instance_factory.py:6
  error: build/android/pylib/base/test_instance_factory.py: patch does not apply

Patch:       build/android/pylib/base/test_instance_factory.py
Index: build/android/pylib/base/test_instance_factory.py
diff --git a/build/android/pylib/base/test_instance_factory.py
b/build/android/pylib/base/test_instance_factory.py
index
129bd7f2f62a65b636ae92723a2071788d86332f..3658ab69a17b19b3b469b3ac7031720bf6b52775
100644
--- a/build/android/pylib/base/test_instance_factory.py
+++ b/build/android/pylib/base/test_instance_factory.py
@@ -6,17 +6,17 @@ from pylib.gtest import gtest_test_instance
 from pylib.instrumentation import instrumentation_test_instance
 from pylib.junit import junit_test_instance
 from pylib.perf import perf_test_instance
-from pylib.utils import isolator
+from pylib.utils import device_dependencies
 
 
 def CreateTestInstance(args, error_func):
 
   if args.command == 'gtest':
     return gtest_test_instance.GtestTestInstance(
-        args, isolator.Isolator(), error_func)
+        args, device_dependencies.GetDataDependencies, error_func)
   elif args.command == 'instrumentation':
     return instrumentation_test_instance.InstrumentationTestInstance(
-        args, isolator.Isolator(), error_func)
+        args, device_dependencies.GetDataDependencies, error_func)
   elif args.command == 'junit':
     return junit_test_instance.JunitTestInstance(args, error_func)
   elif args.command == 'perf':

[jbudorick, 2016-11-17 17:36:31]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2016-11-17 17:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2492123002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 17:36:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 20:12:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-17 20:29:03]

Description was changed from

==========
[android] Stop using isolate.py for data dependency management.

BUG=663110
==========

to

==========
[android] Stop using isolate.py for data dependency management.

BUG=663110

Committed: https://crrev.com/634c87cf4af7ac11f2dbe7687a6d7904c2217ee3
Cr-Commit-Position: refs/heads/master@{#432940}
==========

[commit-bot: I haz the power, 2016-11-17 20:29:04]

Patchset 9 (id:??) landed as
https://crrev.com/634c87cf4af7ac11f2dbe7687a6d7904c2217ee3
Cr-Commit-Position: refs/heads/master@{#432940}

[jbudorick, 2016-11-17 23:51:41]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2514453003/ by jbudorick@chromium.org.

The reason for reverting is: Breaks the blink bot, e.g.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N....