Bug 151156 - [Win] webkitpy is applying abspath to DOS paths, yielding invalid paths
Summary: [Win] webkitpy is applying abspath to DOS paths, yielding invalid paths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-11 14:42 PST by Brent Fulgham
Modified: 2015-11-19 10:43 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.74 KB, patch)
2015-11-11 14:47 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v2 (5.55 KB, patch)
2015-11-12 12:31 PST, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-11-11 14:42:47 PST
Because we are in the process of moving from pure Cygwin to a pure Windows build, some of our Cygwin tools end up calling native Perl and Python. When this happens, the path types received by Cygwin scripts can sometimes be unexpected.

The most obvious example of this failure is:

> run-webkit-tests -1 fast

... yielding the error:

DumpRenderTree was not found at /home/trybot/WebKit/C:/cygwin/home/trybot/WebKit/WebKitBuild/Relese/bin32/DumpRenderTree

This is happening because a DOS style path is being passed through a Cygwin "abspath' command, which prefixes the passed path with the current directories absolute path. This is wrong behavior, and should not be used when running under Cygwin or native Windows.
Comment 1 Brent Fulgham 2015-11-11 14:47:28 PST
Created attachment 265320 [details]
Patch
Comment 2 Brent Fulgham 2015-11-11 15:39:21 PST
Committed r192330: <http://trac.webkit.org/changeset/192330>
Comment 3 Alexey Proskuryakov 2015-11-11 16:34:44 PST
Comment on attachment 265320 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:781
> -            self._results_directory = self._filesystem.abspath(option_val)
> +            if sys.platform in ('win32', 'cygwin'):
> +                self._results_directory = option_val
> +            else:
> +                self._results_directory = self._filesystem.abspath(option_val)

