|
|
Description[devil] Set permissions on install and gracefully handle errors
Grant required app permissions on apk Install, regardless of whether
the apk was already installed or not.
This caused some issues where developers manually installed apks and
then test runners would fail due to the missing permissions.
Also improve the code to deal with errors while setting permissions,
and blacklist some more unsettable permissions.
BUG=chromium:732724, chromium:739531
Review-Url: https://codereview.chromium.org/2973293002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/6fc5e4196940629dc5227a4bfdeb6db9879bbe3c
Patch Set 1 : original CL #Patch Set 2 : handle pm grant errors #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : single adb shell call #
Total comments: 4
Patch Set 5 : nits #Patch Set 6 : small fix #Messages
Total messages: 20 (11 generated)
Description was changed from ========== [devil] Always set permissions on install - Try 2 Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also blacklist some more unsettable permissions. BUG=chromium:732724 ========== to ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724 ==========
Description was changed from ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724 ========== to ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,739531 ==========
Description was changed from ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,739531 ========== to ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,chromium:739531 ==========
Description was changed from ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,chromium:739531 ========== to ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,chromium:739531 ==========
perezju@chromium.org changed reviewers: + jbudorick@chromium.org
PTAL https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:104: 'android.permission.RESTRICTED_VR_ACCESS', These two caused failures on chrome_public_test_vr_apk and mojo_test_apk. https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:124: '*.permission.C2D_MESSAGE', Previously this failed to match: org.chromium.chrome.sync_shell.permission.C2D_MESSAGE https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:2657: ['pm', 'grant', package, permission], check_return=True) Unrolling the "pm grant" calls like this has the unfortunate drawback of adding more adb calls; however I thought that both: (a) actually trying to set each and every permission (even if some of them failed) and (b) getting clearer error logs when failing to set permissions, was worth the trade-off. https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:2660: m = re.search(r'java\.lang\.\w+Exception: .*$', exc.output) Error messages seen from [1], when my previous CL landed were: Bad argument: java.lang.IllegalArgumentException: Unknown permission: android.permission.RESTRICTED_VR_ACCESS Operation not allowed: java.lang.SecurityException: Permission org.chromium.chrome.sync_shell.permission.C2D_MESSAGE is not a changeable permission type Operation not allowed: java.lang.SecurityException: Permission android.permission.INJECT_EVENTS is not a changeable permission type
https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:2660: m = re.search(r'java\.lang\.\w+Exception: .*$', exc.output) On 2017/07/10 12:31:30, perezju wrote: > Error messages seen from [1], when my previous CL landed were: > > Bad argument: java.lang.IllegalArgumentException: Unknown permission: > android.permission.RESTRICTED_VR_ACCESS > > Operation not allowed: java.lang.SecurityException: Permission > org.chromium.chrome.sync_shell.permission.C2D_MESSAGE is not a changeable > permission type > > Operation not allowed: java.lang.SecurityException: Permission > android.permission.INJECT_EVENTS is not a changeable permission type [1]: https://luci-milo.appspot.com/buildbot/chromium.android/Android%20N5X%20Swarm...
ping :)
oops, was OOO M+Tu and missed this on coming back. https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2973293002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:2657: ['pm', 'grant', package, permission], check_return=True) On 2017/07/10 12:31:30, perezju wrote: > Unrolling the "pm grant" calls like this has the unfortunate drawback of adding > more adb calls; however I thought that both: (a) actually trying to set each and > every permission (even if some of them failed) and (b) getting clearer error > logs when failing to set permissions, was worth the trade-off. Could we join the pm grant calls w/ ';' rather than '&&'? Maybe something like ';'.join('pm grant %s %s || echo "failed to set permission %s"' % (package, p, p) for p in permissions) It'd be nice to not increase the number of adb calls here.
On 2017/07/14 15:45:28, jbudorick wrote: > It'd be nice to not increase the number of adb calls here. Done. Maybe I went a bit more paranoid than needed but, well, let me know what you think.
lgtm w/ nits https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:2686: res = self.RunShellCommand( nit: might be worth using large_output=True here, too. https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:2699: m = re.search(r'java\.lang\.\w+Exception: .*$', output, re.MULTILINE) nit: compile the regex as a module-scope constant
https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:2686: res = self.RunShellCommand( On 2017/07/17 15:31:38, jbudorick wrote: > nit: might be worth using large_output=True here, too. Thought about that, but needs another fix. Currently large_output=True causes stderr (where these error messages are found) to be lost. I'll whip up a CL to fix that before landing this. https://codereview.chromium.org/2973293002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:2699: m = re.search(r'java\.lang\.\w+Exception: .*$', output, re.MULTILINE) On 2017/07/17 15:31:38, jbudorick wrote: > nit: compile the regex as a module-scope constant Done.
The CQ bit was checked by perezju@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by perezju@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2973293002/#ps100001 (title: "small fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500367718568990, "parent_rev": "44b022b2a09508ec025ae76a26308e89deb2cf69", "commit_rev": "6fc5e4196940629dc5227a4bfdeb6db9879bbe3c"}
Message was sent while issue was closed.
Description was changed from ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,chromium:739531 ========== to ========== [devil] Set permissions on install and gracefully handle errors Grant required app permissions on apk Install, regardless of whether the apk was already installed or not. This caused some issues where developers manually installed apks and then test runners would fail due to the missing permissions. Also improve the code to deal with errors while setting permissions, and blacklist some more unsettable permissions. BUG=chromium:732724,chromium:739531 Review-Url: https://codereview.chromium.org/2973293002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |