Bug 95346 – Get rid of device/host clock synchronization.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Philippe Liard]

This prevents us from running tests on non-rooted devices.

[Philippe Liard]

Created attachment 161233 [details]
Patch

[Peter Beverloo]

Comment on attachment 161233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161233&action=review

Thank you Philippe! A few more comments.

> Tools/ChangeLog:3
> +        Support LayoutTests on non-rooted devices for Chromium Android.

This should be equal to the bug's title. Maybe this would work?

Get rid of device/host clock synchronization for Chromium Android.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:44
> +    os.path.join(os.environ['CHROME_SRC'], 'build', 'android', 'pylib'))

Peter:
I'm not comfortable introducing this dependency. We're trying to get rid of dependencies on environment variable (like CHROME_SRC), and since we need adb commands in order parts of WebKit as well, I'd prefer to have a smaller but similar runner on the WebKit side. This is out of scope, but is there any way we can work around needing this right now?

Philippe:
I can avoid relying on $CHROME_SRC and use a relative path assuming that this file won't be moved. Regarding the use of android_commands, AndroidCommands.PushIfNeeded() provides a way to sync data files with the device by using a custom implementation of md5sum (written in C++ and depending on Chromium's base/). Although we could have our own Python layer for that in WebKit, we can hardly do the same for md5sum (I don't think you want to duplicate it either). So it seems that we would have to use Chromium's md5sum (being submitted under tools/android/md5sum) anyway.

Since we own that file, which is Chromium-specific, I think that preventing us from depending on Chromium might be an unnecessary constraint. I agree that we could introduce at least a thin layer in WebKit, calling Chromium's python, rather than calling it directly though.
But I'm sure that I don't see the full picture since this is my first WebKit contribution. What do you think? Is this dependency to Chromium the only one?

Peter:
The relative path would be different depending on whether layout tests are being run in a Chromium or WebKit environment (from WebKit's root: ../../build/android/pylib/ for Chromium versus Source/WebKit/chromium/build/android/pylib/ for WebKit). I don't think anything else in the layout tests runners depend on Chromium code right now, which also is the reason we have our own adb interface. Since we only need a subset of functionality here, and we need it for running other targets as well, eventually we'll probably end up with a script of our own.

Going back to this case: CHROME_SRC will not be available when running layout tests in a WebKit environment, which is my worry. If we can add the md5sum target as a dependency of DumpRenderTree, will that allow you to implement a similar PushIfNeeded method in the ChromiumAndroidDriver?

> LayoutTests/http/tests/cache/resources/current-time.cgi:5
> +print time()

nit: statements end with a semi-colon.

> LayoutTests/http/tests/cache/resources/subresource-test.js:28
> +var serverClientTimeDelta = getServerDate().getTime() - new Date().getTime();

Thanks! Very minor nit for readability: maybe define this property after the getServerDate() function?

[Dirk Pranke]

Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout.

[Philippe Liard]

(In reply to comment #3)
> Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout.

Nice. Going back to your comment Peter, can I use what Dirk suggested to include the android_commands module and use AndroidCommands.PushIfNeeded() or do we still really want to avoid this dependency (which would make this patch much larger)? My understanding is that if we have a way to fetch chromium's path, it might not be totally alien to depend on some code coming from it.

[Peter Beverloo]

(In reply to comment #4)
> (In reply to comment #3)
> > Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout.
> 
> Nice. Going back to your comment Peter, can I use what Dirk suggested to include the android_commands module and use AndroidCommands.PushIfNeeded() or do we still really want to avoid this dependency (which would make this patch much larger)? My understanding is that if we have a way to fetch chromium's path, it might not be totally alien to depend on some code coming from it.

There are a number of similar dependencies WebKit has on Chromium already. The implication of starting to rely on test-runner scripts is that refactoring / "API" changes would need the new method to be introduced on the Chromium side, then usage of the old versions removed within WebKit, before the old code can be removed. A cross-project three-way patch is significantly more painful than a single patch.

Given the amount of random build breakages due to Chromium infrastructure changes recently, we should try to minimize the number of dependencies until the code stabilizes. While android_commands.py probably will be more stable, our branch still has a 165 line diff and it's not unthinkable more refactoring will happen too.

As discussed on chat, adding a dependency to md5sum in DumpRenderTree's gyp configuration is perfectly fine. It'll introduce some additional code for now, but we'll be able to clean that up later again.

[Philippe Liard]

Created attachment 161674 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161674 [details]
Patch

Attachment 161674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13725122

[Peter Beverloo (cr-android ews)]

Comment on attachment 161674 [details]
Patch

Attachment 161674 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13728063

[Philippe Liard]

FYI, the try bots fail because this CL depends on https://chromiumcodereview.appspot.com/10867008/ which is not submitted yet.

[Philippe Liard]

Created attachment 161680 [details]
Patch

[Philippe Liard]

I uploaded a new patch. Is that the right way to iterate on a CL? I'm still new to the WebKit contribution process :)

Quick question: does ChromiumAndroidDriver need to be thread safe? I added a static variable which is not guarded by any lock at the moment. I supposed that the class was accessed from a single thread. Is that correct?

[WebKit Review Bot]

Comment on attachment 161680 [details]
Patch

Attachment 161680 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13728069

[Peter Beverloo (cr-android ews)]

Comment on attachment 161680 [details]
Patch

Attachment 161680 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13721160

[Philippe Liard]

Created attachment 162197 [details]
Patch

[Philippe Liard]

I've just re-uploaded the exact same patch now that the other CL on the chromium side has landed. This will re-trigger the try bots which will be green hopefully this time.

This is also a way to be back to the top of your inbox :)

[Philippe Liard]

Created attachment 162220 [details]
Patch

[Philippe Liard]

I uploaded a new patch since the old one didn't apply correctly anymore to the current master.

[Peter Beverloo (cr-android ews)]

Comment on attachment 162220 [details]
Patch

Attachment 162220 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13753767

[Philippe Liard]

Ah I forgot that we need to roll a new revision of Chromium too. I will re-trigger the try bots by uploading a new patch once we did that.

[WebKit Review Bot]

Comment on attachment 162220 [details]
Patch

Attachment 162220 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13744971

[Philippe Liard]

Created attachment 163098 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163098 [details]
Patch

Attachment 163098 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13804539

[Peter Beverloo]

Comment on attachment 163098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review

A few minor nits. Looks fine overall, except for the build failure (I commented on that later on).

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128
> +                '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum',

This needs to be specific to Android, i.e. in a conditional block.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328
> +        if not worker_number in self._data_already_pushed_for_worker:

The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385
> +                                 stdout=subprocess.PIPE).stdout)

nit: It's fine to leave it like this (since it may become unreadable otherwise), but WebKit has no limit on line lengths.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:677
> +                if last_char in ['#', '$']:

nit: could use a tuple here (which is preferred for lists in gyp).

[Philippe Liard]

Created attachment 163105 [details]
Patch

[Philippe Liard]

Comment on attachment 163098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128
>> +                '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum',
> 
> This needs to be specific to Android, i.e. in a conditional block.

Indeed.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328
>> +        if not worker_number in self._data_already_pushed_for_worker:
> 
> The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point?

I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static.

[Peter Beverloo]

(In reply to comment #25)
> (From update of attachment 163098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review
> 
> >> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128
> >> +                '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum',
> > 
> > This needs to be specific to Android, i.e. in a conditional block.
> 
> Indeed.
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328
> >> +        if not worker_number in self._data_already_pushed_for_worker:
> > 
> > The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point?
> 
> I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static.

Ah, I see. You probably should be using "ChromiumAndroidDriver._data_already_pushed_for_worker" to access the variable, then.

The rest LGTM.

[Philippe Liard]

Created attachment 163125 [details]
Patch

[Philippe Liard]

Comment on attachment 163098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328
>>> +        if not worker_number in self._data_already_pushed_for_worker:
>> 
>> The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point?
> 
> I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static.

I have updated this part to refer to the static variable through 'ChromiumAndroidDriver' rather than 'self' which doesn't make sense for a non-instance member.

[Peter Beverloo]

Thanks! Adam, could you do a formal review please?

[WebKit Review Bot]

Comment on attachment 163125 [details]
Patch

Attachment 163125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13796915

New failing tests:
http/tests/cache/subresource-expiration-2.html
http/tests/cache/subresource-expiration-1.html

[Philippe Liard]

I'm looking at the test failures.

[Philippe Liard]

I believe the failure is due to the missing execution permission on current-time.cgi. I will re-upload a patch right now.

[Philippe Liard]

Created attachment 163137 [details]
Patch

[Dirk Pranke]

Comment on attachment 163137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
> +                                 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower())

As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385
> +                                 stdout=subprocess.PIPE).stdout)

You can use self._port.host.executive.popen() here and below -  that's slightly preferred to calling subprocess.Popen() directly.

[Philippe Liard]

Created attachment 163313 [details]
Patch

[Philippe Liard]

Comment on attachment 163137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
>> +                                 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower())
> 
> As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people.

