Bug 98323 - run-webkit-tests tests on win32 after r127302
Summary: run-webkit-tests tests on win32 after r127302
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-03 16:36 PDT by Dirk Pranke
Modified: 2012-10-03 19:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2012-10-03 16:38 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (2.33 KB, patch)
2012-10-03 17:14 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
make initialize_scm() public (9.88 KB, patch)
2012-10-03 19:00 PDT, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-10-03 16:36:33 PDT
run-webkit-tests tests on win32 after r127302
Comment 1 Dirk Pranke 2012-10-03 16:38:13 PDT
Created attachment 166990 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-10-03 16:39:33 PDT
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?
Comment 3 Dirk Pranke 2012-10-03 16:43:08 PDT
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.
Comment 4 Eric Seidel (no email) 2012-10-03 16:44:53 PDT
Don't we already have hacks for svn.bat?

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/host.py#L82
Comment 5 Dirk Pranke 2012-10-03 16:46:33 PDT
Good question. I thought we had something like that but it must not be working. I will dig in and find out why not.
Comment 6 Dirk Pranke 2012-10-03 17:07:11 PDT
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.
Comment 7 Dirk Pranke 2012-10-03 17:07:35 PDT
I'll post a patch that fixes the crash for now and add a FIXME: to mention the slowdown and possible refactoring.
Comment 8 Dirk Pranke 2012-10-03 17:14:22 PDT
Created attachment 167003 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-10-03 17:22:49 PDT
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?
Comment 10 Dirk Pranke 2012-10-03 17:23:55 PDT
(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).
Comment 11 Eric Seidel (no email) 2012-10-03 17:29:16 PDT
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?
Comment 12 Dirk Pranke 2012-10-03 17:33:16 PDT
(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.
Comment 13 Dirk Pranke 2012-10-03 19:00:11 PDT
Created attachment 167018 [details]
make initialize_scm() public
Comment 14 Eric Seidel (no email) 2012-10-03 19:02:01 PDT
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.
Comment 15 Dirk Pranke 2012-10-03 19:24:18 PDT
(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!
Comment 16 Dirk Pranke 2012-10-03 19:53:05 PDT
Committed r130356: <http://trac.webkit.org/changeset/130356>