Bug 172553 - [CMake] Wrap CODE_GENERATOR_PREPROCESSOR_EXECUTABLE on Windows hosts
Summary: [CMake] Wrap CODE_GENERATOR_PREPROCESSOR_EXECUTABLE on Windows hosts
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: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-24 13:00 PDT by Don Olmstead
Modified: 2017-05-30 20:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.99 KB, patch)
2017-05-24 13:14 PDT, Don Olmstead
achristensen: review+
annulen: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1011.38 KB, application/zip)
2017-05-24 14:39 PDT, Build Bot
no flags Details
Patch (2.35 KB, patch)
2017-05-24 18:41 PDT, Don Olmstead
don.olmstead: review-
don.olmstead: commit-queue-
Details | Formatted Diff | Diff
Patch with Shellwords (4.17 KB, patch)
2017-05-24 19:32 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2017-05-25 10:28 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2017-05-25 10:46 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2017-05-25 12:59 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2017-05-25 15:42 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-05-24 13:00:48 PDT
CODE_GENERATOR_PREPROCESSOR_EXECUTABLE should use an absolute path that is wrapped in quotes when cross compiling on Windows.
Comment 1 Don Olmstead 2017-05-24 13:14:20 PDT
Created attachment 311148 [details]
Patch

Uses CMAKE_HOST_WIN32 https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_WIN32.html to identify a build happening on a Windows machine. This detects cygwin as well. From there the command is wrapped in quotes just in case there are spaces.

This is useful for anyone cross compiling WebKit on Windows such as what we're doing on the PlayStation.

Also removed a stale comment.

Going to wait on the apple win bots before reviewing.
Comment 2 Don Olmstead 2017-05-24 14:14:58 PDT
Comment on attachment 311148 [details]
Patch

Windows bots are good to go.
Comment 3 Konstantin Tokarev 2017-05-24 14:29:57 PDT
Comment on attachment 311148 [details]
Patch

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

> Source/cmake/OptionsCommon.cmake:5
> +if (CMAKE_HOST_WIN32)

I think this is a wrong logic. I don't see any reason why should we take absolute path only on Windows, and not use it on other platforms
Comment 4 Build Bot 2017-05-24 14:39:23 PDT
Comment on attachment 311148 [details]
Patch

Attachment 311148 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3808717

New failing tests:
fast/events/before-unload-returnValue.html
webrtc/peer-connection-audio-mute.html
Comment 5 Build Bot 2017-05-24 14:39:24 PDT
Created attachment 311155 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 6 Don Olmstead 2017-05-24 18:41:14 PDT
Created attachment 311177 [details]
Patch

Quotes the preprocessor path for everyone. CMAKE_CXX_COMPILER is the full path so there's no reason to get its absolute path, https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER.html.
Comment 7 Don Olmstead 2017-05-24 19:08:57 PDT
The GTK bot doesn't like this change annulen. Maybe it needs to be combined with your shellwords patch at https://bugs.webkit.org/show_bug.cgi?id=164484 ?
Comment 8 Don Olmstead 2017-05-24 19:32:39 PDT
Created attachment 311182 [details]
Patch with Shellwords

Lets see what happens when shellwords is used.

If this doesn't work then I think we should just go back to the windows host check.
Comment 9 Don Olmstead 2017-05-25 10:28:59 PDT
Created attachment 311241 [details]
Patch

GTK fails without shellwords and AppleWin fails with shellwords. This is going back to roughly the original patch submitted but with the knowledge that ${CMAKE_CXX_COMPILER} already has the absolute path.
Comment 10 Don Olmstead 2017-05-25 10:46:25 PDT
Created attachment 311245 [details]
Patch
Comment 11 Konstantin Tokarev 2017-05-25 12:59:45 PDT
Created attachment 311270 [details]
Patch
Comment 12 Don Olmstead 2017-05-25 15:32:24 PDT
(In reply to Konstantin Tokarev from comment #11)
> Created attachment 311270 [details]
> Patch

The failures on Win are indicating that the call to the preprocessor isn't actually generating anything.

You're correct that its possible that another system has spaces in the path but the shellwords stuff isn't working out.

My only other idea is that rather than splitting on " " the arguments become a CMake list rather than a string and then split on ";".

Unless you want to spend some more time with this I think we should just land my last patch.
Comment 13 Konstantin Tokarev 2017-05-25 15:42:13 PDT
Created attachment 311305 [details]
Patch
Comment 14 Don Olmstead 2017-05-26 12:26:01 PDT
Your patch is fine by me. Still kinda bizarre you can't use shellwords on the whole.
Comment 15 Konstantin Tokarev 2017-05-26 12:58:57 PDT
Path with spaces work on Linux too with my version of patch
Comment 16 Don Olmstead 2017-05-26 13:03:53 PDT
Comment on attachment 311305 [details]
Patch

LGTM
Comment 17 Brent Fulgham 2017-05-26 13:59:35 PDT
Comment on attachment 311305 [details]
Patch

Looks good. r=me.
Comment 18 WebKit Commit Bot 2017-05-26 14:28:38 PDT
Comment on attachment 311305 [details]
Patch

Clearing flags on attachment: 311305

Committed r217506: <http://trac.webkit.org/changeset/217506>
Comment 19 WebKit Commit Bot 2017-05-26 14:28:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2017-05-30 20:23:45 PDT
<rdar://problem/32479764>