This kind of code is duplicated across multiple places in the Chromium Android Python codebase. We clearly miss a way to figure out the build type in a single line regardless of the language (Python or Shell). I think we have a bug tracking that. If I remember correctly we plan to make GYP output the build type to a file in out/. This code can be updated once we landed that change.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385
>> +                                 stdout=subprocess.PIPE).stdout)
> 
> You can use self._port.host.executive.popen() here and below -  that's slightly preferred to calling subprocess.Popen() directly.

Done.

[Peter Beverloo]

Comment on attachment 163313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163313&action=review

Thanks Philippe!

> Tools/ChangeLog:5
> +

This needs a "Reviewed by" line. As Dirk already reviewed it, it's fine to add "Reviewed by Dirk Pranke".

> LayoutTests/ChangeLog:5
> +

dito.

[Philippe Liard]

Created attachment 163341 [details]
Patch

[Philippe Liard]

Comment on attachment 163313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163313&action=review

>> Tools/ChangeLog:5
>> +
> 
> This needs a "Reviewed by" line. As Dirk already reviewed it, it's fine to add "Reviewed by Dirk Pranke".

Done.

>> LayoutTests/ChangeLog:5
>> +
> 
> dito.

Done.

[Peter Beverloo]

Created attachment 163345 [details]
Patch for landing

