Bug 98323 – run-webkit-tests tests on win32 after r127302

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

run-webkit-tests tests on win32 after r127302

[Dirk Pranke]

Created attachment 166990 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 166990 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:94
> +        exit_code = executive.run_command(svn_info_args, cwd=path, return_exit_code=True, shell=(sys.platform == 'win32'))

shell is the devil.  Causes all sorts of oddness.  Can we solve this w/o shell?

[Dirk Pranke]

Using the default chromium install, we point to an svn.bat in depot_tools; without shell=True, calling "svn info" won't work.

An alternative would be to make cls.executable a method and probe to figure out if we should use svn.exe or svn.bat.

[Eric Seidel (no email)]

Don't we already have hacks for svn.bat?

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/host.py#L82

[Dirk Pranke]

Good question. I thought we had something like that but it must not be working. I will dig in and find out why not.

[Dirk Pranke]

okay, turns out we don't actually call _initialize_scm() unless we're running on a bot; this was done to avoid the hit of calling out to svn if we didn't really need the revisions. Unfortunately, the code in json_result_generator doesn't know about that, and doesn't trap the error or call initialize_scm. 

It's easy enough to call initialize_scm() again, but it's harder (if not impossible) to figure out if we need to or not at that layer of the code without doing some refactoring.

[Dirk Pranke]

I'll post a patch that fixes the crash for now and add a FIXME: to mention the slowdown and possible refactoring.

[Dirk Pranke]

Created attachment 167003 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 167003 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:391
> +        self._port.host._initialize_scm()
>          scm = SCMDetector(self._filesystem, self._executive).detect_scm_system(in_directory)
>          if scm:
>              return scm.svn_revision(in_directory)

Um... Why can't it just use self._port.host.scm now then?  Why re-detect?

[Dirk Pranke]

(In reply to comment #9)
> (From update of attachment 167003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167003&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:391
> > +        self._port.host._initialize_scm()
> >          scm = SCMDetector(self._filesystem, self._executive).detect_scm_system(in_directory)
> >          if scm:
> >              return scm.svn_revision(in_directory)
> 
> Um... Why can't it just use self._port.host.scm now then?  Why re-detect?

The directory matters, since we look once for the webkit checkout and once for the chromium checkout (and they might actually be different).

[Eric Seidel (no email)]

Comment on attachment 167003 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:391
>>>              return scm.svn_revision(in_directory)
>> 
>> Um... Why can't it just use self._port.host.scm now then?  Why re-detect?
> 
> The directory matters, since we look once for the webkit checkout and once for the chromium checkout (and they might actually be different).

I see.  We should probably add an explaination of why we call _initialize_scm() here.  Also, don't we have a non-private API for causing initialize_scm to fire?  Could we just grab scm()?  Or if we're going to call private functions on host, why not just call _engage_awesome_windows_hacks directly?

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 167003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167003&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:391
> >>>              return scm.svn_revision(in_directory)
> >> 
> >> Um... Why can't it just use self._port.host.scm now then?  Why re-detect?
> > 
> > The directory matters, since we look once for the webkit checkout and once for the chromium checkout (and they might actually be different).
> 
> I see.  We should probably add an explaination of why we call _initialize_scm() here. 

Will do.

Also, don't we have a non-private API for causing initialize_scm to fire? Could we just grab scm()?  Or if we're going to call private functions on host, why not just call _engage_awesome_windows_hacks directly?

grabbing scm() directly won't call initialize_scm(). We should either make that happen, or make initialize_scm() public IMO.

[Dirk Pranke]

Created attachment 167018 [details]
make initialize_scm() public

[Eric Seidel (no email)]

Comment on attachment 167018 [details]
make initialize_scm() public

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:389
> +        # We can't reuse an existing scm object because we need to look at the specific directories.

Rather, because the specific directories we're interested in may come from a different checkout.

[Dirk Pranke]

(In reply to comment #14)
> (From update of attachment 167018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167018&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:389
> > +        # We can't reuse an existing scm object because we need to look at the specific directories.
> 
> Rather, because the specific directories we're interested in may come from a different checkout.

That is clearer, thanks!

[Dirk Pranke]

Committed r130356: <http://trac.webkit.org/changeset/130356>