Bug 172553 – [CMake] Wrap CODE_GENERATOR_PREPROCESSOR_EXECUTABLE on Windows hosts

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

[Don Olmstead]

CODE_GENERATOR_PREPROCESSOR_EXECUTABLE should use an absolute path that is wrapped in quotes when cross compiling on Windows.

[Don Olmstead]

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.

[Don Olmstead]

Comment on attachment 311148 [details]
Patch

Windows bots are good to go.

[Konstantin Tokarev]

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

[Build Bot]

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

[Build Bot]

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

[Don Olmstead]

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.

[Don Olmstead]

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 ?

[Don Olmstead]

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.

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 311245 [details]
Patch

[Konstantin Tokarev]

Created attachment 311270 [details]
Patch

[Don Olmstead]

(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.

[Konstantin Tokarev]

Created attachment 311305 [details]
Patch

[Don Olmstead]

Your patch is fine by me. Still kinda bizarre you can't use shellwords on the whole.

[Konstantin Tokarev]

Path with spaces work on Linux too with my version of patch

[Don Olmstead]

Comment on attachment 311305 [details]
Patch

LGTM

[Brent Fulgham]

Comment on attachment 311305 [details]
Patch

Looks good. r=me.

[WebKit Commit Bot]

Comment on attachment 311305 [details]
Patch

Clearing flags on attachment: 311305

Committed r217506: <http://trac.webkit.org/changeset/217506>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479764>