Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1167693002:
remove stale test data on the device (Closed)
|
Created:
5 years, 6 months ago by Menglin Modified:
5 years, 5 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionremove stale test data on the device
BUG=457904
Committed: https://crrev.com/d7f9fe973863ca93337991513c754a28299b152d
Cr-Commit-Position: refs/heads/master@{#334438}
Patch Set 1 #
Total comments: 10
Patch Set 2 : remove stale data from device #
Total comments: 14
Patch Set 3 : remove stale files on device #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 7
Patch Set 6 : #
Messages
Total messages: 24 (4 generated)
mlliu@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Hi John and Mike, I'd like you to review this bug fix. Thank you Menglin
https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, This function has some significant issues: - it duplicates too much code from _GetChangedFiles - it isn't actually deleting anything - it's modifying one of its arguments in-place with the intention of the caller both knowing and using the modified argument _GetChangedFiles currently returns a list of (host_file, device_file) pairs for each file that exists on the host that should be pushed to the device. This can include both newly added files and changed files. Perhaps we could change what it returns to somehow also capture what isn't present on the host?
Hi John, please comment on the following Menglin https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/02 15:30:50, jbudorick wrote: > This function has some significant issues: > - it duplicates too much code from _GetChangedFiles > - it isn't actually deleting anything > - it's modifying one of its arguments in-place with the intention of the caller > both knowing and using the modified argument > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs for > each file that exists on the host that should be pushed to the device. This can > include both newly added files and changed files. Perhaps we could change what > it returns to somehow also capture what isn't present on the host? device_checksums is calculated in each _GetChangedFiles because it takes each (host, device) pair as the arguments. But it is not the case in _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again in the function. So _GetChangedFiles' arguments and returns need to be changed, i.e., add a device_checksums=None as the argument, return (push_list, delete_list) as a tuple.
https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/02 at 16:31:44, Menglin wrote: > On 2015/06/02 15:30:50, jbudorick wrote: > > This function has some significant issues: > > - it duplicates too much code from _GetChangedFiles > > - it isn't actually deleting anything > > - it's modifying one of its arguments in-place with the intention of the caller > > both knowing and using the modified argument > > > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs for > > each file that exists on the host that should be pushed to the device. This can > > include both newly added files and changed files. Perhaps we could change what > > it returns to somehow also capture what isn't present on the host? > > device_checksums is calculated in each _GetChangedFiles because it takes each (host, device) pair as the arguments. But it is not the case in _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again in the function. So _GetChangedFiles' arguments and returns need to be changed, i.e., add a device_checksums=None as the argument, return (push_list, delete_list) as a tuple. I'm not sure about that particular implementation, but I'm fine with changing what _GetChangedFiles returns. Maybe instead of just a list of things to push, it returns files and hashes in cases where the two don't match, e.g. [ { 'host': { 'path': '/path/to/host/file', 'hash': '0123456789abcdeffedcba9876543210', }, 'device': None, }, { 'host': None, 'device': { 'path': '/path/to/device/file', 'hash': '0123456789abcdeffedcba9876543210', }, }, # etc ] https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils_real_test.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils_real_test.py:8: The test will invoke real devices Maybe "device_utils_device_test.py" ?
jbudorick@chromium.org changed reviewers: + perezju@chromium.org
+perezju, in case you have any thoughts on how to do this
https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/08 18:34:29, jbudorick wrote: > On 2015/06/02 at 16:31:44, Menglin wrote: > > On 2015/06/02 15:30:50, jbudorick wrote: > > > This function has some significant issues: > > > - it duplicates too much code from _GetChangedFiles > > > - it isn't actually deleting anything > > > - it's modifying one of its arguments in-place with the intention of the > caller > > > both knowing and using the modified argument > > > > > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs > for > > > each file that exists on the host that should be pushed to the device. This > can > > > include both newly added files and changed files. Perhaps we could change > what > > > it returns to somehow also capture what isn't present on the host? > > > > device_checksums is calculated in each _GetChangedFiles because it takes each > (host, device) pair as the arguments. But it is not the case in > _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before > _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again > in the function. So _GetChangedFiles' arguments and returns need to be changed, > i.e., add a device_checksums=None as the argument, return (push_list, > delete_list) as a tuple. > > I'm not sure about that particular implementation, but I'm fine with changing > what _GetChangedFiles returns. Maybe instead of just a list of things to push, > it returns files and hashes in cases where the two don't match, e.g. > > [ > { > 'host': { > 'path': '/path/to/host/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > 'device': None, > }, > { > 'host': None, > 'device': { > 'path': '/path/to/device/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > }, > # etc > ] I had a good thought about this. First, I'm not sure whether we want to have a new function PushAndDeleteFiles or just a 'delete_device_stale=True/False' option on 'PushChangedFiles'. In either case, I think the cleanest approach would be to have a new _GetChangedAndStaleFiles(host_path, device_path) which replaces _GetChangedFiles and returns two lists: (file_tuples_to_push, device_stale_files). I also think the logic would be simplified if the implementation starts with an if, branching for two different implementations whether host_path is a file or a directory (the list of device_stale_files should only be populated in the later case).
On 2015/06/09 13:07:26, perezju wrote: > https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... > build/android/pylib/device/device_utils.py:957: def > _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, > On 2015/06/08 18:34:29, jbudorick wrote: > > On 2015/06/02 at 16:31:44, Menglin wrote: > > > On 2015/06/02 15:30:50, jbudorick wrote: > > > > This function has some significant issues: > > > > - it duplicates too much code from _GetChangedFiles > > > > - it isn't actually deleting anything > > > > - it's modifying one of its arguments in-place with the intention of the > > caller > > > > both knowing and using the modified argument > > > > > > > > _GetChangedFiles currently returns a list of (host_file, device_file) > pairs > > for > > > > each file that exists on the host that should be pushed to the device. > This > > can > > > > include both newly added files and changed files. Perhaps we could change > > what > > > > it returns to somehow also capture what isn't present on the host? > > > > > > device_checksums is calculated in each _GetChangedFiles because it takes > each > > (host, device) pair as the arguments. But it is not the case in > > _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before > > _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating > again > > in the function. So _GetChangedFiles' arguments and returns need to be > changed, > > i.e., add a device_checksums=None as the argument, return (push_list, > > delete_list) as a tuple. > > > > I'm not sure about that particular implementation, but I'm fine with changing > > what _GetChangedFiles returns. Maybe instead of just a list of things to push, > > it returns files and hashes in cases where the two don't match, e.g. > > > > [ > > { > > 'host': { > > 'path': '/path/to/host/file', > > 'hash': '0123456789abcdeffedcba9876543210', > > }, > > 'device': None, > > }, > > { > > 'host': None, > > 'device': { > > 'path': '/path/to/device/file', > > 'hash': '0123456789abcdeffedcba9876543210', > > }, > > }, > > # etc > > ] > > I had a good thought about this. > > First, I'm not sure whether we want to have a new function PushAndDeleteFiles or > just a 'delete_device_stale=True/False' option on 'PushChangedFiles'. > > In either case, I think the cleanest approach would be to have a new > _GetChangedAndStaleFiles(host_path, device_path) which replaces _GetChangedFiles > and returns two lists: (file_tuples_to_push, device_stale_files). > > I also think the logic would be simplified if the implementation starts with an > if, branching for two different implementations whether host_path is a file or a > directory (the list of device_stale_files should only be populated in the later > case). Changes: 1. Keep only one PushChangedFiles, add a delete_device_stale default as False option 2. _GetChangedAndStaleFiles replaces _GetChangedFiles, which returns (file_tuple_list_to_push, device_stale_file_list)
Hi John, Mike, and Juan, I've uploaded code change. Please review it Thank you Menglin https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/02 15:30:50, jbudorick wrote: > This function has some significant issues: > - it duplicates too much code from _GetChangedFiles > - it isn't actually deleting anything > - it's modifying one of its arguments in-place with the intention of the caller > both knowing and using the modified argument > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs for > each file that exists on the host that should be pushed to the device. This can > include both newly added files and changed files. Perhaps we could change what > it returns to somehow also capture what isn't present on the host? Acknowledged. https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/08 18:34:29, jbudorick wrote: > On 2015/06/02 at 16:31:44, Menglin wrote: > > On 2015/06/02 15:30:50, jbudorick wrote: > > > This function has some significant issues: > > > - it duplicates too much code from _GetChangedFiles > > > - it isn't actually deleting anything > > > - it's modifying one of its arguments in-place with the intention of the > caller > > > both knowing and using the modified argument > > > > > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs > for > > > each file that exists on the host that should be pushed to the device. This > can > > > include both newly added files and changed files. Perhaps we could change > what > > > it returns to somehow also capture what isn't present on the host? > > > > device_checksums is calculated in each _GetChangedFiles because it takes each > (host, device) pair as the arguments. But it is not the case in > _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before > _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again > in the function. So _GetChangedFiles' arguments and returns need to be changed, > i.e., add a device_checksums=None as the argument, return (push_list, > delete_list) as a tuple. > > I'm not sure about that particular implementation, but I'm fine with changing > what _GetChangedFiles returns. Maybe instead of just a list of things to push, > it returns files and hashes in cases where the two don't match, e.g. > > [ > { > 'host': { > 'path': '/path/to/host/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > 'device': None, > }, > { > 'host': None, > 'device': { > 'path': '/path/to/device/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > }, > # etc > ] Done. https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/08 18:34:29, jbudorick wrote: > On 2015/06/02 at 16:31:44, Menglin wrote: > > On 2015/06/02 15:30:50, jbudorick wrote: > > > This function has some significant issues: > > > - it duplicates too much code from _GetChangedFiles > > > - it isn't actually deleting anything > > > - it's modifying one of its arguments in-place with the intention of the > caller > > > both knowing and using the modified argument > > > > > > _GetChangedFiles currently returns a list of (host_file, device_file) pairs > for > > > each file that exists on the host that should be pushed to the device. This > can > > > include both newly added files and changed files. Perhaps we could change > what > > > it returns to somehow also capture what isn't present on the host? > > > > device_checksums is calculated in each _GetChangedFiles because it takes each > (host, device) pair as the arguments. But it is not the case in > _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before > _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again > in the function. So _GetChangedFiles' arguments and returns need to be changed, > i.e., add a device_checksums=None as the argument, return (push_list, > delete_list) as a tuple. > > I'm not sure about that particular implementation, but I'm fine with changing > what _GetChangedFiles returns. Maybe instead of just a list of things to push, > it returns files and hashes in cases where the two don't match, e.g. > > [ > { > 'host': { > 'path': '/path/to/host/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > 'device': None, > }, > { > 'host': None, > 'device': { > 'path': '/path/to/device/file', > 'hash': '0123456789abcdeffedcba9876543210', > }, > }, > # etc > ] Done. https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:957: def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path, On 2015/06/09 13:07:26, perezju wrote: > On 2015/06/08 18:34:29, jbudorick wrote: > > On 2015/06/02 at 16:31:44, Menglin wrote: > > > On 2015/06/02 15:30:50, jbudorick wrote: > > > > This function has some significant issues: > > > > - it duplicates too much code from _GetChangedFiles > > > > - it isn't actually deleting anything > > > > - it's modifying one of its arguments in-place with the intention of the > > caller > > > > both knowing and using the modified argument > > > > > > > > _GetChangedFiles currently returns a list of (host_file, device_file) > pairs > > for > > > > each file that exists on the host that should be pushed to the device. > This > > can > > > > include both newly added files and changed files. Perhaps we could change > > what > > > > it returns to somehow also capture what isn't present on the host? > > > > > > device_checksums is calculated in each _GetChangedFiles because it takes > each > > (host, device) pair as the arguments. But it is not the case in > > _GetChangedFilesRemoveStaleFiles. device_checksums is calculated before > > _GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating > again > > in the function. So _GetChangedFiles' arguments and returns need to be > changed, > > i.e., add a device_checksums=None as the argument, return (push_list, > > delete_list) as a tuple. > > > > I'm not sure about that particular implementation, but I'm fine with changing > > what _GetChangedFiles returns. Maybe instead of just a list of things to push, > > it returns files and hashes in cases where the two don't match, e.g. > > > > [ > > { > > 'host': { > > 'path': '/path/to/host/file', > > 'hash': '0123456789abcdeffedcba9876543210', > > }, > > 'device': None, > > }, > > { > > 'host': None, > > 'device': { > > 'path': '/path/to/device/file', > > 'hash': '0123456789abcdeffedcba9876543210', > > }, > > }, > > # etc > > ] > > I had a good thought about this. > > First, I'm not sure whether we want to have a new function PushAndDeleteFiles or > just a 'delete_device_stale=True/False' option on 'PushChangedFiles'. > > In either case, I think the cleanest approach would be to have a new > _GetChangedAndStaleFiles(host_path, device_path) which replaces _GetChangedFiles > and returns two lists: (file_tuples_to_push, device_stale_files). > > I also think the logic would be simplified if the implementation starts with an > if, branching for two different implementations whether host_path is a file or a > directory (the list of device_stale_files should only be populated in the later > case). Done. https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils_real_test.py (right): https://codereview.chromium.org/1167693002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils_real_test.py:8: The test will invoke real devices On 2015/06/08 18:34:29, jbudorick wrote: > Maybe "device_utils_device_test.py" > > ? Done.
Thanks for the changes. I think the implementation is looking a lot better! https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/bas... File build/android/pylib/base/base_setup.py (right): https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/bas... build/android/pylib/base/base_setup.py:69: for p in os.listdir(constants.ISOLATE_DEPS_DIR)]) why not replace the whole thing with just: device.PushChangedFiles( [(constants.ISOLATE_DEPS_DIR, device_dir)], delete_device_stale=test_options.delete_stale_data) https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:817: If delete_device_stale option is True, delete stale files on device. Maybe explain in a bit more detail: When pushing a directory, it is traversed recursively on the host and all files found are pushed to the device as needed. Additionally, if delete_device_stale option is True, files on the device that don't exist on the host are deleted. (also nit: add space between title doc line and additional notes.) https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:841: stale_files_on_device += stale_files nit: maybe rename these variables to something like changed_files/stale_files for the local variable within each iteration, and all_changed_files/all_stale_files for the aggregates. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:895: del device_checksums[device_abs_path] I think you can replace the last three lines with: device_checksum = device_checksums.pop(device_abs_path, None) https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:896: if ((device_checksum) != host_checksum): nit: drop all the parentheses https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:898: to_delete = [p for p in device_checksums.iterkeys()] nit: to_delete = device_checksums.keys() https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:934: self.RunShellCommand(['rm', stale_file_path]) do we really need this method? maybe just inline within PushChangedFiles
Hi John, Mike and Juan, I uploaded the code change again. Please review it. Thank you Menglin https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/bas... File build/android/pylib/base/base_setup.py (right): https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/bas... build/android/pylib/base/base_setup.py:69: for p in os.listdir(constants.ISOLATE_DEPS_DIR)]) On 2015/06/10 09:24:22, perezju wrote: > why not replace the whole thing with just: > > device.PushChangedFiles( > [(constants.ISOLATE_DEPS_DIR, device_dir)], > delete_device_stale=test_options.delete_stale_data) Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:817: If delete_device_stale option is True, delete stale files on device. On 2015/06/10 09:24:23, perezju wrote: > Maybe explain in a bit more detail: > > When pushing a directory, it is traversed recursively on the host and all files > found are pushed to the device as needed. Additionally, if delete_device_stale > option is True, files on the device that don't exist on the host are deleted. > > (also nit: add space between title doc line and additional notes.) Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:841: stale_files_on_device += stale_files On 2015/06/10 09:24:23, perezju wrote: > nit: maybe rename these variables to something like changed_files/stale_files > for the local variable within each iteration, and > all_changed_files/all_stale_files for the aggregates. Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:895: del device_checksums[device_abs_path] On 2015/06/10 09:24:22, perezju wrote: > I think you can replace the last three lines with: > > device_checksum = device_checksums.pop(device_abs_path, None) Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:896: if ((device_checksum) != host_checksum): On 2015/06/10 09:24:23, perezju wrote: > nit: drop all the parentheses Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:898: to_delete = [p for p in device_checksums.iterkeys()] On 2015/06/10 09:24:22, perezju wrote: > nit: to_delete = device_checksums.keys() Done. https://codereview.chromium.org/1167693002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:934: self.RunShellCommand(['rm', stale_file_path]) On 2015/06/10 09:24:22, perezju wrote: > do we really need this method? maybe just inline within PushChangedFiles Done.
Thanks! Just a few nits now. Also to double check, patch set #4 is just a rebase, and #3 has all your changes, right? https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:839: If delete_device_stale option is True, delete stale files on device. nit: remove that line, it's now explained below. https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:872: self.RunShellCommand(['rm', stale_file_path]) add option '-f' to rm (just in case the file is gone between finding it and trying to delete it) and check_return=True, so we get a loud error if something is really wrong. https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils_test.py:561: ([],[]))): nit: space after the comma https://codereview.chromium.org/1167693002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils_test.py:1178: nit: two blank lines between classes
Thank you for the comments Juan! Yes patch set #4 is just a rebase. Menglin https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... File build/android/pylib/device/device_utils.py (right): https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... build/android/pylib/device/device_utils.py:839: If delete_device_stale option is True, delete stale files on device. On 2015/06/11 08:38:47, perezju wrote: > nit: remove that line, it's now explained below. Done. https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... build/android/pylib/device/device_utils.py:872: self.RunShellCommand(['rm', stale_file_path]) On 2015/06/11 08:38:47, perezju wrote: > add option '-f' to rm (just in case the file is gone between finding it and > trying to delete it) and check_return=True, so we get a loud error if something > is really wrong. Done. https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... File build/android/pylib/device/device_utils_test.py (right): https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... build/android/pylib/device/device_utils_test.py:561: ([],[]))): On 2015/06/11 08:38:47, perezju wrote: > nit: space after the comma Done. https://chromiumcodereview.appspot.com/1167693002/diff/60001/build/android/py... build/android/pylib/device/device_utils_test.py:1178: On 2015/06/11 08:38:48, perezju wrote: > nit: two blank lines between classes Done.
lgtm, thanks! john, do you have any other comments?
lgtm w/ nits We'll need to add support for this to platform mode, too. https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:871: self.RunShellCommand(['rm', '-f', stale_file_path], Does this work as self.RunShellCommand(['rm', '-f'] + all_stale_files, ...) ? https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:917: to_delete = [] nit: delete this line https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils_device_test.py (right): https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils_device_test.py:21: _old_contents = "foo" If these are constants, they should be _ALL_CAPS. If they aren't, they shouldn't be defined at module scope.
https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:871: self.RunShellCommand(['rm', '-f', stale_file_path], On 2015/06/12 19:54:44, jbudorick wrote: > Does this work as > > self.RunShellCommand(['rm', '-f'] + all_stale_files, ...) > > ? Yes it does, though in the description of the function writes This (the cmd being a sequence) allows to easily pass arguments containing spaces or special characters without having to worry about getting quoting right. Whenever possible, it is recomended to pass |cmd| as a sequence. https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:917: to_delete = [] On 2015/06/12 19:54:45, jbudorick wrote: > nit: delete this line Done. https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils_device_test.py (right): https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils_device_test.py:21: _old_contents = "foo" On 2015/06/12 19:54:45, jbudorick wrote: > If these are constants, they should be _ALL_CAPS. > > If they aren't, they shouldn't be defined at module scope. Done.
https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:871: self.RunShellCommand(['rm', '-f', stale_file_path], On 2015/06/12 22:31:46, Menglin wrote: > On 2015/06/12 19:54:44, jbudorick wrote: > > Does this work as > > > > self.RunShellCommand(['rm', '-f'] + all_stale_files, ...) > > > > ? > > Yes it does, though in the description of the function writes > This (the cmd being a sequence) > allows to easily pass arguments containing spaces or special characters > without having to worry about getting quoting right. Whenever possible, it > is recomended to pass |cmd| as a sequence. I guess what John is saying is whether we could remove all files with a single shell call, instead of removing one by one (and yes, always passing as a list of args, since that is preferred). I thought about that too, but wasn't sure how nicely would adb play with that if there are many files and the command becomes really big. I know we have other checks in place for that, but adb always makes me nervous :P. Anyway, as John suggested, let's go bold and try removing all_stale_files in one go. That would certainly be more efficient in most cases. As a side (not for this CL), maybe we want to provide a RemoveFiles method? Seems to be a common operation and small details (e.g. the need for '-f' and check_return) are easy to slip.
The CQ bit was checked by mlliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1167693002/#ps100001 (title: " ")
On 2015/06/15 09:03:52, perezju wrote: > https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1167693002/diff/80001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:871: self.RunShellCommand(['rm', > '-f', stale_file_path], > On 2015/06/12 22:31:46, Menglin wrote: > > On 2015/06/12 19:54:44, jbudorick wrote: > > > Does this work as > > > > > > self.RunShellCommand(['rm', '-f'] + all_stale_files, ...) > > > > > > ? > > > > Yes it does, though in the description of the function writes > > This (the cmd being a sequence) > > allows to easily pass arguments containing spaces or special characters > > without having to worry about getting quoting right. Whenever possible, it > > is recomended to pass |cmd| as a sequence. > > I guess what John is saying is whether we could remove all files with a single > shell call, instead of removing one by one (and yes, always passing as a list of > args, since that is preferred). > > I thought about that too, but wasn't sure how nicely would adb play with that if > there are many files and the command becomes really big. I know we have other > checks in place for that, but adb always makes me nervous :P. Anyway, as John > suggested, let's go bold and try removing all_stale_files in one go. That would > certainly be more efficient in most cases. Yes I have made this change in the code > > As a side (not for this CL), maybe we want to provide a RemoveFiles method? > Seems to be a common operation and small details (e.g. the need for '-f' and > check_return) are easy to slip. We will refactor it later
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167693002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d7f9fe973863ca93337991513c754a28299b152d Cr-Commit-Position: refs/heads/master@{#334438} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
