[Android] Use isolate remap instead of check.

[Android] Use isolate remap instead of check.

- Instead of parsing *.isolated files, use isolate remap
to create a temporary dependency dir.
- Add an exclusion list to filter dependecies at a finer grain
than what's specified in isolate files.
- Convert base_unittests and unit_tests to use isolate. 

This adds an additional 50MB to the dependency size due
to many small directories not specified in the exclusion list.

BUG=249870
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211350

[frankf, 2013-07-03 22:36:43]

maruel/csharp: Please look at how isolate.py is used.
bulach/craigdh: Please focus on build/android scripts
jam: FYI

This is dependent on https://codereview.chromium.org/17467003/ to avoid timeouts
when calculating what to push.

[bulach, 2013-07-04 11:05:21]

lgtm for android, but a small suggestion to avoid changing the API in
base_test_runner.py..

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/base/b...
File build/android/pylib/base/base_test_runner.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/base/b...
build/android/pylib/base/base_test_runner.py:115:
src_root=constants.DIR_SOURCE_ROOT):
suggestion: there's only one caller passing the extra param, and this function
is fairly simple as it is.. so maybe it'd be better to have a
_PushDepsDirs(self) down in the test_runner.py itself? it doesn't even need any
param, it can access self.adb and self._deps_dir directly:

def _PushDepsDirs(self):
  for p in os.listdir(self._deps_dir):
    self.adb.PushIfNeeded(
        os.path.join(self._deps_dir, p)),
        os.path.join(self.adb.GetStoragePath(), p)


to be fair, it doesn't even need this function:
the caller has current 3 lines, if we just inline this it'd be 4 :)

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:233: if
self.test_package.test_suite_basename == 'webkit_unit_tests':
as above, perhaps simplify this a bit by moving the webkit before, early return
and then just inlining the deps copy.

if self.test_package.test_suite_basename == 'webkit_unit_tests':
  self.PushWebKitUnitTestsData()
if not self._deps_dir:
  logging.info('Did not find an isolate file for the test suite.')
  self.CopyTestData(_GetDataFilesForTestSuite(
    self.test_package.test_suite_basename), self.adb.GetExternalStorage())
  return
# Using deps dir.
for p in os.listdir(self._deps_dir):
  self.adb.PushIfNeeded(...)

[csharp, 2013-07-04 13:23:15]

LGTM, just a slight concern with relying on remap working a certain way.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:80: # by the remap command (hardlinked).
Otherwise, all the data
I'm a bit concerned about this code relying on remap to work a certain way.
Would it be worth adding a quick sanity check to ensure that timestamps are
preserved in the destination directory? So if something does change we easily
spot it instead of seeing problems at the pushing step.

[M-A Ruel, 2013-07-08 17:56:01]

I few notes but overall lgtm

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:31: #'net_unittests':
'net/net_unittests.isolate',
I'd recommend to keep the keys sorted even if the lines are commented out.

It makes adding and removing entries easier afterward. I don't think it's useful
to keep the commented out entries now.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:35: # Used for filtering large data deps
at a finer grain than what's allowed in
Add at least one whitespace between a variable and the next comment.

You could add a note to make it clear the list is using wildcards. I don't
really mind in fact.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:80: # by the remap command (hardlinked).
Otherwise, all the data
On 2013/07/04 13:23:16, csharp wrote:
> I'm a bit concerned about this code relying on remap to work a certain way.
> Would it be worth adding a quick sanity check to ensure that timestamps are
> preserved in the destination directory? So if something does change we easily
> spot it instead of seeing problems at the pushing step.

Or add a unit test for remap to ensure this assumption. I'm fine with that too.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:86: '--isolate=%s' % isolate_abs_path,
'--isolate', isolate_abs_path,

same for --isolated and --outdir.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:96: os.chdir(deps_dir)
try/finally

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:224: self.CopyTestData([p for p in
os.listdir(self._deps_dir)],
replace this:
  [p for p in os.listdir(self._deps_dir)]
with that:
  os.listdir(self._deps_dir)

unless I'm missing something.

[frankf, 2013-07-09 01:09:35]

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/base/b...
File build/android/pylib/base/base_test_runner.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/base/b...
build/android/pylib/base/base_test_runner.py:115:
src_root=constants.DIR_SOURCE_ROOT):
Done. I've inlined CopyTestData and removed it all together. The only other
invocation was done by instrumentation test runner.

On 2013/07/04 11:05:21, bulach wrote:
> suggestion: there's only one caller passing the extra param, and this function
> is fairly simple as it is.. so maybe it'd be better to have a
> _PushDepsDirs(self) down in the test_runner.py itself? it doesn't even need
any
> param, it can access self.adb and self._deps_dir directly:
> 
> def _PushDepsDirs(self):
>   for p in os.listdir(self._deps_dir):
>     self.adb.PushIfNeeded(
>         os.path.join(self._deps_dir, p)),
>         os.path.join(self.adb.GetStoragePath(), p)
> 
> 
> to be fair, it doesn't even need this function:
> the caller has current 3 lines, if we just inline this it'd be 4 :)

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:31: #'net_unittests':
'net/net_unittests.isolate',
On 2013/07/08 17:56:02, Marc-Antoine Ruel wrote:
> I'd recommend to keep the keys sorted even if the lines are commented out.
> 
> It makes adding and removing entries easier afterward. I don't think it's
useful
> to keep the commented out entries now.

Done.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:35: # Used for filtering large data deps
at a finer grain than what's allowed in
On 2013/07/08 17:56:02, Marc-Antoine Ruel wrote:
> Add at least one whitespace between a variable and the next comment.
> 
> You could add a note to make it clear the list is using wildcards. I don't
> really mind in fact.

Done.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:80: # by the remap command (hardlinked).
Otherwise, all the data
Done. I'm checking that the first file we hit during a walk has the same inode
as the original.

On 2013/07/08 17:56:02, Marc-Antoine Ruel wrote:
> On 2013/07/04 13:23:16, csharp wrote:
> > I'm a bit concerned about this code relying on remap to work a certain way.
> > Would it be worth adding a quick sanity check to ensure that timestamps are
> > preserved in the destination directory? So if something does change we
easily
> > spot it instead of seeing problems at the pushing step.
> 
> Or add a unit test for remap to ensure this assumption. I'm fine with that
too.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:86: '--isolate=%s' % isolate_abs_path,
On 2013/07/08 17:56:02, Marc-Antoine Ruel wrote:
> '--isolate', isolate_abs_path,
> 
> same for --isolated and --outdir.

Done.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:96: os.chdir(deps_dir)
On 2013/07/08 17:56:02, Marc-Antoine Ruel wrote:
> try/finally

Done.

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/18233018/diff/6001/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:233: if
self.test_package.test_suite_basename == 'webkit_unit_tests':
On 2013/07/04 11:05:21, bulach wrote:
> as above, perhaps simplify this a bit by moving the webkit before, early
return
> and then just inlining the deps copy.
> 
> if self.test_package.test_suite_basename == 'webkit_unit_tests':
>   self.PushWebKitUnitTestsData()
> if not self._deps_dir:
>   logging.info('Did not find an isolate file for the test suite.')
>   self.CopyTestData(_GetDataFilesForTestSuite(
>     self.test_package.test_suite_basename), self.adb.GetExternalStorage())
>   return
> # Using deps dir.
> for p in os.listdir(self._deps_dir):
>   self.adb.PushIfNeeded(...)

Done.

[M-A Ruel, 2013-07-11 19:07:10]

lgtm

https://codereview.chromium.org/18233018/diff/7006/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18233018/diff/7006/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:105: raise Exception('isolate remap
command did not use hardlinks.')
I'm thinking of changing remap behavior so it doesn't fallback to copy by
default, and would only do if --allow-copy is passed. Anyhow this sanity check
is still a good idea.

[commit-bot: I haz the power, 2013-07-12 04:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/frankf@chromium.org/18233018/57001

[commit-bot: I haz the power, 2013-07-12 06:58:45]

Change committed as 211350