[Android] Add PIE support for ICS devices.

[Android] Add PIE support for ICS devices.

BUG=458658
Committed: https://crrev.com/d28bff28f9b6621227be975d620f89a0a200ea90
Cr-Commit-Position: refs/heads/master@{#316595}

[jbudorick, 2015-02-14 00:45:43]

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

[jbudorick, 2015-02-14 00:48:54]

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:518: def GetDevicePieWrapper(self,
timeout=None, retries=None):
Maybe this should be done as a property, wdyt?

[perezju, 2015-02-16 10:25:23]

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:415: as_root=False, with_pie=False,
single_line=False,
Do we expect many clients needing this option? If we don't expect that many, I
think I would rather have clients calling GetDevicePieWrapper and building the
command themselves.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:518: def GetDevicePieWrapper(self,
timeout=None, retries=None):
On 2015/02/14 00:48:54, jbudorick wrote:
> Maybe this should be done as a property, wdyt?

I would lean a bit towards "No". I would reserve properties to things which are
more or less quick to compute, and do not have many side-effects when reading.
Pushing a file to the device seems to break both of those criteria.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:687:
mock.patch('os.path.exists', return_value=True)):
you could also place these in the list of assertCalls as:

  (mock.call.pylib.constants.GetOutDirectory(), '/foo/bar'),
  (mock.call.os.path.exists(mock.ANY), True),

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
File build/android/pylib/utils/md5sum.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
build/android/pylib/utils/md5sum.py:73: out = device.RunShellCommand(['sh',
md5sum_device_script_file.name])
while we are at it, could we remove the hand-rolled "write to script then run"
logic? This is now already done by RunShellCommand when necessary.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/mo...
File build/android/pylib/utils/mock_calls.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/mo...
build/android/pylib/utils/mock_calls.py:116: type(target), attribute,
new_callable=mock.PropertyMock, **kwargs)
why do we need this for this CL? could we add a test? maybe also a
comment/example?

[jbudorick, 2015-02-17 15:15:10]

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:415: as_root=False, with_pie=False,
single_line=False,
On 2015/02/16 10:25:23, perezju wrote:
> Do we expect many clients needing this option? If we don't expect that many, I
> think I would rather have clients calling GetDevicePieWrapper and building the
> command themselves.

I don't, though I do anticipate it being more than one. I chose to include it as
an option since that's how we handle most wrappers.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:518: def GetDevicePieWrapper(self,
timeout=None, retries=None):
On 2015/02/16 10:25:23, perezju wrote:
> On 2015/02/14 00:48:54, jbudorick wrote:
> > Maybe this should be done as a property, wdyt?
> 
> I would lean a bit towards "No". I would reserve properties to things which
are
> more or less quick to compute, and do not have many side-effects when reading.
> Pushing a file to the device seems to break both of those criteria.

Acknowledged.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:602:
(self.call.device.build_version_sdk(),
These calls to build_version_sdk() are why we need the changes to mock_calls.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:687:
mock.patch('os.path.exists', return_value=True)):
On 2015/02/16 10:25:23, perezju wrote:
> you could also place these in the list of assertCalls as:
> 
>   (mock.call.pylib.constants.GetOutDirectory(), '/foo/bar'),
>   (mock.call.os.path.exists(mock.ANY), True),

Oh, nice. I like that better.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
File build/android/pylib/utils/md5sum.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
build/android/pylib/utils/md5sum.py:73: out = device.RunShellCommand(['sh',
md5sum_device_script_file.name])
On 2015/02/16 10:25:23, perezju wrote:
> while we are at it, could we remove the hand-rolled "write to script then run"
> logic? This is now already done by RunShellCommand when necessary.

I'd like to do that, but I don't want to do so in this patch. The MD5 sum logic
is critical for the CQ, so I'd like to keep changes small and isolated.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/mo...
File build/android/pylib/utils/mock_calls.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/mo...
build/android/pylib/utils/mock_calls.py:116: type(target), attribute,
new_callable=mock.PropertyMock, **kwargs)
On 2015/02/16 10:25:23, perezju wrote:
> why do we need this for this CL? could we add a test? maybe also a
> comment/example?