Should self._filesystem.abspath do the right thing on Windows? Or do we expect to use Unix style paths most pf the time?
Comment 4 Brent Fulgham 2015-11-11 17:13:43 PST
(In reply to comment #3)
> Comment on attachment 265320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265320&action=review
> 
> > Tools/Scripts/webkitpy/port/base.py:781
> > -            self._results_directory = self._filesystem.abspath(option_val)
> > +            if sys.platform in ('win32', 'cygwin'):
> > +                self._results_directory = option_val
> > +            else:
> > +                self._results_directory = self._filesystem.abspath(option_val)
> 
> Should self._filesystem.abspath do the right thing on Windows? Or do we
> expect to use Unix style paths most pf the time?

This is just a weird edge case. "self._filesystem.abspath" is just a wrapper around the os.filesystem.abspath. On Cygwin that tries to covert things based on "/cygpath/C/..." (or whatever), and since it doesn't understand DOS paths, it doesn't properly combine the two paths.

When we run under native Windows, the abspath would probably do the right thing. We should probably revisit this special case once that work is finished.
Comment 5 Csaba Osztrogonác 2015-11-12 09:37:12 PST
(In reply to comment #2)
> Committed r192330: <http://trac.webkit.org/changeset/192330>

It broke layout testing on the Apple Windows bots:
ServerError raised: Failed to start httpd: httpd.exe: Could not open configuration file C:/xampp/apache/layout-test-results/httpd.conf: The system cannot find the path specified.  

Traceback (most recent call last):
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 77, in main
    run_details = run(port, options, args, stderr)
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 409, in run
    run_details = manager.run(args)
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 196, in run
    int(self._options.child_processes), retrying=False)
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 259, in _run_tests
    return self._runner.run_tests(self._expectations, test_inputs, tests_to_skip, num_workers, needs_http, needs_websockets, needs_web_platform_test_server, retrying)
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 107, in run_tests
    self.start_servers()
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 190, in start_servers
    self._port.start_http_server()
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/port/base.py", line 901, in start_http_server
    server.start()
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py", line 89, in start
    self._pid = self._spawn_process()
  File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py", line 171, in _spawn_process
    raise http_server_base.ServerError('Failed to start %s: %s' % (self._name, err))
ServerError: Failed to start httpd: httpd.exe: Could not open configuration file C:/xampp/apache/layout-test-results/httpd.conf: The system cannot find the path specified.  

No handlers could be found for logger "webkitpy.port.win"




Additionally it broke a webkitpy test on the Apple Windows bots:
[452/1444] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_results_directory_relative failed:
  Traceback (most recent call last):
    File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py", line 625, in test_results_directory_relative
      self.assertEqual(user.opened_urls, [path.abspath_to_uri(host.platform, '/tmp/cwd/foo/results.html')])
  AssertionError: Lists differ: ['file://foo/results.html'] != ['file:///tmp/cwd/foo/results....
  
  First differing element 0:
  file://foo/results.html
  file:///tmp/cwd/foo/results.html
  
  - ['file://foo/results.html']
  + ['file:///tmp/cwd/foo/results.html']
  ?         +++++++++
Comment 6 Brent Fulgham 2015-11-12 09:45:01 PST
(In reply to comment #5)
> (In reply to comment #2)
> > Committed r192330: <http://trac.webkit.org/changeset/192330>
> 
> It broke layout testing on the Apple Windows bots:
> ServerError raised: Failed to start httpd: httpd.exe: Could not open
> configuration file C:/xampp/apache/layout-test-results/httpd.conf: The
> system cannot find the path specified.  
> 

Whoops! I'll correct that.
Comment 7 Brent Fulgham 2015-11-12 12:31:26 PST
Created attachment 265421 [details]
Patch v2
Comment 8 Brent Fulgham 2015-11-12 12:37:26 PST
Comment on attachment 265421 [details]
Patch v2

This patch allows us to give Cygwin paths to Python libraries that expect UNIX style paths. Although we generally want to be dealing in native DOS paths on Windows, when the test infrastructure is run inside Cygwin Python, we need to give Python paths that it can handle properly.
Comment 9 Brent Fulgham 2015-11-12 16:21:09 PST
Committed r192395: <http://trac.webkit.org/changeset/192395>
Comment 10 Csaba Osztrogonác 2015-11-13 00:45:18 PST
(In reply to comment #9)
> Committed r192395: <http://trac.webkit.org/changeset/192395>

Layout testing is stilk broken on the Apple Windows bots.
Comment 11 Brent Fulgham 2015-11-13 09:20:37 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Committed r192395: <http://trac.webkit.org/changeset/192395>
> 
> Layout testing is stilk broken on the Apple Windows bots.

Interesting. The tests run when triggered from the command line. The BuildSlave environment must be set differently.
Comment 12 Brent Fulgham 2015-11-13 09:24:17 PST
I see the problem. We launch Apache with a relative path to the configuration file:

08:18:57.398 6484 Starting httpd server, cmd="C:/xampp/apache/bin/httpd.exe -f "layout-test-results/httpd.conf" -C 'DocumentRoot "C:/cygwin/home/buildbot/slave/win-release-tests/build/LayoutTests/http/tests"' -c 'Alias /js-test-resources "C:/cygwin/home/buildbot/slave/win-release-tests/build/LayoutTests/resources"' -c 'Alias /media-resources "C:/cygwin/home/buildbot/slave/win-release-tests/build/LayoutTests/media"' -c 'TypesConfig "C:/cygwin/home/buildbot/slave/win-release-tests/build/LayoutTests/http/conf/mime.types"' -c 'CustomLog "layout-test-results/access_log.txt" common' -c 'ErrorLog "layout-test-results/error_log.txt"' -c 'PidFile C:/xampp/apache/logs/httpd.pid' -k start -C 'Listen 127.0.0.1:8000' -C 'Listen 127.0.0.1:8080' -C 'Listen 127.0.0.1:8443' -c 'SSLCertificateFile C:/cygwin/home/buildbot/slave/win-release-tests/build/LayoutTests/http/conf/webkit-httpd.pem'"

The -f "layout-tests-results/httpd.conf" is getting converted to the httpd.exe root directory -> "C:/xampp/apache/layout-test-results/httpd.conf".

We should be passing the full path to the 'httpd.conf', since it lives in the test folder.
Comment 13 Brent Fulgham 2015-11-13 11:36:39 PST
Fix landed in <https://trac.webkit.org/changeset/192431>.
Comment 14 Ryan Haddad 2015-11-16 09:41:39 PST
(In reply to comment #13)
> Fix landed in <https://trac.webkit.org/changeset/192431>.

The tests are now running, but they are exiting early after 500 failures. Do you think this could still be fallout from this change?
Comment 15 Csaba Osztrogonác 2015-11-18 02:38:36 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Fix landed in <https://trac.webkit.org/changeset/192431>.
> 
> The tests are now running, but they are exiting early after 500 failures. Do
> you think this could still be fallout from this change?

I think the culprit is this change and the Windows layout 
testing is broken since a week. Brent, are you planning
to fix the regression your patches caused?
Comment 16 Csaba Osztrogonác 2015-11-19 09:20:27 PST
ping?
Comment 17 Ryan Haddad 2015-11-19 09:24:34 PST
(In reply to comment #16)
> ping?

I know that Brent was investigating this yesterday on one of the bots. I'll follow up with him.
Comment 18 Brent Fulgham 2015-11-19 10:43:00 PST
Test machines should be fixed with Commit r192635: <http://trac.webkit.org/changeset/192635>