Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734023002:
Generalize the removal of system apps (Closed)
DescriptionGeneralize the removal of system apps
Generalizes the approach used by RemoveSystemWebView to allow the removal
of arbitrary system apps via the --remove-system-apps argument.
Tested by running the following command on a newly reflashed Pixel and
5X and verifying the log output + the contents of /system/app:
provision_devices.py -d <device> --remove-system-webview \
--remove-system-apps GoogleVrCore NotARealSystemApp -v -v -v
BUG=chromium:671373
, catapult:#3341
Review-Url: https://codereview.chromium.org/2734023002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/eaf9a11d28212ac48b5bfd16c4529ec5756afc2f
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address yolandyan@ comments #
Total comments: 28
Patch Set 3 : Address comments #Patch Set 4 : Un-hardcode /system/app #
Total comments: 4
Patch Set 5 : Address jbudorick@ comments #
Total comments: 7
Patch Set 6 : Address perezju@ comments #Messages
Total messages: 31 (9 generated)
Description was changed from ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-app argument. Tested by running the following command on a newly reflashed Pixel and verifying the log output + the contents of /system/app: provision_devices.py -d FA6AN0303517 --remove-system-webview \ --remove-system-app GoogleVrCore --remove-system-app NotARealSystemApp \ -v -v -v BUG=catapult:#3341 ========== to ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-app argument. Tested by running the following command on a newly reflashed Pixel and verifying the log output + the contents of /system/app: provision_devices.py -d FA6AN0303517 --remove-system-webview \ --remove-system-app GoogleVrCore --remove-system-app NotARealSystemApp \ -v -v -v BUG=chromium:671373, catapult:#3341 ==========
yolandyan@chromium.org changed reviewers: + yolandyan@chromium.org
https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:128: system_app_remove_list = system_app_remove_list or [] it seems like you don't need this line https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:375: path = os.path.join('/system/app', system_app) nit: doc string https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:383: for system_app in system_app_remove_list: nit: doc string https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:480: device.EnableRoot() hmm, this would not work without root? https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:576: '--remove-system-app', action='append', dest='system_app_remove_list', nits: "append" means you will have to do `--remove-system-app a.b.c --remove-system-app b.c.d` maybe just use `--remove-system-apps` with `nargs` and do `--remove-system-apps a.b.c b.d.c` feel free to ignore this one
https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:128: system_app_remove_list = system_app_remove_list or [] On 2017/03/07 00:57:13, the real yoland wrote: > it seems like you don't need this line Done. https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:375: path = os.path.join('/system/app', system_app) On 2017/03/07 00:57:13, the real yoland wrote: > nit: doc string Done. https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:383: for system_app in system_app_remove_list: On 2017/03/07 00:57:13, the real yoland wrote: > nit: doc string Done. https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:480: device.EnableRoot() On 2017/03/07 00:57:13, the real yoland wrote: > hmm, this would not work without root? Without this, I was getting a the following: java.lang.SecurityException: Permission Denial: not allowed to send broadcast android.intent.action.TIME_SET from pid=2920, uid=2000 I was able to reproduce this manually by running "adb shell am broadcast -a android.intent.action.TIME_SET" without adbd running as root. Running as root fixed it. https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:576: '--remove-system-app', action='append', dest='system_app_remove_list', On 2017/03/07 00:57:13, the real yoland wrote: > nits: > "append" means you will have to do `--remove-system-app a.b.c > --remove-system-app b.c.d` > maybe just use `--remove-system-apps` with `nargs` and do `--remove-system-apps > a.b.c b.d.c` > > feel free to ignore this one Done.
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] If I run `adb shell pm list packages | awk -F: '{print $2}' | grep "com.google" | xargs -I {} adb shell pm path {}` I get packages under both `/system/app` and `/system/priv-app` https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:356: and not device.verity_disabled): ? https://cs.chromium.org/search/?q=verity_disabled&sq=package:chromium&type=cs the purpose of this check is to not reboot again and again, you can move the disable verity part to the outer function in RemoveSystemApps, and get rid of RemoveSystemApp
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Add owners for this directory as reviewers
https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/1/devil/devil/android/tools/p... devil/devil/android/tools/provision_devices.py:480: device.EnableRoot() On 2017/03/07 01:42:53, bsheedy wrote: > On 2017/03/07 00:57:13, the real yoland wrote: > > hmm, this would not work without root? > > Without this, I was getting a the following: > > java.lang.SecurityException: Permission Denial: not allowed to send broadcast > android.intent.action.TIME_SET from pid=2920, uid=2000 > > I was able to reproduce this manually by running "adb shell am broadcast -a > android.intent.action.TIME_SET" without adbd running as root. Running as root > fixed it. I want to talk to you offline about this one. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:325: self.verity_disabled = False This shouldn't be handled w/ raw public data. It's too easy for it to get corrupted, particularly across reboots. If we want to store this -- and I'm not sure we do -- we'd want to move the Enable/DisableVerity functions up into DeviceUtils and store the state in self._cache. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:126: steps.append(ProvisionStep(RemoveSystemWebView)) remove_system_webview should just extend system_app_remove_list (though you'll need to add the `system_app_remove_list = system_app_remove_list or []` line) https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:350: def _RemoveSystemApp(device, path): I'm wondering if this behavior is something we'd want in DeviceUtils (or a DeviceUtils agent of some kind, a la BatteryUtils). Regardless, this should probably be decorated w/ one of the timeout&retry decorators from devil.android.decorators for threading reasons -- maybe WithExplicitTimeoutAndRetries w/ DeviceUtils.REBOOT_DEFAULT_TIMEOUT? https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:370: logger.warning('Cannot remove %s from a non-rooted device', path) This should raise an exception (probably a CommandFailedError). If a caller wants to demote it to a warning, that's fine. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:373: def RemoveSystemApp(device, system_app): Why is this separate from _RemoveSystemApp? https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:399: def RemoveSystemWebView(device): I don't mind the option staying for backwards compatibility, but this function should be replaced by RemoveSystemApps.
perezju@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:350: def _RemoveSystemApp(device, path): On 2017/03/07 02:50:54, jbudorick wrote: > I'm wondering if this behavior is something we'd want in DeviceUtils (or a > DeviceUtils agent of some kind, a la BatteryUtils). > > Regardless, this should probably be decorated w/ one of the timeout&retry > decorators from devil.android.decorators for threading reasons -- maybe > WithExplicitTimeoutAndRetries w/ DeviceUtils.REBOOT_DEFAULT_TIMEOUT? -1 I don't think we want a top-level timeout/retry decorator here. All things called *do* have their own timeout/retries. If one of those needs to be larger/smaller then we can change that. (Also I'm not entirely sure whether the default logic for nested timeout/retries is what we want here.) https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:370: logger.warning('Cannot remove %s from a non-rooted device', path) On 2017/03/07 02:50:54, jbudorick wrote: > This should raise an exception (probably a CommandFailedError). If a caller > wants to demote it to a warning, that's fine. +1 https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:396: RemoveSystemApp(device, system_app) I think we don't want to reboot/restart for each app being removed. between a single "adb shell start" and "stop" (above) we should remove all system apps. This also removes the need (I think) to track whether "verity_disabled" has been applied. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:399: def RemoveSystemWebView(device): On 2017/03/07 02:50:54, jbudorick wrote: > I don't mind the option staying for backwards compatibility, but this function > should be replaced by RemoveSystemApps. +1 --remove-system-webview should just be a shortcut for the equivalent --remove-system-apps flags; and this method should be gone. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:589: help='the names of system apps to remove') nit: move this option up before --remove-system-webview
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:350: def _RemoveSystemApp(device, path): On 2017/03/07 11:35:08, perezju wrote: > On 2017/03/07 02:50:54, jbudorick wrote: > > I'm wondering if this behavior is something we'd want in DeviceUtils (or a > > DeviceUtils agent of some kind, a la BatteryUtils). > > > > Regardless, this should probably be decorated w/ one of the timeout&retry > > decorators from devil.android.decorators for threading reasons -- maybe > > WithExplicitTimeoutAndRetries w/ DeviceUtils.REBOOT_DEFAULT_TIMEOUT? > > -1 > > I don't think we want a top-level timeout/retry decorator here. All things > called *do* have their own timeout/retries. If one of those needs to be > larger/smaller then we can change that. My reasoning here is less about setting a timeout for the entire process and more about threading -- each of the decorated DeviceUtils calls (and there are a lot of them) spawns its own thread. If this were decorated, that would not be the case. > > (Also I'm not entirely sure whether the default logic for nested timeout/retries > is what we want here.) I suppose that we may not want to start over from the beginning in the event of failure.
Addressed all the comments except the one group that seemed like a solution hadn't been agreed on yet. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/dev... devil/devil/android/device_utils.py:325: self.verity_disabled = False On 2017/03/07 02:50:53, jbudorick wrote: > This shouldn't be handled w/ raw public data. It's too easy for it to get > corrupted, particularly across reboots. If we want to store this -- and I'm not > sure we do -- we'd want to move the Enable/DisableVerity functions up into > DeviceUtils and store the state in self._cache. Removed. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] On 2017/03/07 02:28:32, the real yoland wrote: > If I run `adb shell pm list packages | awk -F: '{print $2}' | grep "com.google" > | xargs -I {} adb shell pm path {}` > I get packages under both `/system/app` and `/system/priv-app` I'm not sure if deleting from that directory is necessary - the previous code and Daydream's script for removing the stock VRCore installed on Daydream-ready devices both only remove from /system/app. Additionally, removing from just /system/app seems to fully remove the app - it doesn't show up in the Apps settings menu, and you can install/uninstall from the Play Store instead of upgrade/disable. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:126: steps.append(ProvisionStep(RemoveSystemWebView)) On 2017/03/07 02:50:54, jbudorick wrote: > remove_system_webview should just extend system_app_remove_list (though you'll > need to add the `system_app_remove_list = system_app_remove_list or []` line) Done. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:356: and not device.verity_disabled): On 2017/03/07 02:28:32, the real yoland wrote: > ? https://cs.chromium.org/search/?q=verity_disabled&sq=package:chromium&type=cs > > the purpose of this check is to not reboot again and again, you can move the > disable verity part to the outer function > in RemoveSystemApps, and get rid of RemoveSystemApp Done. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:370: logger.warning('Cannot remove %s from a non-rooted device', path) On 2017/03/07 11:35:08, perezju wrote: > On 2017/03/07 02:50:54, jbudorick wrote: > > This should raise an exception (probably a CommandFailedError). If a caller > > wants to demote it to a warning, that's fine. > > +1 Done. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:373: def RemoveSystemApp(device, system_app): On 2017/03/07 02:50:54, jbudorick wrote: > Why is this separate from _RemoveSystemApp? Merged into one. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:396: RemoveSystemApp(device, system_app) On 2017/03/07 11:35:09, perezju wrote: > I think we don't want to reboot/restart for each app being removed. > > between a single "adb shell start" and "stop" (above) we should remove all > system apps. > > This also removes the need (I think) to track whether "verity_disabled" has been > applied. Done. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:399: def RemoveSystemWebView(device): On 2017/03/07 11:35:08, perezju wrote: > On 2017/03/07 02:50:54, jbudorick wrote: > > I don't mind the option staying for backwards compatibility, but this function > > should be replaced by RemoveSystemApps. > > +1 --remove-system-webview should just be a shortcut for the equivalent > --remove-system-apps flags; and this method should be gone. Done. https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:589: help='the names of system apps to remove') On 2017/03/07 11:35:08, perezju wrote: > nit: move this option up before --remove-system-webview Done.
Description was changed from ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-app argument. Tested by running the following command on a newly reflashed Pixel and verifying the log output + the contents of /system/app: provision_devices.py -d FA6AN0303517 --remove-system-webview \ --remove-system-app GoogleVrCore --remove-system-app NotARealSystemApp \ -v -v -v BUG=chromium:671373, catapult:#3341 ========== to ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-apps argument. Tested by running the following command on a newly reflashed Pixel and 5X and verifying the log output + the contents of /system/app: provision_devices.py -d <device> --remove-system-webview \ --remove-system-apps GoogleVrCore NotARealSystemApp -v -v -v BUG=chromium:671373, catapult:#3341 ==========
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:350: def _RemoveSystemApp(device, path): On 2017/03/07 15:14:05, jbudorick wrote: > On 2017/03/07 11:35:08, perezju wrote: > > On 2017/03/07 02:50:54, jbudorick wrote: > > > I'm wondering if this behavior is something we'd want in DeviceUtils (or a > > > DeviceUtils agent of some kind, a la BatteryUtils). > > > > > > Regardless, this should probably be decorated w/ one of the timeout&retry > > > decorators from devil.android.decorators for threading reasons -- maybe > > > WithExplicitTimeoutAndRetries w/ DeviceUtils.REBOOT_DEFAULT_TIMEOUT? > > > > -1 > > > > I don't think we want a top-level timeout/retry decorator here. All things > > called *do* have their own timeout/retries. If one of those needs to be > > larger/smaller then we can change that. > > My reasoning here is less about setting a timeout for the entire process and > more about threading -- each of the decorated DeviceUtils calls (and there are a > lot of them) spawns its own thread. If this were decorated, that would not be > the case. > > > > > (Also I'm not entirely sure whether the default logic for nested > timeout/retries > > is what we want here.) > > I suppose that we may not want to start over from the beginning in the event of > failure. FWIW, the updated logic now runs a similar number of device_utils calls as RemoveSystemWebView did. The only difference is the multiple calls to device.RunShellCommand(['rm', ... ]) if there are multiple apps that actually need to be removed.
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] On 2017/03/07 at 18:04:29, bsheedy wrote: > On 2017/03/07 02:28:32, the real yoland wrote: > > If I run `adb shell pm list packages | awk -F: '{print $2}' | grep "com.google" > > | xargs -I {} adb shell pm path {}` > > I get packages under both `/system/app` and `/system/priv-app` > > I'm not sure if deleting from that directory is necessary - the previous code and Daydream's script for removing the stock VRCore installed on Daydream-ready devices both only remove from /system/app. Additionally, removing from just /system/app seems to fully remove the app - it doesn't show up in the Apps settings menu, and you can install/uninstall from the Play Store instead of upgrade/disable. I didn't mean it needs to be removed from more than /system/app, the API is support to support delete system app. But system apps can exist in both `/system/app` and `/system/priv-app` for my N device. So it should not hardcode /system/app/ part
On 2017/03/08 at 00:32:53, the real yoland wrote: > https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... > File devil/devil/android/tools/provision_devices.py (right): > > https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... > devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] > On 2017/03/07 at 18:04:29, bsheedy wrote: > > On 2017/03/07 02:28:32, the real yoland wrote: > > > If I run `adb shell pm list packages | awk -F: '{print $2}' | grep "com.google" > > > | xargs -I {} adb shell pm path {}` > > > I get packages under both `/system/app` and `/system/priv-app` > > > > I'm not sure if deleting from that directory is necessary - the previous code and Daydream's script for removing the stock VRCore installed on Daydream-ready devices both only remove from /system/app. Additionally, removing from just /system/app seems to fully remove the app - it doesn't show up in the Apps settings menu, and you can install/uninstall from the Play Store instead of upgrade/disable. > > I didn't mean it needs to be removed from more than /system/app, the API is support to support delete system app. But system apps can exist in both `/system/app` and `/system/priv-app` for my N device. > So it should not hardcode /system/app/ part supposed to support*
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] On 2017/03/08 at 00:32:53, the real yoland wrote: > On 2017/03/07 at 18:04:29, bsheedy wrote: > > On 2017/03/07 02:28:32, the real yoland wrote: > > > If I run `adb shell pm list packages | awk -F: '{print $2}' | grep "com.google" > > > | xargs -I {} adb shell pm path {}` > > > I get packages under both `/system/app` and `/system/priv-app` > > > > I'm not sure if deleting from that directory is necessary - the previous code and Daydream's script for removing the stock VRCore installed on Daydream-ready devices both only remove from /system/app. Additionally, removing from just /system/app seems to fully remove the app - it doesn't show up in the Apps settings menu, and you can install/uninstall from the Play Store instead of upgrade/disable. > > I didn't mean it needs to be removed from more than /system/app, the API is support to support delete system app. But system apps can exist in both `/system/app` and `/system/priv-app` for my N device. > So it should not hardcode /system/app/ part Or, you can use do RunShellCommand(['pm', 'path' 'x.y.z']) to find out the location of the system app.
https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:52: _SYSTEM_WEBVIEW_NAMES = ['webview', 'WebViewGoogle'] On 2017/03/08 00:36:03, the real yoland wrote: > On 2017/03/08 at 00:32:53, the real yoland wrote: > > On 2017/03/07 at 18:04:29, bsheedy wrote: > > > On 2017/03/07 02:28:32, the real yoland wrote: > > > > If I run `adb shell pm list packages | awk -F: '{print $2}' | grep > "com.google" > > > > | xargs -I {} adb shell pm path {}` > > > > I get packages under both `/system/app` and `/system/priv-app` > > > > > > I'm not sure if deleting from that directory is necessary - the previous > code and Daydream's script for removing the stock VRCore installed on > Daydream-ready devices both only remove from /system/app. Additionally, removing > from just /system/app seems to fully remove the app - it doesn't show up in the > Apps settings menu, and you can install/uninstall from the Play Store instead of > upgrade/disable. > > > > I didn't mean it needs to be removed from more than /system/app, the API is > support to support delete system app. But system apps can exist in both > `/system/app` and `/system/priv-app` for my N device. > > So it should not hardcode /system/app/ part > > Or, you can use do RunShellCommand(['pm', 'path' 'x.y.z']) to find out the > location of the system app. Done. Now looks in all the directories in _SYSTEM_APP_DIRECTORIES instead of just /system/app/
lgtm w/ nits https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/20001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:350: def _RemoveSystemApp(device, path): On 2017/03/08 00:27:30, bsheedy wrote: > On 2017/03/07 15:14:05, jbudorick wrote: > > On 2017/03/07 11:35:08, perezju wrote: > > > On 2017/03/07 02:50:54, jbudorick wrote: > > > > I'm wondering if this behavior is something we'd want in DeviceUtils (or a > > > > DeviceUtils agent of some kind, a la BatteryUtils). > > > > > > > > Regardless, this should probably be decorated w/ one of the timeout&retry > > > > decorators from devil.android.decorators for threading reasons -- maybe > > > > WithExplicitTimeoutAndRetries w/ DeviceUtils.REBOOT_DEFAULT_TIMEOUT? > > > > > > -1 > > > > > > I don't think we want a top-level timeout/retry decorator here. All things > > > called *do* have their own timeout/retries. If one of those needs to be > > > larger/smaller then we can change that. > > > > My reasoning here is less about setting a timeout for the entire process and > > more about threading -- each of the decorated DeviceUtils calls (and there are > a > > lot of them) spawns its own thread. If this were decorated, that would not be > > the case. > > > > > > > > (Also I'm not entirely sure whether the default logic for nested > > timeout/retries > > > is what we want here.) > > > > I suppose that we may not want to start over from the beginning in the event > of > > failure. > > FWIW, the updated logic now runs a similar number of device_utils calls as > RemoveSystemWebView did. The only difference is the multiple calls to > device.RunShellCommand(['rm', ... ]) if there are multiple apps that actually > need to be removed. hm, ok. I'm ok with leaving this w/o the decorator. https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:351: nit: +1 blank line https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:379: device.verity_disabled = True Remove this.
I'll wait on an LGTM from another reviewer before submitting. https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:351: On 2017/03/08 01:36:33, jbudorick wrote: > nit: +1 blank line Done. https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:379: device.verity_disabled = True On 2017/03/08 01:36:33, jbudorick wrote: > Remove this. Done.
On 2017/03/08 01:39:17, bsheedy wrote: > I'll wait on an LGTM from another reviewer before submitting. yeah, wait for perezju > > https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... > File devil/devil/android/tools/provision_devices.py (right): > > https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... > devil/devil/android/tools/provision_devices.py:351: > On 2017/03/08 01:36:33, jbudorick wrote: > > nit: +1 blank line > > Done. > > https://codereview.chromium.org/2734023002/diff/60001/devil/devil/android/too... > devil/devil/android/tools/provision_devices.py:379: device.verity_disabled = > True > On 2017/03/08 01:36:33, jbudorick wrote: > > Remove this. > > Done.
Thanks for the changes! Just a couple more comments. https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:87: remove_system_webview=False, I think that the ProvisionDevices function should *not* have a remove_system_webview option. And the bit of logic to extend the list of system apps to remove should be moved to the main below. https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:360: logger.info('System app %s already removed', path) This might be a bit too log-spammy, particularly because apps tend to be found either on 'app' or 'priv-app' but not both; so we'll get slightly misleading notes about a "system app FOO already removed" that wasn't actually removed (it was never there!). I would suggest instead: loop over each system_app to remove, collect into a list of found_paths when the app is found in any of the _SYSTEM_APP_DIRECTORIES, if it isn't found in any of them, then log a warning. Finally, call device.RemovePath(found_paths, force=True, recursive=True) once. https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:388: 'Cannot remove system apps from non-rooted device', str(device)) nit: 'Cannot remove' -> 'Failed to remove'
https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:87: remove_system_webview=False, On 2017/03/08 10:19:04, perezju wrote: > I think that the ProvisionDevices function should *not* have a > remove_system_webview option. And the bit of logic to extend the list of system > apps to remove should be moved to the main below. I disagree. I've increasingly been moving all of the business logic for these scripts into functions that share their names (i.e., provision_devices ~ ProvisionDevices) w/ the arguments for the function matching the command-line arguments for the script s.t. callers can interact with them directly if desired. Leaving remove_system_webview here allows us to maintain that parity.
https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:87: remove_system_webview=False, On 2017/03/08 13:54:13, jbudorick wrote: > On 2017/03/08 10:19:04, perezju wrote: > > I think that the ProvisionDevices function should *not* have a > > remove_system_webview option. And the bit of logic to extend the list of > system > > apps to remove should be moved to the main below. > > I disagree. I've increasingly been moving all of the business logic for these > scripts into functions that share their names (i.e., provision_devices ~ > ProvisionDevices) w/ the arguments for the function matching the command-line > arguments for the script s.t. callers can interact with them directly if > desired. Leaving remove_system_webview here allows us to maintain that parity. OK. Keeping these as-is sgtm.
Testing one final time locally before committing. https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... File devil/devil/android/tools/provision_devices.py (right): https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:360: logger.info('System app %s already removed', path) On 2017/03/08 10:19:04, perezju wrote: > This might be a bit too log-spammy, particularly because apps tend to be found > either on 'app' or 'priv-app' but not both; so we'll get slightly misleading > notes about a "system app FOO already removed" that wasn't actually removed (it > was never there!). > > I would suggest instead: loop over each system_app to remove, collect into a > list of found_paths when the app is found in any of the _SYSTEM_APP_DIRECTORIES, > if it isn't found in any of them, then log a warning. Finally, call > device.RemovePath(found_paths, force=True, recursive=True) once. Done. https://codereview.chromium.org/2734023002/diff/80001/devil/devil/android/too... devil/devil/android/tools/provision_devices.py:388: 'Cannot remove system apps from non-rooted device', str(device)) On 2017/03/08 10:19:04, perezju wrote: > nit: 'Cannot remove' -> 'Failed to remove' Done.
The CQ bit was checked by bsheedy@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/2734023002/#ps100001 (title: "Address perezju@ comments")
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": 1488996533350730,
"parent_rev": "d32fc8b4320331ecca29ce180cba2b95ab21374c", "commit_rev":
"eaf9a11d28212ac48b5bfd16c4529ec5756afc2f"}
Message was sent while issue was closed.
Description was changed from ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-apps argument. Tested by running the following command on a newly reflashed Pixel and 5X and verifying the log output + the contents of /system/app: provision_devices.py -d <device> --remove-system-webview \ --remove-system-apps GoogleVrCore NotARealSystemApp -v -v -v BUG=chromium:671373, catapult:#3341 ========== to ========== Generalize the removal of system apps Generalizes the approach used by RemoveSystemWebView to allow the removal of arbitrary system apps via the --remove-system-apps argument. Tested by running the following command on a newly reflashed Pixel and 5X and verifying the log output + the contents of /system/app: provision_devices.py -d <device> --remove-system-webview \ --remove-system-apps GoogleVrCore NotARealSystemApp -v -v -v BUG=chromium:671373, catapult:#3341 Review-Url: https://codereview.chromium.org/2734023002 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... |