see comment on device_utils_test.py, which uses it.

[perezju, 2015-02-17 16:04:26]

lgtm, except for my little rant about the with_pie option.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:415: as_root=False, with_pie=False,
single_line=False,
On 2015/02/17 15:15:10, jbudorick wrote:
> On 2015/02/16 10:25:23, perezju wrote:
> > Do we expect many clients needing this option? If we don't expect that many,
I
> > think I would rather have clients calling GetDevicePieWrapper and building
the
> > command themselves.
> 
> I don't, though I do anticipate it being more than one. I chose to include it
as
> an option since that's how we handle most wrappers.

I'm not crazy about this. We're adding this option now, but there are no callers
requiring it yet. Already one of the potential clients (the md5sum tool), will
probably never use this option because it needs to build it's own script anyway.

That feels, to me, like the preferred approach: the helper for the tool
requiring a special wrapper to be run, should just create the appropriate
command as required by the tool itself. Why should RunShellCommand know about
all the different wrappers that different tools might need?

Anyway, I guess you've got more experience, and I might be missing some context,
so I won't press on this point.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:602:
(self.call.device.build_version_sdk(),
On 2015/02/17 15:15:10, jbudorick wrote:
> These calls to build_version_sdk() are why we need the changes to mock_calls.

Acknowledged.

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
File build/android/pylib/utils/md5sum.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/utils/md...
build/android/pylib/utils/md5sum.py:73: out = device.RunShellCommand(['sh',
md5sum_device_script_file.name])
On 2015/02/17 15:15:10, jbudorick wrote:
> On 2015/02/16 10:25:23, perezju wrote:
> > while we are at it, could we remove the hand-rolled "write to script then
run"
> > logic? This is now already done by RunShellCommand when necessary.
> 
> I'd like to do that, but I don't want to do so in this patch. The MD5 sum
logic
> is critical for the CQ, so I'd like to keep changes small and isolated.

Acknowledged. Maybe start a bug so that we don't forget?

[jbudorick, 2015-02-17 16:14:18]

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/929603002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:415: as_root=False, with_pie=False,
single_line=False,
On 2015/02/17 16:04:26, perezju wrote:
> On 2015/02/17 15:15:10, jbudorick wrote:
> > On 2015/02/16 10:25:23, perezju wrote:
> > > Do we expect many clients needing this option? If we don't expect that
many,
> I
> > > think I would rather have clients calling GetDevicePieWrapper and building
> the
> > > command themselves.
> > 
> > I don't, though I do anticipate it being more than one. I chose to include
it
> as
> > an option since that's how we handle most wrappers.
> 
> I'm not crazy about this. We're adding this option now, but there are no
callers
> requiring it yet. Already one of the potential clients (the md5sum tool), will
> probably never use this option because it needs to build it's own script
anyway.
> 
> That feels, to me, like the preferred approach: the helper for the tool
> requiring a special wrapper to be run, should just create the appropriate
> command as required by the tool itself. Why should RunShellCommand know about
> all the different wrappers that different tools might need?
> 
> Anyway, I guess you've got more experience, and I might be missing some
context,
> so I won't press on this point.

I suppose it makes sense to hold off on adding this until we have a client. I
anticipate that happening soon, but since we don't need it now, I'll take it
out. We can discuss it further when it's relevant.

[jbudorick, 2015-02-17 16:21:17]

New patchsets have been uploaded after l-g-t-m from perezju@chromium.org

[jbudorick, 2015-02-17 16:23:35]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:24:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929603002/60001

[commit-bot: I haz the power, 2015-02-17 17:23:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-17 17:24:09]

Patchset 4 (id:??) landed as
https://crrev.com/d28bff28f9b6621227be975d620f89a0a200ea90
Cr-Commit-Position: refs/heads/master@{#316595}