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.
Created attachment 344159 [details] Patch
(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.
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()?
(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?
(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
(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.
To reframe: -
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.
Created attachment 344251 [details] Patch
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 :)
Comment on attachment 344251 [details] Patch Clearing flags on attachment: 344251 Committed r233651: <https://trac.webkit.org/changeset/233651>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41984355>