Issue 1416703003: [Android] Add a configurable environment for devil/. (RELAND 2)

Issue 1416703003: [Android] Add a configurable environment for devil/. (RELAND 2) (Closed)

Created:
5 years, 2 months ago by jbudorick

Modified:
5 years ago

Reviewers:
aiolos (Not reviewing), nednguyen, mikecase (-- gone --)

CC:
chromium-reviews, telemetry-reviews_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Android] Add a configurable environment for devil/. (RELAND 2) This is a reland of https://codereview.chromium.org/1399273002 BUG=476719 Committed: https://crrev.com/0c2a94ab87e6351b924a19932442e85a8f786fa0 Cr-Commit-Position: refs/heads/master@{#363204}

Patch Set 1 : https://codereview.chromium.org/1399273002 #

Patch Set 3 : rebase after multiprocess CL #

Patch Set 5 : racy os.makedirs + only discover devices on posix #

Patch Set 6 : update fastboot + F0401 -> import-error #

Patch Set 9 : aiolos comments #

Patch Set 10 : rebase #

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+742 lines, -143 lines)			Patch
M	build/android/PRESUBMIT.py	View	1 2 3 4 5 6 7 8	4 chunks	+24 lines, -9 lines	0 comments	Download
M	build/android/adb_install_apk.py	View	1 2	2 chunks	+4 lines, -0 lines	0 comments	Download
M	build/android/buildbot/bb_device_status_check.py	View	1 2	2 chunks	+3 lines, -0 lines	0 comments	Download
M	build/android/devil/android/battery_utils_test.py	View	1 2 3 4 5	1 chunk	+3 lines, -5 lines	0 comments	Download
M	build/android/devil/android/device_utils.py	View	1 2 3 4 5 6 7 8 9	3 chunks	+3 lines, -2 lines	0 comments	Download
M	build/android/devil/android/device_utils_test.py	View	1 2 3 4 5	3 chunks	+18 lines, -16 lines	0 comments	Download
M	build/android/devil/android/fastboot_utils_test.py	View	1 2 3 4 5	1 chunk	+2 lines, -4 lines	0 comments	Download
M	build/android/devil/android/logcat_monitor_test.py	View	1 2 3 4 5	1 chunk	+3 lines, -5 lines	0 comments	Download
M	build/android/devil/android/md5sum.py	View		4 chunks	+15 lines, -6 lines	0 comments	Download
M	build/android/devil/android/md5sum_test.py	View		2 chunks	+13 lines, -8 lines	0 comments	Download
M	build/android/devil/android/sdk/aapt.py	View	1	2 chunks	+7 lines, -5 lines	0 comments	Download
M	build/android/devil/android/sdk/adb_wrapper.py	View	1 2 3 4 5 6 7 8 9	4 chunks	+27 lines, -4 lines	0 comments	Download
A	build/android/devil/android/sdk/build_tools.py	View	1	1 chunk	+51 lines, -0 lines	0 comments	Download
M	build/android/devil/android/sdk/dexdump.py	View	1	2 chunks	+6 lines, -5 lines	0 comments	Download
M	build/android/devil/android/sdk/fastboot.py	View	1 2 3 4 5	2 chunks	+6 lines, -4 lines	0 comments	Download
M	build/android/devil/android/sdk/shared_prefs_test.py	View		1 chunk	+2 lines, -4 lines	0 comments	Download
M	build/android/devil/android/sdk/split_select.py	View	1	2 chunks	+7 lines, -5 lines	0 comments	Download
A	build/android/devil/devil_dependencies.json	View		1 chunk	+112 lines, -0 lines	0 comments	Download
A	build/android/devil/devil_env.py	View	1 2 3 4 5 6 7 8	1 chunk	+138 lines, -0 lines	0 comments	Download
A +	build/android/devil/utils/lazy/__init__.py	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	build/android/devil/utils/lazy/weak_constant.py	View	1	1 chunk	+29 lines, -0 lines	0 comments	Download
M	build/android/devil/utils/lsusb.py	View		1 chunk	+1 line, -1 line	0 comments	Download
M	build/android/devil/utils/mock_calls.py	View		1 chunk	+2 lines, -4 lines	0 comments	Download
M	build/android/devil/utils/mock_calls_test.py	View		1 chunk	+2 lines, -3 lines	0 comments	Download
A	build/android/devil_chromium.json	View		1 chunk	+59 lines, -0 lines	0 comments	Download
A	build/android/devil_chromium.py	View	1	1 chunk	+88 lines, -0 lines	0 comments	Download
M	build/android/gyp/apk_install.py	View		2 chunks	+10 lines, -3 lines	0 comments	Download
M	build/android/gyp/create_device_library_links.py	View		3 chunks	+8 lines, -1 line	0 comments	Download
M	build/android/gyp/get_device_configuration.py	View	1	1 chunk	+12 lines, -1 line	0 comments	Download
M	build/android/gyp/push_libraries.py	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	build/android/gyp/util/build_device.py	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	build/android/incremental_install/installer.py	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -1 line	0 comments	Download
M	build/android/provision_devices.py	View	1 2	2 chunks	+3 lines, -0 lines	0 comments	Download
M	build/android/pylib/constants/__init__.py	View	1 2 3 4 5 6 7	2 chunks	+6 lines, -35 lines	0 comments	Download
M	build/android/pylib/forwarder.py	View	1 2	5 chunks	+13 lines, -8 lines	0 comments	Download
M	build/android/pylib/perf/test_runner.py	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	build/android/pylintrc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	build/android/setup.gyp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	build/android/test_runner.py	View	1 2 3 4 5 6 7 8	3 chunks	+18 lines, -1 line	0 comments	Download
M	build/android/tombstones.py	View	1 2	2 chunks	+4 lines, -0 lines	0 comments	Download
M	build/java_apk.gypi	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	tools/telemetry/catapult_base/dependency_manager/cloud_storage_info.py	View	1 2 3 4 5 6 7	2 chunks	+8 lines, -1 line	0 comments	Download
M	tools/telemetry/telemetry/internal/platform/android_device.py	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	tools/telemetry/telemetry/internal/platform/android_device_unittest.py	View	1 2 3 4 5 6	10 chunks	+10 lines, -0 lines	0 comments	Download
M	tools/telemetry/telemetry/internal/util/binary_manager.py	View		2 chunks	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 32 (9 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

nednguyen

lgtm https://codereview.chromium.org/1416703003/diff/20001/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/1416703003/diff/20001/build/android/test_runner.py#newcode82 build/android/test_runner.py:82: group.add_argument('--num_retries', '--num-retries', dest='num_retries', Duplicate args?

5 years, 1 month ago (2015-10-28 16:52:32 UTC) #3

jbudorick

https://codereview.chromium.org/1416703003/diff/20001/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/1416703003/diff/20001/build/android/test_runner.py#newcode82 build/android/test_runner.py:82: group.add_argument('--num_retries', '--num-retries', dest='num_retries', On 2015/10/28 16:52:32, nednguyen wrote: > ...

5 years, 1 month ago (2015-10-28 16:54:25 UTC) #4

nednguyen

5 years, 1 month ago (2015-10-28 16:55:23 UTC) #5

aiolos (Not reviewing)

lgtm once it passes tests. (I haven't looked at all of them, but the ones ...

5 years, 1 month ago (2015-10-29 20:46:55 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416703003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416703003/20001

5 years, 1 month ago (2015-11-03 20:11:11 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/129725) win_chromium_x64_rel_ng on ...

5 years, 1 month ago (2015-11-03 21:09:31 UTC) #10

jbudorick

Changed telemetry/catapult to: - only support android device discovery on posix systems (which, since it ...

5 years ago (2015-12-01 16:37:16 UTC) #11

jbudorick

On 2015/12/01 16:37:16, jbudorick wrote: > Changed telemetry/catapult to: > - only support android device ...

5 years ago (2015-12-01 17:37:38 UTC) #12

aiolos (Not reviewing)

https://codereview.chromium.org/1416703003/diff/140001/build/android/PRESUBMIT.py File build/android/PRESUBMIT.py (right): https://codereview.chromium.org/1416703003/diff/140001/build/android/PRESUBMIT.py#newcode16 build/android/PRESUBMIT.py:16: chromium_src_dir = input_api.os_path.join(build_android_dir, '..', '..') Do you want to ...

5 years ago (2015-12-02 23:47:07 UTC) #14

jbudorick

5 years ago (2015-12-03 01:04:35 UTC) #15

https://codereview.chromium.org/1416703003/diff/140001/build/android/PRESUBMI...
File build/android/PRESUBMIT.py (right):

https://codereview.chromium.org/1416703003/diff/140001/build/android/PRESUBMI...
build/android/PRESUBMIT.py:16: chromium_src_dir =
input_api.os_path.join(build_android_dir, '..', '..')
On 2015/12/02 23:47:07, aiolos wrote:
> Do you want to make sure this is using an absolute path, or can we make that
> assumption about input_api.PresubmitLocalPath() ? (ie, should you add a call
to
> os_path.abspath here?)

were I keeping this, I'd probably add abspath (though AFAICT PresubmitLocalPath
should usually return one), but I think that chromium_src_dir can be removed
here since I'm just coming back down to build/android/ in the only place where I
use it.

https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/de...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/de...
build/android/devil/devil_env.py:85: with self._config_init_lock:
On 2015/12/02 23:47:07, aiolos wrote:
> This might be racy since you aren't waiting for any prior initialization to
> finish. (If Initialize was called twice, the second call will wait for
> self._config to be created, but will return before the initialization of the
> dependency_manager finishes.)
> 
> I would just remove self._config, and replace it with a check on self._dm
> existing. And then have the rest of the initialization happen within the
locked
> scope.

wow, yikes. Done.

https://codereview.chromium.org/1416703003/diff/140001/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://codereview.chromium.org/1416703003/diff/140001/build/android/pylib/fo...
build/android/pylib/forwarder.py:201: self._host_forwarder_path =
devil_env.config.FetchPath('forwarder_host')
On 2015/12/02 23:47:07, aiolos wrote:
> I assume you changed the name to forwarder_host to be consistent with the
> forwarder_device added below?

yep. The binary is still named 'host_forwarder', though.

nednguyen

lgtm with a comment https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/devil_dependencies.json File build/android/devil/devil_dependencies.json (right): https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/devil_dependencies.json#newcode96 build/android/devil/devil_dependencies.json:96: "../../../tools/telemetry/third_party/mock" I would just include ...

5 years ago (2015-12-03 02:09:51 UTC) #17

jbudorick

https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/devil_dependencies.json File build/android/devil/devil_dependencies.json (right): https://codereview.chromium.org/1416703003/diff/140001/build/android/devil/devil_dependencies.json#newcode96 build/android/devil/devil_dependencies.json:96: "../../../tools/telemetry/third_party/mock" On 2015/12/03 02:09:51, nednguyen wrote: > I would ...

5 years ago (2015-12-03 03:52:31 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416703003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416703003/160001

5 years ago (2015-12-03 03:53:11 UTC) #20

commit-bot: I haz the power

Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/104465)

5 years ago (2015-12-03 04:44:34 UTC) #22

aiolos (Not reviewing)

5 years ago (2015-12-03 08:09:39 UTC) #23

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416703003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416703003/160001

5 years ago (2015-12-03 21:56:59 UTC) #25

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416703003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416703003/180001

5 years ago (2015-12-04 14:14:53 UTC) #28

commit-bot: I haz the power

Patchset 10 (id:??) landed as https://crrev.com/0c2a94ab87e6351b924a19932442e85a8f786fa0 Cr-Commit-Position: refs/heads/master@{#363204}

5 years ago (2015-12-04 14:29:09 UTC) #31

stevenjb

5 years ago (2015-12-06 00:20:31 UTC) #32

Message was sent while issue was closed.

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1502033002/ by stevenjb@chromium.org.

The reason for reverting is: crbug.com/566503
.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages