Bug 187283 – [WinCairo] httpd service install needs to precede server start

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ross Kirsling]

https://trac.webkit.org/r229469 introduced an `httpd -k install` step, which is needed before running HTTP tests from Windows command prompt.

Unfortunately, this step was added as an override to to Port.check_httpd, which is called (via Port.check_sys_deps) *after* Port.start_http_server.

As such, a test bot that has not manually run `httpd -k install` in advance will never reach it:
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/451/steps/layout-test/logs/stdio

We should override Port.start_http_server instead.

[Ross Kirsling]

Created attachment 344159 [details]
Patch

[Daniel Bates]

(In reply to Ross Kirsling from comment #0)
> https://trac.webkit.org/r229469 introduced an `httpd -k install` step, which
> is needed before running HTTP tests from Windows command prompt.
> 
> Unfortunately, this step was added as an override to to Port.check_httpd,
> which is called (via Port.check_sys_deps) *after* Port.start_http_server.
> 


Has this always been the case? This sounds like the bug we should fix. Moving the logic to Port.start_http_server seems() like a workaround for a regression? In the architecture.

[Daniel Bates]

is there reason your’re inclined to override Port.start_http_server() as opposed to fixing the architecture such that Port.check_sys_deps() is called before Port.starthttp_server()?

[Ross Kirsling]

(In reply to Daniel Bates from comment #2)
> Has this always been the case? This sounds like the bug we should fix.
> Moving the logic to Port.start_http_server seems() like a workaround for a
> regression? In the architecture.

Since this issue is WinCairo-specific, I was just aiming for the smallest-impact solution.

The architectural concern seems to come from this patch:
https://bugs.webkit.org/show_bug.cgi?id=172176

Namely, the start_http_server call now originates from the LayoutTestRunner constructor...
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py#L87

...while the call to check_sys_deps is still made later (via Manager._set_up_run) in preparation for each test subset.
The comments around that callsite suggest that we wouldn't want to duplicate it hastily:
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py#L170-L184

It seems like decoupling check_httpd from check_sys_deps could be a possible architectural fix though?

[Fujii Hironori]

(In reply to Ross Kirsling from comment #4)
> Since this issue is WinCairo-specific, I was just aiming for the
> smallest-impact solution.

AppleWin port is also using XAMPP Apache.
The smallest-impact solution is invoking "httpd -k install" after install XAMPP, and remove WinCairo specific code.

https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/tools/windowsservercore/Dockerfile#L71

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #5)
> (In reply to Ross Kirsling from comment #4)
> > Since this issue is WinCairo-specific, I was just aiming for the
> > smallest-impact solution.
> 
> AppleWin port is also using XAMPP Apache.
> The smallest-impact solution is invoking "httpd -k install" after install
> XAMPP, and remove WinCairo specific code.
> 
> https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/tools/
> windowsservercore/Dockerfile#L71

If this is bot-only purpose, that's acceptable, but for everyday development, it is still required patch. It used to be working feature and the bug is introduced by https://bugs.webkit.org/show_bug.cgi?id=172176 . It should be fixed.

[Ross Kirsling]

To reframe:
-

[Ross Kirsling]

To reframe:

- The code currently in WinCairoPort.check_httpd doesn't execute at a useful time, so we do need a patch.

- From Don: This doesn't go in the Dockerfile because not all bots are used for layout testing.

- Based on the current usage of check_sys_deps and check_httpd, I believe it would suffice to move the check_httpd call from Port.check_sys_deps to Port.start_http_server, instead of the current patch.

- If this step is useful for AppleWin as well (i.e. even in a Cygwin context), we could move the override from WinCairoPort to WinPort.

[Ross Kirsling]

Created attachment 344251 [details]
Patch

[Daniel Bates]

Comment on attachment 344251 [details]
Patch

r=me

An Apple engineer will need to make an Internal change following the removal of needs_http from check_sys_deps(). It would be great if you could land this change during Cupertino working hours :)

[WebKit Commit Bot]

Comment on attachment 344251 [details]
Patch

Clearing flags on attachment: 344251

Committed r233651: <https://trac.webkit.org/changeset/233651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41984355>