Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293803003:
[Android] Fix adb realpath command failure. (Closed)
|
Created:
6 years, 7 months ago by Kibeom Kim (inactive) Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Fix adb realpath command failure.
In android_commands.py's GetFilesChanged function,
realpath command fails when the path does not exist.
Create the path before we call GetFilesChanged.
BUG=374859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271817
Patch Set 1 #
Total comments: 3
Patch Set 2 : moved to PushIfNeeded #
Total comments: 5
Patch Set 3 : host_path dir check #Messages
Total messages: 17 (0 generated)
I'm not 100% sure this is a correct way to fix it.
https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1025: self.RunShellCommand('mkdir -p "%s"' % device_path) I don't think that GetFilesChanged should be creating directories on the device. That should be left to PushIfNeeded. I'm also not sure that this fix would work if the device path was more than one directory deep. I think you're right that this only triggers on the code path that pushes individual files in PushIfNeeded. Perhaps we should be creating directories there? (although that could mean more adb round trips, which we'd like to avoid)
https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1025: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 01:33:13, jbudorick wrote: > I don't think that GetFilesChanged should be creating directories on the device. > That should be left to PushIfNeeded. I'm also not sure that this fix would work > if the device path was more than one directory deep. I agree that this should not create directories on the device. Can PushIfNeeded create the directory instead?
Moved to PushIfNeeded. Tested by "rm -rf /mnt/shell/emulated/0/chrome" and confirmed that this script creates "/mnt/shell/emulated/0/chrome/test/data/clank" and runs the tests. https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1025: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 01:33:13, jbudorick wrote: > I don't think that GetFilesChanged should be creating directories on the device. > That should be left to PushIfNeeded. I'm also not sure that this fix would work > if the device path was more than one directory deep. > > I think you're right that this only triggers on the code path that pushes > individual files in PushIfNeeded. Perhaps we should be creating directories > there? (although that could mean more adb round trips, which we'd like to avoid) Done.
On 2014/05/20 01:58:29, Kibeom Kim wrote: > Moved to PushIfNeeded. Tested by > > "rm -rf /mnt/shell/emulated/0/chrome" > and confirmed that this script creates > "/mnt/shell/emulated/0/chrome/test/data/clank" and runs the tests. > > https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... > File build/android/pylib/android_commands.py (right): > > https://codereview.chromium.org/293803003/diff/1/build/android/pylib/android_... > build/android/pylib/android_commands.py:1025: self.RunShellCommand('mkdir -p > "%s"' % device_path) > On 2014/05/20 01:33:13, jbudorick wrote: > > I don't think that GetFilesChanged should be creating directories on the > device. > > That should be left to PushIfNeeded. I'm also not sure that this fix would > work > > if the device path was more than one directory deep. > > > > I think you're right that this only triggers on the code path that pushes > > individual files in PushIfNeeded. Perhaps we should be creating directories > > there? (although that could mean more adb round trips, which we'd like to > avoid) > > Done. lgtm. Side note: I wonder if PushIfNeeded is being called incorrectly other places. It looks like PushIfNeeded returns immediately if the files on the host have not changed, without testing if the files already exist on the device. It seems to me, it should only return without pushing if the files on the host have not changed AND the files are on the device. Perhaps it is implemented as it is so it can avoid doing adb commands on disk. In that case, we should be careful how we call it. E.g. I wonder if the md5sum error we continue to see is because someone tries to push with PushIfNeeded thinking that will push the md5sum, but it never checks if it is on the device. Anyway, I'll take a look at that. This change looks good.
+pliard (OWNER)
On 2014/05/20 17:46:08, Kibeom Kim wrote: > +pliard (OWNER) Please wait for approval from jbudorick, as he has the most experience with PushIfNeeded. @navabi: Someone added that 'cache' optimization to PushIfNeeded for some very specific perf testing use case, if I remember correctly. In general PushIfNeeded should always check the device for files as that cache only sticks around for the life of the specific AndroidCommands instance, which is created at the start of each test. I agree that it is a very dangerous hack, though, and could lead to some very tricky to debug issues if we ever need to push a file more than once (command line files?). I'd be very happy to see those 5 lines deleted.
@navabi: Yes, that's definitely a bug. Fortunately, it doesn't look like it comes up often - as Craig noted, a specific AndroidCommands instance has to be asked to push a file - not a directory - more than once, with the file being deleted from the device in between. https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1089: self.RunShellCommand('mkdir -p "%s"' % device_path) I'm still not sure that this will properly handle individual push path if the directory is multiple levels deep. I'm writing a quick test to check.
https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1089: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 18:03:39, jbudorick wrote: > I'm still not sure that this will properly handle individual push path if the > directory is multiple levels deep. I'm writing a quick test to check. Still not sure w.r.t. the above (although leaning toward it not being an issue), but this doesn't appear to work if the device_path is the intended file name on the device rather than the intended directory.
https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1089: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 18:53:53, jbudorick wrote: > On 2014/05/20 18:03:39, jbudorick wrote: > > I'm still not sure that this will properly handle individual push path if the > > directory is multiple levels deep. I'm writing a quick test to check. > > Still not sure w.r.t. the above (although leaning toward it not being an issue), > but this doesn't appear to work if the device_path is the intended file name on > the device rather than the intended directory. Oh, I wasn't aware of that device_path can also be a file path. Is it OK to assume that if host_path is a dir, then device_path is also a dir? (well, obviously otherwise doesn't make sense) so like. if os.path.isdir(host_path): self.RunShellCommand('mkdir -p "%s"' % device_path)
https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1089: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 19:06:15, Kibeom Kim wrote: > On 2014/05/20 18:53:53, jbudorick wrote: > > On 2014/05/20 18:03:39, jbudorick wrote: > > > I'm still not sure that this will properly handle individual push path if > the > > > directory is multiple levels deep. I'm writing a quick test to check. > > > > Still not sure w.r.t. the above (although leaning toward it not being an > issue), > > but this doesn't appear to work if the device_path is the intended file name > on > > the device rather than the intended directory. > > Oh, I wasn't aware of that device_path can also be a file path. Is it OK to > assume that if host_path is a dir, then device_path is also a dir? (well, > obviously otherwise doesn't make sense) > > so like. > > if os.path.isdir(host_path): > self.RunShellCommand('mkdir -p "%s"' % device_path) > Yeah, that seems fine to me. As you said, it wouldn't really make sense otherwise.
https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/293803003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1089: self.RunShellCommand('mkdir -p "%s"' % device_path) On 2014/05/20 19:09:40, jbudorick wrote: > On 2014/05/20 19:06:15, Kibeom Kim wrote: > > On 2014/05/20 18:53:53, jbudorick wrote: > > > On 2014/05/20 18:03:39, jbudorick wrote: > > > > I'm still not sure that this will properly handle individual push path if > > the > > > > directory is multiple levels deep. I'm writing a quick test to check. > > > > > > Still not sure w.r.t. the above (although leaning toward it not being an > > issue), > > > but this doesn't appear to work if the device_path is the intended file name > > on > > > the device rather than the intended directory. > > > > Oh, I wasn't aware of that device_path can also be a file path. Is it OK to > > assume that if host_path is a dir, then device_path is also a dir? (well, > > obviously otherwise doesn't make sense) > > > > so like. > > > > if os.path.isdir(host_path): > > self.RunShellCommand('mkdir -p "%s"' % device_path) > > > > Yeah, that seems fine to me. As you said, it wouldn't really make sense > otherwise. Done.
lgtm
lgtm.
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/293803003/40001
Message was sent while issue was closed.
Change committed as 271817 |