[Peter Beverloo]

Comment on attachment 163345 [details]
Patch for landing

Putting on the queue. I only touched _setup_md5sum() to make sure it works both on Chromium and on WebKit, of course with Philippe's consent.

[WebKit Review Bot]

Comment on attachment 163345 [details]
Patch for landing

Clearing flags on attachment: 163345

Committed r128181: <http://trac.webkit.org/changeset/128181>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke webkitpy-test, at least on mac:

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/38183/steps/webkitpy-test/logs/stdio

[462/1616] webkitpy.layout_tests.port.chromium_android_unittest.ChromiumAndroidDriverTest.test_cmd_line erred:
  Traceback (most recent call last):
    File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android_unittest.py", line 175, in setUp
      self.driver = chromium_android.ChromiumAndroidDriver(self.port, worker_number=0, pixel_tests=True)
    File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 327, in __init__
      self._setup_md5sum()
    File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 340, in _setup_md5sum
      assert os.path.exists(self._md5sum_path)
  AssertionError

[Philippe Liard]

Yeah it did. Sorry for the inconvenience. The fix has just landed: https://bugs.webkit.org/show_bug.cgi?id=96393.

[Peter Beverloo]

The Qt bot is happy again:
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/26726/steps/webkitpy-test/logs/stdio

Closing.

[Dirk Pranke]

(In reply to comment #36)
> (From update of attachment 163137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
> >> +                                 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower())
> > 
> > As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people.
> 
> This kind of code is duplicated across multiple places in the Chromium Android Python codebase. We clearly miss a way to figure out the build type in a single line regardless of the language (Python or Shell). I think we have a bug tracking that. If I remember correctly we plan to make GYP output the build type to a file in out/. This code can be updated once we landed that change.
> 

You lost me here. I think you're talking about something different from what I'm talking about; are you talking about picking the "correct" configuration by default so that you don't have to specify either --release or --debug to get the one you want? You can use set-webkit-configuration to specify that. I'm talking about what happens when you have *both* release and debug configurations and are alternating between them - one of them will *always* produce this error.