Bug 38756 – new-run-webkit-tests: "win" port (Apple's Cygwin/Windows port) doesn't work

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

new-run-webkit-tests does not work on windows

Needs a few tweaks to win.py

[Adam Roben (:aroben)]

Created attachment 55389 [details]
work-in-progress all-in-one messy patch

Here's a patch that I was working on during the Contributors Meeting. It gets things pretty far along on Windows. Here's a summary of the changes:

port/apache_http_server.py:

* Ripped out support for Chromium/win, since apparently they don't use this file. That let me remove lots of code that converted paths between Cygwin Apache and Windows Python. We don't need that code for Apple's Windows port, since we use both Cygwin Apache and Cygwin Python. This could probably be broken up into two or more patches.
* Now honors the port.apache_supports_ssl() method and turns off SSL features as needed. This could easily be broken out into its own patch.

port/base.py:

* Added apache_supports_ssl()
* Changed filename_to_uri not to add an extra / in Apple's Windows port, since we're dealing with Cygwin paths at this point. This could obviously be better abstracted.

port/server_process.py:

* Added prepare_path_for_driver, which at this level just returns the path unmodified.
* Changed run_test to use the new prepare_path_for_driver method.

port/websocket_server.py:

* Excluded some Chromium-specific code from running on Apple's Windows port

port/win.py:

* Implemented apache_supports_ssl
* Implemented baseline_search_path
* Implemented prepare_path_for_driver by converting the path to a Windows-style path using cygpath
* Implemented show_results_html_file using cygstart, since python's webbrowser module doesn't work in Cygwin
* Added supremely lame versions of default_configuration and _check_port_build

run_webkit_tests.py:

* Default to using Apache on Cygwin. This might screw up Chromium/win.

[Eric Seidel (no email)]

Thanks for the starting point Adam!

[Dirk Pranke]

It's true that at the moment Chromium doesn't use apache on Windows, but we have often wanted to (and we do have the command line flags to switch). I believe in the past we've found Apache too unstable to use, but I'm not sure if that's cygwin apache or a native port.

So, please don't rip that code out just yet.

Ojan, can you confirm?

[Eric Seidel (no email)]

It was ripped out after talking to Ojan. :)  SVN never forgets after all.

[Ojan Vafai]

(In reply to comment #4)
> It was ripped out after talking to Ojan. :)  SVN never forgets after all.

Yup. Since we don't have anyone in Chromium land actively trying to get this working, this is just dead code. Anyone who decides to make this work for chrome will have the SVN revision to look at to piece this back together. Until then, leaving this code in just slows down other development on it.

[Eric Seidel (no email)]

Part of this will be fixed by bug 38716.

[Dirk Pranke]

editing subject slightly, from "new-run-webkit-tests doesn't work on windows" to "new-run-webkit-tests: 'win' port doesn't work".

chromium-win works fine ;)

[Dirk Pranke]

Created attachment 82890 [details]
Patch

[Dirk Pranke]

Comment on attachment 82890 [details]
Patch

nm ... filing a different bug to track this.

[Adam Roben (:aroben)]

Created attachment 87818 [details]
Update to the all-in-one messy patch

This gets things a little bit working again in ToT, but DRT seems to hang in fread() after running one test.

[Radar WebKit Bug Importer]

<rdar://problem/10224910>

[Eric Seidel (no email)]

Moving this off of the "move all bots" bug, as I don't plan to do this before closing that bug and making NRWT default for all other ports. win will remain on an explicit black-list of unsupported ports for now.

[Adam Roben (:aroben)]

(In reply to comment #12)
> Moving this off of the "move all bots" bug, as I don't plan to do this before closing that bug and making NRWT default for all other ports. win will remain on an explicit black-list of unsupported ports for now.

That seems a little weird. Seems like the other bug needs a new title if it isn't going to be about moving "all" bots.

[Eric Seidel (no email)]

I'm happy to leave it on.  I just figured with now 122 dependent bugs, it was time to retire the "all bots" bug.  I was going to post a patch to it shortly to move from a white-list, to a black-list.  And leave "win" in the blacklist (along with qt-arm).

Since I can't easily do this win work myself, it seemed silly to have this still on my burn-down list.

But I'm happy to relate the bugs however you'd like. :)

[Adam Roben (:aroben)]

(In reply to comment #14)
> I'm happy to leave it on.  I just figured with now 122 dependent bugs, it was time to retire the "all bots" bug.  I was going to post a patch to it shortly to move from a white-list, to a black-list.  And leave "win" in the blacklist (along with qt-arm).

Moving to a blacklist sounds good.

> Since I can't easily do this win work myself, it seemed silly to have this still on my burn-down list.
> 
> But I'm happy to relate the bugs however you'd like. :)

The bugs exist outside of any one person's to-do list. "Switch all bots to NRWT" is still a valid task even if you can't complete it on your own. It seems confusing to have the "switch all bots" bug get closed without all bots being switched.

[Dirk Pranke]

Perhaps more on-target, is anyone actually working on getting the apple win port to work? Or planning to work on it, at least?

[Eric Seidel (no email)]

I know of no active development on the subject.  But I'm sure it eventually will happen.  :)

[Adam Roben (:aroben)]

I am planning to work on it sometime in the next few months, but it's hard to be more specific than that.

[Brent Fulgham]

Created attachment 202647 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 202647 [details]
Patch

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

> Tools/Scripts/webkitpy/port/win.py:107
> +    #
> +    # PROTECTED ROUTINES
> +    #
> +    # The routines below should only be called by routines in this class
> +    # or any of its subclasses.
> +    #

We don't normally add comments like this. Please remove them.

[Brent Fulgham]

Committed r150612: <http://trac.webkit.org/changeset/150612>

[Brent Fulgham]

Landed an initial change that gets the tests running.  Everything seems to work pretty well, although there are some mysterious CSS failures that Adam already pointed out in Bug 75707.

[Brent Fulgham]

Comment on attachment 202647 [details]
Patch

Clearing patch flag now that the change landed so I can make further updates.

[Adam Roben (:aroben)]

Awesome!

[Brent Fulgham]

Created attachment 202746 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 202746 [details]
Patch

Clearing flags on attachment: 202746

Committed r150615: <http://trac.webkit.org/changeset/150615>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This is a meta-bug. Close when the sub-tasks are complete.

[Brent Fulgham]

All subtasks complete.  Closing bug.