Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17467003:
[android] Switch test scripts to use last modified time instead of md5sum when checking dependencie… (Closed)
|
Created:
7 years, 6 months ago by craigdh Modified:
7 years, 4 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[android] Switch test scripts to use last modified time instead of md5sum when checking dependencies.
This works well because "adb push" copies over the modified time directly from the host file.
BUG=166338
TEST=None
Patch Set 1 #Patch Set 2 : #
Total comments: 22
Patch Set 3 : ignore extra files on the device #Patch Set 4 : addressed comments on timemodified.cc #
Total comments: 16
Patch Set 5 : rebase #Patch Set 6 : namedtuple and other nits #Patch Set 7 : forgot TODO #Patch Set 8 : NOT TO COMMIT #Patch Set 9 : adds back CheckMd5Sum for apks #Patch Set 10 : #Patch Set 11 : removed TODO, now a feature :) #
Total comments: 9
Patch Set 12 : bulach's comments #Patch Set 13 : fix compile error #Patch Set 14 : rebase #Patch Set 15 : rebase due to file_util:: -> base:: #Patch Set 16 : #
Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:64: TIMEMODIFIED_DEVICE_FOLDER = constants.TEST_EXECUTABLE_DIR + '/timemodified/' If these are only used in one function, let's move them there. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:64: TIMEMODIFIED_DEVICE_FOLDER = constants.TEST_EXECUTABLE_DIR + '/timemodified/' FOLDER -> DIRECTORY https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:699: def CheckTimesModified(self, local_path, device_path, ignore_paths=False): I would rename this to something like FileOutOfSync() https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:717: cmd_helper.OutDirectory().get(), default_build_type) We should really set the out directory once instead of figuring it out multiple times. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:718: timemodified_dist_path = '%s/timemodified_dist' % build_dir let's have 'host'/'device' in the variable names to make this more readable. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:739: #TODO(craigdh): This doesn't appear to have any effect. What's the deal here? If not used, can we remove it? https://codereview.chromium.org/17467003/diff/2001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/17467003/diff/2001/build/java_apk.gypi#newcod... build/java_apk.gypi:237: '<(DEPTH)/tools/android/timemodified/timemodified.gyp:timemodified', Let's have a more specific name such 'timechecker' https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 13 https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:5: // Md5sum implementation for Android. This version handles files as well as Update doc https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:39: (child = file_enumerator.Next()) != empty; ) { I think a while loop here is more readable. https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:42: if (!PrintFileAccessTime(child)) Can we log the failure cases for debugging.
https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:64: TIMEMODIFIED_DEVICE_FOLDER = constants.TEST_EXECUTABLE_DIR + '/timemodified/' On 2013/06/20 22:52:38, frankf wrote: > FOLDER -> DIRECTORY Done. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:64: TIMEMODIFIED_DEVICE_FOLDER = constants.TEST_EXECUTABLE_DIR + '/timemodified/' On 2013/06/20 22:52:38, frankf wrote: > If these are only used in one function, let's move them there. Done. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:699: def CheckTimesModified(self, local_path, device_path, ignore_paths=False): On 2013/06/20 22:52:38, frankf wrote: > I would rename this to something like FileOutOfSync() Done. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:699: def CheckTimesModified(self, local_path, device_path, ignore_paths=False): On 2013/06/20 22:52:38, frankf wrote: > I would rename this to something like FileOutOfSync() Done. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:717: cmd_helper.OutDirectory().get(), default_build_type) On 2013/06/20 22:52:38, frankf wrote: > We should really set the out directory once instead of figuring it out multiple > times. Why? It's not a very heavy call. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:718: timemodified_dist_path = '%s/timemodified_dist' % build_dir On 2013/06/20 22:52:38, frankf wrote: > let's have 'host'/'device' in the variable names to make this more readable. Done. https://codereview.chromium.org/17467003/diff/2001/build/android/pylib/androi... build/android/pylib/android_commands.py:739: #TODO(craigdh): This doesn't appear to have any effect. On 2013/06/20 22:52:38, frankf wrote: > What's the deal here? If not used, can we remove it? Done. https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/06/20 22:52:38, frankf wrote: > 13 Done. https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:5: // Md5sum implementation for Android. This version handles files as well as On 2013/06/20 22:52:38, frankf wrote: > Update doc Done. https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:39: (child = file_enumerator.Next()) != empty; ) { On 2013/06/20 22:52:38, frankf wrote: > I think a while loop here is more readable. Done. https://codereview.chromium.org/17467003/diff/2001/tools/android/timemodified... tools/android/timemodified/timemodified.cc:42: if (!PrintFileAccessTime(child)) On 2013/06/20 22:52:38, frankf wrote: > Can we log the failure cases for debugging. Done.
https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:179: return [line.split(' ')[:2] for line in timemodified_output] Is this number of spaces important. Can you just do line.split()? https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:707: device_path: Path on the device. file or directory? https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:724: host_timemodified_dist_path = '%s/timemodified_dist' % build_dir Why is this not a constant like the device counterparts? https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:743: host_time_tuples = _ComputeFileListTimesModified( Can you use named tuples instead of p[0]/[1] https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:749: # Ignore extra files on the device. Add a TODO to clean up old data on the device. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:750: device_time_tuples = [t for t in device_time_tuples if t[1] != '.'] Why are listing this? https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:754: def _host_has(fname): Use blank lines to improve readability https://codereview.chromium.org/17467003/diff/12001/tools/android/timemodifie... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/12001/tools/android/timemodifie... tools/android/timemodified/timemodified.cc:34: const std::string svn_dir_component = FILE_PATH_LITERAL("/.svn/"); have a ignore list with .git as well?
ptal. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:179: return [line.split(' ')[:2] for line in timemodified_output] On 2013/07/02 18:45:29, frankf wrote: > Is this number of spaces important. Can you just do line.split()? Yep, important as discussed offline. Added an assert. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:707: device_path: Path on the device. On 2013/07/02 18:45:29, frankf wrote: > file or directory? either. clarified. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:724: host_timemodified_dist_path = '%s/timemodified_dist' % build_dir On 2013/07/02 18:45:29, frankf wrote: > Why is this not a constant like the device counterparts? Because it is determined dynamically at runtime. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:743: host_time_tuples = _ComputeFileListTimesModified( On 2013/07/02 18:45:29, frankf wrote: > Can you use named tuples instead of p[0]/[1] Done. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:749: # Ignore extra files on the device. On 2013/07/02 18:45:29, frankf wrote: > Add a TODO to clean up old data on the device. Done. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:750: device_time_tuples = [t for t in device_time_tuples if t[1] != '.'] On 2013/07/02 18:45:29, frankf wrote: > Why are listing this? Already removed in the patch I uploaded while you were reviewing. https://codereview.chromium.org/17467003/diff/12001/build/android/pylib/andro... build/android/pylib/android_commands.py:754: def _host_has(fname): On 2013/07/02 18:45:29, frankf wrote: > Use blank lines to improve readability Done. https://codereview.chromium.org/17467003/diff/12001/tools/android/timemodifie... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/12001/tools/android/timemodifie... tools/android/timemodified/timemodified.cc:34: const std::string svn_dir_component = FILE_PATH_LITERAL("/.svn/"); On 2013/07/02 18:45:29, frankf wrote: > have a ignore list with .git as well? This isn't an issue at the moment. Added a TODO but I think it can wait for its own cl.
lgtm
On 2013/07/02 20:55:18, frankf wrote: > lgtm Can we try to land this today?
Added digit for tools/android OWNERS.
Wrong digit. +bulach for tools/android OWNERS.
On 2013/07/15 18:20:12, craigdh wrote: > Wrong digit. +bulach for tools/android OWNERS. +digit for tools/android OWNERS, figured out the right digit :)
lgtm, nice! just devil's advocate: is adb push behavior consistent across different android versions? https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:757: def AreFilesUnsynchronized(self, local_path, device_path): nit: make this internal, "_" prefix https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:758: """Compares the time modified of a local path against a device path. could probably add a note saying that files transferred via "adb push" will keep the host's modified time. https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:821: return any(int(dtime) < int(htime) for dtime, htime in should we check for != instead? i.e., a file modified in the device should probably be pushed again? https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... tools/android/timemodified/timemodified.cc:33: const std::string svn_dir_component = FILE_PATH_LITERAL("/.svn/"); how about .git?
From what information I could find online and local experimentation "adb push"'s behavior seems to be consistent. https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:757: def AreFilesUnsynchronized(self, local_path, device_path): On 2013/07/15 18:35:20, bulach wrote: > nit: make this internal, "_" prefix Done. https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:758: """Compares the time modified of a local path against a device path. On 2013/07/15 18:35:20, bulach wrote: > could probably add a note saying that files transferred via "adb push" will keep > the host's modified time. Done. https://codereview.chromium.org/17467003/diff/58001/build/android/pylib/andro... build/android/pylib/android_commands.py:821: return any(int(dtime) < int(htime) for dtime, htime in On 2013/07/15 18:35:20, bulach wrote: > should we check for != instead? i.e., a file modified in the device should > probably be pushed again? Done. https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... tools/android/timemodified/timemodified.cc:33: const std::string svn_dir_component = FILE_PATH_LITERAL("/.svn/"); On 2013/07/15 18:35:20, bulach wrote: > how about .git? It's not an issue right now because .git is only in the top level directory for the source tree. It won't be an issue in the future because frank's isolate changes hardlink only the dependencies before pushing so there won't be any extra files. Once everything is switched to use isolate files the .svn check can be removed as well.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/66001
https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... File tools/android/timemodified/timemodified.cc (right): https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... tools/android/timemodified/timemodified.cc:33: const std::string svn_dir_component = FILE_PATH_LITERAL("/.svn/"); Actually, this won't work if you're just pushing arbitrary directories that are not generated by isolate, which is likely since PushIfNeeded is a general utility method. On 2013/07/15 19:06:50, craigdh wrote: > On 2013/07/15 18:35:20, bulach wrote: > > how about .git? > > It's not an issue right now because .git is only in the top level directory for > the source tree. It won't be an issue in the future because frank's isolate > changes hardlink only the dependencies before pushing so there won't be any > extra files. Once everything is switched to use isolate files the .svn check can > be removed as well.
On 2013/07/15 19:16:57, frankf wrote: > https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... > File tools/android/timemodified/timemodified.cc (right): > > https://codereview.chromium.org/17467003/diff/58001/tools/android/timemodifie... > tools/android/timemodified/timemodified.cc:33: const std::string > svn_dir_component = FILE_PATH_LITERAL("/.svn/"); > Actually, this won't work if you're just pushing arbitrary directories that are > not generated by isolate, which is likely since PushIfNeeded is a general > utility method. > > On 2013/07/15 19:06:50, craigdh wrote: > > On 2013/07/15 18:35:20, bulach wrote: > > > how about .git? > > > > It's not an issue right now because .git is only in the top level directory > for > > the source tree. It won't be an issue in the future because frank's isolate > > changes hardlink only the dependencies before pushing so there won't be any > > extra files. Once everything is switched to use isolate files the .svn check > can > > be removed as well. Good point. I'll make a follow up cl to exclude .git as well.
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/58003
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
rubberstamp lgtm For the record, I checked the ADB sources, and all versions of it have always sent / applied the modification time from the host to the device, so this patch should work everywhere :-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/58003
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/70002
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/107003
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/17467003/138001 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
