Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1234153004:
Cache device apk checksums in device_utils.py (Closed)
|
Created:
5 years, 5 months ago by agrieve Modified:
5 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@gtest-fast Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache device apk checksums in device_utils.py
On my machine, this brings the time to run:
bin/run_base_unittests --gtest-filter PathUtilsTest.TestGetDataDirectory
from ~18 seconds to ~15 seconds
BUG=
Committed: https://crrev.com/69a68c76ec6d27e8b27eeb0851bbc3247b700a3b
Cr-Commit-Position: refs/heads/master@{#342860}
Patch Set 1 #
Total comments: 6
Patch Set 2 : hook uninstall #
Total comments: 6
Patch Set 3 : address review comments #
Total comments: 4
Patch Set 4 : add Uninstall() #
Total comments: 11
Patch Set 5 : some style nits #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (3 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
Come up with this while I was looking at why it was so slow on my machine. Would like to know if there are additional trybots this should be tried on since it's not really my area of expertise...
jbudorick@chromium.org changed reviewers: + perezju@chromium.org
This CL worries me for a few reasons: - stale cache data. Not everything is managed through the DeviceUtils interface, nor do we require it to be. It's perfectly legal for a user to install something via DeviceUtils.Install, then uninstall it via AdbWrapper.Uninstall. - relatively large complexity for a relatively small performance gain - As perezju can attest, I _really_ dislike adding flags to DeviceUtils's public API. The first one is the biggest concern, as it's far more important for DeviceUtils to be correct than it is for it to be performant. That's not to say that the latter isn't important, but it doesn't matter if we save a little time if we introduce flakiness to do so. https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:179: self._package_to_device_apk_checksums_cache = {} This should use self._cache, which we already have some lifetime control for, rather than using its own dict -- something like self._cache['package_checksums'] = {} and self._cache['package_paths'] = {} https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:349: skip_cache=False): Do we envision external clients using this, or does this option exist solely for the pm_ready check? If the latter, perhaps we should expose this option in a private method that gets called by the public one. If this sticks around, it needs to be: - before timeout and retries - documented https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:577: def _Uninstall(self, package_name): This should _not_ be named "Uninstall". As-is, it's liable to confuse a user who looks in here and thinks "why is this private?" instead of "oh, uninstall is provided by AdbWrapper"
On 2015/07/16 at 16:03:03, agrieve wrote: > Come up with this while I was looking at why it was so slow on my machine. Would like to know if there are additional trybots this should be tried on since it's not really my area of expertise... As for trybots: linux_android_rel_ng is the primary tester trybot. I usually throw DeviceUtils changes at it multiple times, and I added 5 runs to this CL.
On 2015/07/16 at 16:32:21, jbudorick wrote: > On 2015/07/16 at 16:03:03, agrieve wrote: > > Come up with this while I was looking at why it was so slow on my machine. Would like to know if there are additional trybots this should be tried on since it's not really my area of expertise... > > As for trybots: linux_android_rel_ng is the primary tester trybot. I usually throw DeviceUtils changes at it multiple times, and I added 5 runs to this CL. one last thing: if you're interested in what's going on under the hood, tack -vv onto the runner script, e.g. out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* the first v will give you all of the adb commands it's running, and the second will give you a variety of other things.
Had the same concerns about Uninstall being missing / cache somehow becoming out-of-date. Still, multiple seconds is a nice savings when trying to iterate locally on a test. I've changed it around to wrap self.adb.Uninstall() in the hopes that it'll be more robust (although clearly uglier). https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:179: self._package_to_device_apk_checksums_cache = {} On 2015/07/16 16:31:33, jbudorick wrote: > This should use self._cache, which we already have some lifetime control for, > rather than using its own dict -- something like > self._cache['package_checksums'] = {} and self._cache['package_paths'] = {} I think these caches are better split out since their freshness are tied to package installs rather than the device itself. https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:349: skip_cache=False): On 2015/07/16 16:31:33, jbudorick wrote: > Do we envision external clients using this, or does this option exist solely for > the pm_ready check? If the latter, perhaps we should expose this option in a > private method that gets called by the public one. > > If this sticks around, it needs to be: > - before timeout and retries > - documented Made it an internal helper. https://codereview.chromium.org/1234153004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:577: def _Uninstall(self, package_name): On 2015/07/16 16:31:32, jbudorick wrote: > This should _not_ be named "Uninstall". As-is, it's liable to confuse a user who > looks in here and thinks "why is this private?" instead of "oh, uninstall is > provided by AdbWrapper" Deleted in favour of hooking self.adb.Uninstall
On 2015/07/16 16:53:52, jbudorick wrote: > On 2015/07/16 at 16:32:21, jbudorick wrote: > > On 2015/07/16 at 16:03:03, agrieve wrote: > > > Come up with this while I was looking at why it was so slow on my machine. > Would like to know if there are additional trybots this should be tried on since > it's not really my area of expertise... > > > > As for trybots: linux_android_rel_ng is the primary tester trybot. I usually > throw DeviceUtils changes at it multiple times, and I added 5 runs to this CL. > > one last thing: if you're interested in what's going on under the hood, tack -vv > onto the runner script, e.g. > > out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* > > the first v will give you all of the adb commands it's running, and the second > will give you a variety of other things. Yaron showed me this yesterday, and definitely was super helpful to see what was going on (seems Install is called 3 times for one test). Seems we could probably save a bit by batching more adb shell commands together, but the next longest thing is definitely the actual adb install call. Next big improvement would probably be from doing a gyp_managed_install-style thing for local dev. so the install can be avoided and the .so just updated. I'll be working on adding gyp_managed_install to GN in the next few weeks, so I'll keep an eye out for doing this with tests as well.
On 2015/07/16 at 19:04:39, agrieve wrote: > On 2015/07/16 16:53:52, jbudorick wrote: > > On 2015/07/16 at 16:32:21, jbudorick wrote: > > > On 2015/07/16 at 16:03:03, agrieve wrote: > > > > Come up with this while I was looking at why it was so slow on my machine. > > Would like to know if there are additional trybots this should be tried on since > > it's not really my area of expertise... > > > > > > As for trybots: linux_android_rel_ng is the primary tester trybot. I usually > > throw DeviceUtils changes at it multiple times, and I added 5 runs to this CL. > > > > one last thing: if you're interested in what's going on under the hood, tack -vv > > onto the runner script, e.g. > > > > out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* > > > > the first v will give you all of the adb commands it's running, and the second > > will give you a variety of other things. > > Yaron showed me this yesterday, and definitely was super helpful to see what was going on (seems Install is called 3 times for one test). Seems we could probably save a bit by batching more adb shell commands together, but the next longest thing is definitely the actual adb install call. Next big improvement would probably be from doing a gyp_managed_install-style thing for local dev. so the install can be avoided and the .so just updated. I'll be working on adding gyp_managed_install to GN in the next few weeks, so I'll keep an eye out for doing this with tests as well. re batching adb commands - we do this in some places (e.g. md5sum: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...) but it's tricky and non-portable enough that I don't recommend it in most cases.
On 2015/07/16 19:11:22, jbudorick wrote: > On 2015/07/16 at 19:04:39, agrieve wrote: > > On 2015/07/16 16:53:52, jbudorick wrote: > > > On 2015/07/16 at 16:32:21, jbudorick wrote: > > > > On 2015/07/16 at 16:03:03, agrieve wrote: > > > > > Come up with this while I was looking at why it was so slow on my > machine. > > > Would like to know if there are additional trybots this should be tried on > since > > > it's not really my area of expertise... > > > > > > > > As for trybots: linux_android_rel_ng is the primary tester trybot. I > usually > > > throw DeviceUtils changes at it multiple times, and I added 5 runs to this > CL. > > > > > > one last thing: if you're interested in what's going on under the hood, tack > -vv > > > onto the runner script, e.g. > > > > > > out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* > > > > > > the first v will give you all of the adb commands it's running, and the > second > > > will give you a variety of other things. > > > > Yaron showed me this yesterday, and definitely was super helpful to see what > was going on (seems Install is called 3 times for one test). Seems we could > probably save a bit by batching more adb shell commands together, but the next > longest thing is definitely the actual adb install call. Next big improvement > would probably be from doing a gyp_managed_install-style thing for local dev. so > the install can be avoided and the .so just updated. I'll be working on adding > gyp_managed_install to GN in the next few weeks, so I'll keep an eye out for > doing this with tests as well. > > re batching adb commands - we do this in some places (e.g. md5sum: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...) > but it's tricky and non-portable enough that I don't recommend it in most cases. Sorry - was out last week. Just triggered some more try jobs for the updated patch.
On 2015/07/28 14:19:05, agrieve wrote: > On 2015/07/16 19:11:22, jbudorick wrote: > > On 2015/07/16 at 19:04:39, agrieve wrote: > > > On 2015/07/16 16:53:52, jbudorick wrote: > > > > On 2015/07/16 at 16:32:21, jbudorick wrote: > > > > > On 2015/07/16 at 16:03:03, agrieve wrote: > > > > > > Come up with this while I was looking at why it was so slow on my > > machine. > > > > Would like to know if there are additional trybots this should be tried on > > since > > > > it's not really my area of expertise... > > > > > > > > > > As for trybots: linux_android_rel_ng is the primary tester trybot. I > > usually > > > > throw DeviceUtils changes at it multiple times, and I added 5 runs to this > > CL. > > > > > > > > one last thing: if you're interested in what's going on under the hood, > tack > > -vv > > > > onto the runner script, e.g. > > > > > > > > out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* > > > > > > > > the first v will give you all of the adb commands it's running, and the > > second > > > > will give you a variety of other things. > > > > > > Yaron showed me this yesterday, and definitely was super helpful to see what > > was going on (seems Install is called 3 times for one test). Seems we could > > probably save a bit by batching more adb shell commands together, but the next > > longest thing is definitely the actual adb install call. Next big improvement > > would probably be from doing a gyp_managed_install-style thing for local dev. > so > > the install can be avoided and the .so just updated. I'll be working on adding > > gyp_managed_install to GN in the next few weeks, so I'll keep an eye out for > > doing this with tests as well. > > > > re batching adb commands - we do this in some places (e.g. md5sum: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...) > > but it's tricky and non-portable enough that I don't recommend it in most > cases. > > Sorry - was out last week. > > Just triggered some more try jobs for the updated patch. bump
On 2015/07/31 15:50:10, agrieve wrote: > On 2015/07/28 14:19:05, agrieve wrote: > > On 2015/07/16 19:11:22, jbudorick wrote: > > > On 2015/07/16 at 19:04:39, agrieve wrote: > > > > On 2015/07/16 16:53:52, jbudorick wrote: > > > > > On 2015/07/16 at 16:32:21, jbudorick wrote: > > > > > > On 2015/07/16 at 16:03:03, agrieve wrote: > > > > > > > Come up with this while I was looking at why it was so slow on my > > > machine. > > > > > Would like to know if there are additional trybots this should be tried > on > > > since > > > > > it's not really my area of expertise... > > > > > > > > > > > > As for trybots: linux_android_rel_ng is the primary tester trybot. I > > > usually > > > > > throw DeviceUtils changes at it multiple times, and I added 5 runs to > this > > > CL. > > > > > > > > > > one last thing: if you're interested in what's going on under the hood, > > tack > > > -vv > > > > > onto the runner script, e.g. > > > > > > > > > > out/Debug/bin/run_base_unittests -vv --gtest-filter PathUtilsTest.* > > > > > > > > > > the first v will give you all of the adb commands it's running, and the > > > second > > > > > will give you a variety of other things. > > > > > > > > Yaron showed me this yesterday, and definitely was super helpful to see > what > > > was going on (seems Install is called 3 times for one test). Seems we could > > > probably save a bit by batching more adb shell commands together, but the > next > > > longest thing is definitely the actual adb install call. Next big > improvement > > > would probably be from doing a gyp_managed_install-style thing for local > dev. > > so > > > the install can be avoided and the .so just updated. I'll be working on > adding > > > gyp_managed_install to GN in the next few weeks, so I'll keep an eye out for > > > doing this with tests as well. > > > > > > re batching adb commands - we do this in some places (e.g. md5sum: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...) > > > but it's tricky and non-portable enough that I don't recommend it in most > > cases. > > > > Sorry - was out last week. > > > > Just triggered some more try jobs for the updated patch. > > bump bump
https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:195: self._package_to_device_apk_paths_cache[package_name] = [] I would still prefer to use self._cache and do some additional cache management for this key. https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:591: self._package_to_device_apk_paths_cache.pop(package_name, 0) Do you need to do this? If we hit an exception in InstallMultiple, we won't have put the package_name entry back in yet. https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1070: def _ComputeStaleApks(self, package_name, host_apk_paths): I'm curious about the component time savings of caching the application path & of caching the checksum. I don't particularly like replicating the GetChangedFiles logic, and if this isn't saving very much time I'd rather not add it. Alternatively, maybe we should be doing some amount of caching like this more broadly.
https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:195: self._package_to_device_apk_paths_cache[package_name] = [] On 2015/08/06 19:28:22, jbudorick wrote: > I would still prefer to use self._cache and do some additional cache management > for this key. Done. https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:591: self._package_to_device_apk_paths_cache.pop(package_name, 0) On 2015/08/06 19:28:22, jbudorick wrote: > Do you need to do this? If we hit an exception in InstallMultiple, we won't have > put the package_name entry back in yet. Nice catch. Gone. https://codereview.chromium.org/1234153004/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1070: def _ComputeStaleApks(self, package_name, host_apk_paths): On 2015/08/06 19:28:22, jbudorick wrote: > I'm curious about the component time savings of caching the application path & > of caching the checksum. I don't particularly like replicating the > GetChangedFiles logic, and if this isn't saving very much time I'd rather not > add it. > > Alternatively, maybe we should be doing some amount of caching like this more > broadly. When I first started down this path, I tried to add caching to _GetChangedAndStaleFiles but hit a wall. The fundamental difference is that there's no way to (quickly) know the on-device file name of an apk given the host apk. That's why in InstallSplitApk, it checks whether _any_ of the on-device .apk checksums match a host checksum to know whether or not the file is stale. This approach was already used by InstallSplitApk, but now is a bit more formalized in these functions (with caching)
https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:198: old_adb_uninstall = self.adb.Uninstall urgh, cache invalidation. I don't like this _at all_. It makes calls to AdbWrapper.Uninstall much, much harder to read, and it starts down a horrible rabbit hole for how DeviceUtils interacts with AdbWrapper.
https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:198: old_adb_uninstall = self.adb.Uninstall On 2015/08/11 14:32:27, jbudorick wrote: > urgh, cache invalidation. > > I don't like this _at all_. It makes calls to AdbWrapper.Uninstall much, much > harder to read, and it starts down a horrible rabbit hole for how DeviceUtils > interacts with AdbWrapper. Don't love it either, but having a callback that adbwrapper fires upon uninstall isn't a tonne better. DeviceUtils vs AdbWrapper is a bit of a tough abstraction if you allow clients to call through to AdbWrapper directly. Any of the caching that it provides (e.g. GetProp) can become invalid if someone calls through to AdbWrapper directly. Maybe let's expose an Uninstall() method and state that it's a violation to call AdbWrapper.Uninstall directly?
https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:198: old_adb_uninstall = self.adb.Uninstall On 2015/08/11 at 14:48:34, agrieve wrote: > On 2015/08/11 14:32:27, jbudorick wrote: > > urgh, cache invalidation. > > > > I don't like this _at all_. It makes calls to AdbWrapper.Uninstall much, much > > harder to read, and it starts down a horrible rabbit hole for how DeviceUtils > > interacts with AdbWrapper. > > Don't love it either, but having a callback that adbwrapper fires upon uninstall isn't a tonne better. > > DeviceUtils vs AdbWrapper is a bit of a tough abstraction if you allow clients to call through to AdbWrapper directly. Any of the caching that it provides (e.g. GetProp) can become invalid if someone calls through to AdbWrapper directly. Maybe let's expose an Uninstall() method and state that it's a violation to call AdbWrapper.Uninstall directly? Inviolating the properties part of the cache involves manually calling RunShellCommand(['setprop', ...]) or the AdbWrapper equivalent, which I would certainly consider a violation. I'm ok with the idea of a DeviceUtils.Uninstall. I think the violation would be in mixing calls to DeviceUtils.Install and AdbWrapper.Uninstall (or vice versa). Consistently using one or the other should be fine.
On 2015/08/11 14:53:19, jbudorick wrote: > https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:198: old_adb_uninstall = > self.adb.Uninstall > On 2015/08/11 at 14:48:34, agrieve wrote: > > On 2015/08/11 14:32:27, jbudorick wrote: > > > urgh, cache invalidation. > > > > > > I don't like this _at all_. It makes calls to AdbWrapper.Uninstall much, > much > > > harder to read, and it starts down a horrible rabbit hole for how > DeviceUtils > > > interacts with AdbWrapper. > > > > Don't love it either, but having a callback that adbwrapper fires upon > uninstall isn't a tonne better. > > > > DeviceUtils vs AdbWrapper is a bit of a tough abstraction if you allow clients > to call through to AdbWrapper directly. Any of the caching that it provides > (e.g. GetProp) can become invalid if someone calls through to AdbWrapper > directly. Maybe let's expose an Uninstall() method and state that it's a > violation to call AdbWrapper.Uninstall directly? > > Inviolating the properties part of the cache involves manually calling > RunShellCommand(['setprop', ...]) or the AdbWrapper equivalent, which I would > certainly consider a violation. > > I'm ok with the idea of a DeviceUtils.Uninstall. I think the violation would be > in mixing calls to DeviceUtils.Install and AdbWrapper.Uninstall (or vice versa). > Consistently using one or the other should be fine. Done.
https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:198: old_adb_uninstall = self.adb.Uninstall On 2015/08/11 at 14:53:19, jbudorick wrote: > On 2015/08/11 at 14:48:34, agrieve wrote: > > On 2015/08/11 14:32:27, jbudorick wrote: > > > urgh, cache invalidation. > > > > > > I don't like this _at all_. It makes calls to AdbWrapper.Uninstall much, much > > > harder to read, and it starts down a horrible rabbit hole for how DeviceUtils > > > interacts with AdbWrapper. > > > > Don't love it either, but having a callback that adbwrapper fires upon uninstall isn't a tonne better. > > > > DeviceUtils vs AdbWrapper is a bit of a tough abstraction if you allow clients to call through to AdbWrapper directly. Any of the caching that it provides (e.g. GetProp) can become invalid if someone calls through to AdbWrapper directly. Maybe let's expose an Uninstall() method and state that it's a violation to call AdbWrapper.Uninstall directly? > > Inviolating the properties part of the cache involves manually calling RunShellCommand(['setprop', ...]) or the AdbWrapper equivalent, which I would certainly consider a violation. s/inviolating/invalidating/ (Surprisingly, inviolate is a word, though it's not the one I wanted.) > > I'm ok with the idea of a DeviceUtils.Uninstall. I think the violation would be in mixing calls to DeviceUtils.Install and AdbWrapper.Uninstall (or vice versa). Consistently using one or the other should be fine.
https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:528: if should_install: nit: I know this is how it was before, but can you add a line break before this line? https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:575: if apks_to_install: nit: same https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:603: except: Is there really a functional difference between these two? Can we just handle removing package_name from the caches in a finally block? https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1085: if ret is None: Does this work in the Uninstall case if you've set _cache['package_apk_checksums'][package_name] = set()? https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1839: } nit: don't indent the closing brace
https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:528: if should_install: On 2015/08/11 15:21:04, jbudorick wrote: > nit: I know this is how it was before, but can you add a line break before this > line? Done. https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:575: if apks_to_install: On 2015/08/11 15:21:04, jbudorick wrote: > nit: same Done. https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:603: except: On 2015/08/11 15:21:04, jbudorick wrote: > Is there really a functional difference between these two? Can we just handle > removing package_name from the caches in a finally block? There is. In the first case, we're caching the fact that the apks are not there, in the latter case, we're removing the cache entry since we have not idea if they are there or not. https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1085: if ret is None: On 2015/08/11 15:21:04, jbudorick wrote: > Does this work in the Uninstall case if you've set > _cache['package_apk_checksums'][package_name] = set()? Yes. set() is None == False https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1839: } On 2015/08/11 15:21:04, jbudorick wrote: > nit: don't indent the closing brace Done.
https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1085: if ret is None: On 2015/08/11 at 15:26:08, agrieve wrote: > On 2015/08/11 15:21:04, jbudorick wrote: > > Does this work in the Uninstall case if you've set > > _cache['package_apk_checksums'][package_name] = set()? > > Yes. set() is None == False er... I'm pretty sure that's wrong. None is the singleton NoneType object. set() is an empty set. set() will evaluate to False (i.e., this would work as "if not ret"), but it isn't None.
On 2015/08/11 15:29:53, jbudorick wrote: > https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:1085: if ret is None: > On 2015/08/11 at 15:26:08, agrieve wrote: > > On 2015/08/11 15:21:04, jbudorick wrote: > > > Does this work in the Uninstall case if you've set > > > _cache['package_apk_checksums'][package_name] = set()? > > > > Yes. set() is None == False > > er... I'm pretty sure that's wrong. None is the singleton NoneType object. set() > is an empty set. set() will evaluate to False (i.e., this would work as "if not > ret"), but it isn't None. Right. That's what I want (when putting set() in the map, I'm caching the fact that there are no checksums on the device).
On 2015/08/11 at 15:35:39, agrieve wrote: > On 2015/08/11 15:29:53, jbudorick wrote: > > https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... > > File build/android/pylib/device/device_utils.py (right): > > > > https://codereview.chromium.org/1234153004/diff/60001/build/android/pylib/dev... > > build/android/pylib/device/device_utils.py:1085: if ret is None: > > On 2015/08/11 at 15:26:08, agrieve wrote: > > > On 2015/08/11 15:21:04, jbudorick wrote: > > > > Does this work in the Uninstall case if you've set > > > > _cache['package_apk_checksums'][package_name] = set()? > > > > > > Yes. set() is None == False > > > > er... I'm pretty sure that's wrong. None is the singleton NoneType object. set() > > is an empty set. set() will evaluate to False (i.e., this would work as "if not > > ret"), but it isn't None. > > Right. That's what I want (when putting set() in the map, I'm caching the fact that there are no checksums on the device). Oh, ok. I misunderstood what you were trying to do here.
lgtm
On 2015/08/11 at 16:01:26, jbudorick wrote: > lgtm Also: thanks!
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234153004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234153004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/69a68c76ec6d27e8b27eeb0851bbc3247b700a3b Cr-Commit-Position: refs/heads/master@{#342860} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
