Fix quoting of CMAKE_CROSSCOMPILING_EMULATOR #14233
Conversation
sbc100
force-pushed
the
fix_cmake
branch
2 times, most recently
from
1b263ff to
a192463
Compare
|
@sbc100 Looks like your latest force-push removed the fix and left only the tests. |
|
Ooops, Hilarious! I was temporarily reverting the fix to find where the test was failing.. doh! |
This actually fixes two different failures, one was the quoting
in the CMakeLists.txt file itself, which seems to have become
more strict with later versions of cmake reporting:
```
CMake Error at CMakeLists.txt:7 (add_custom_command):
COMMAND may not contain literal quotes:
"/usr/local/google/home/sbc/dev/wasm/emsdk/node/14.15.5_64bit/bin/node"
```
This error does not occur with cmake version 3.13, for example.
The second issue is with the actual runing of the tests. Even on
older versions of cmake this was failing with:
```
make[2]: "/usr/local/google/home/sbc/dev/wasm/emsdk/node/14.15.5_64bit/bin/node": No such file or directory
make[2]: *** [CMakeFiles/hello.dir/build.make:86: hello.js] Error 127
make[1]: *** [CMakeFiles/Makefile2:73: CMakeFiles/hello.dir/all] Error 2
```
Fixes: #10028
|
|
||
| if not has_substr(args, '-DCMAKE_CROSSCOMPILING_EMULATOR'): | ||
| node_js = config.NODE_JS[0].replace('"', '\"') | ||
| args.append('-DCMAKE_CROSSCOMPILING_EMULATOR="%s"' % node_js) | ||
| args.append(f'-DCMAKE_CROSSCOMPILING_EMULATOR={node_js}') |
There was a problem hiding this comment.
this is identical to before, just without quotes, is that right?
why is this better? and, won't it break if the path to node has spaces, for example?
There was a problem hiding this comment.
Yes the fix is to remove the quote marks which were causing issues with CMake, especially on newer version.
Because args is being directly to subprocess.call there is no need to quote spaces.. cmake will see this is one big single argument.. there is no shell parsing this string before it goes to cmake. I will verify locally with a space in my node path just to be sure..
There was a problem hiding this comment.
Verified that spaces in node path work fine after this change.
| @@ -34,11 +34,10 @@ def has_substr(args, substr): | |||
| # Append the Emscripten toolchain file if the user didn't specify one. | |||
| if not has_substr(args, '-DCMAKE_TOOLCHAIN_FILE'): | |||
| args.append('-DCMAKE_TOOLCHAIN_FILE=' + utils.path_from_root('cmake', 'Modules', 'Platform', 'Emscripten.cmake')) | |||
| node_js = config.NODE_JS | |||
|
|
|||
| if not has_substr(args, '-DCMAKE_CROSSCOMPILING_EMULATOR'): | |||
| node_js = config.NODE_JS[0].replace('"', '\"') | |||
There was a problem hiding this comment.
Hmmm.. I just noticed that this line does not do what the author thinks it does :-/ This is a no-op. I will followup about that.
|
|
||
| if not has_substr(args, '-DCMAKE_CROSSCOMPILING_EMULATOR'): | ||
| node_js = config.NODE_JS[0].replace('"', '\"') | ||
| args.append('-DCMAKE_CROSSCOMPILING_EMULATOR="%s"' % node_js) | ||
| args.append(f'-DCMAKE_CROSSCOMPILING_EMULATOR={node_js}') |
This actually fixes two different failures, one was the quoting
in the CMakeLists.txt file itself, which seems to have become
more strict with later versions of cmake reporting:
This error does not occur with cmake version 3.13, for example.
The second issue is with the actual runing of the tests. Even on
older versions of cmake this was failing with:
Fixes: #10028