Use Node instead of d8 for Wasm AOT testing #6356
Conversation
This requires the right version of Node is installed on your system. Since EMSDK often puts a too-old version of Node in the path, allow overriding via an env var.
|
I'll leave it to Alex to look at the CMake changes, but in principle this LGTM. I definitely prefer using node here and this approach makes sense. |
dependencies/wasm/CMakeLists.txt
Outdated
| if (NOT NODE_JS_EXECUTABLE) | ||
| # If HALIDE_NODE_JS_PATH is specified, always look there, ignoring normal lookup paths; | ||
| # otherwise, just rely on find_program(). (This is fairly essential, since many EMSDK versions | ||
| # include a too-old version of Node that is likely to be in the current search path | ||
| # when EMCC is in use.) | ||
| if ("$ENV{HALIDE_NODE_JS_PATH}" STREQUAL "") | ||
| find_program(NODE_JS_EXECUTABLE node) | ||
| else () | ||
| get_filename_component(HALIDE_NODE_JS_DIR $ENV{HALIDE_NODE_JS_PATH} DIRECTORY) | ||
| message(STATUS "HALIDE_NODE_JS_DIR ${HALIDE_NODE_JS_DIR}") | ||
| find_program(NODE_JS_EXECUTABLE node | ||
| NO_DEFAULT_PATH | ||
| PATHS "${HALIDE_NODE_JS_DIR}") | ||
| endif () | ||
|
|
||
| if (NOT NODE_JS_EXECUTABLE) | ||
| message(FATAL_ERROR "Could not find Node.js shell") | ||
| endif () | ||
|
|
||
| if (DEFINED CACHE{NODE_JS_VERSION_OK}) | ||
| message(STATUS "NODE_JS_VERSION_OK is defined") | ||
| return() | ||
| endif() | ||
|
|
||
| execute_process(COMMAND "${NODE_JS_EXECUTABLE}" --version | ||
| OUTPUT_VARIABLE NODE_JS_VERSION_RAW | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
| string(REPLACE "v" "" NODE_JS_VERSION ${NODE_JS_VERSION_RAW}) | ||
| string(REPLACE "." ";" NODE_JS_VERSION ${NODE_JS_VERSION}) | ||
| message(STATUS "Found Node.js runtime at ${NODE_JS_EXECUTABLE}, version ${NODE_JS_VERSION_RAW}") | ||
|
|
||
| list(GET NODE_JS_VERSION 0 NODE_JS_MAJOR) | ||
| list(GET NODE_JS_VERSION 1 NODE_JS_MINOR) | ||
| list(GET NODE_JS_VERSION 2 NODE_JS_PATCH) | ||
|
|
||
| if () | ||
| message(FATAL_ERROR "Halide requires Node v16.13 or later, but found ${NODE_JS_VERSION_RAW} at ${NODE_JS_EXECUTABLE}") | ||
| endif () | ||
|
|
||
| if ((NODE_JS_MAJOR LESS 16) OR ((NODE_JS_MAJOR EQUALS 16) AND (NODE_JS_MINOR LESS 13))) | ||
| message(FATAL_ERROR "Halide requires Node v16.13 or later, but found ${NODE_JS_VERSION_RAW} at ${NODE_JS_EXECUTABLE}. Try setting the HALIDE_NODE_JS_PATH env var or defining NODE_JS_EXECUTABLE on the CMake command line.") | ||
| endif () | ||
|
|
||
| set(NODE_JS_VERSION_OK "YES" CACHE INTERNAL "Node is OK") | ||
| endif () |
There was a problem hiding this comment.
This could be a lot simpler...
find_program(NODE_JS_EXECUTABLE node nodejs)
# TODO: when we eventually upgrade to CMake >= 3.18, replace with REQUIRED in find_program
if (NOT NODE_JS_EXECUTABLE)
message(FATAL_ERROR "Could not find nodejs. Please set NODE_JS_EXECUTABLE on the CMake command line.")
endif ()
execute_process(COMMAND "${NODE_JS_EXECUTABLE}" --version
OUTPUT_VARIABLE NODE_JS_VERSION_RAW
OUTPUT_STRIP_TRAILING_WHITESPACE)
string(REPLACE "v" "" NODE_JS_VERSION ${NODE_JS_VERSION_RAW})
if (NODE_JS_VERSION VERSION_LESS "16.13")
message(FATAL_ERROR "Halide requires Node v16.13 or later, but found ${NODE_JS_VERSION_RAW} at ${NODE_JS_EXECUTABLE}. Please set NODE_JS_EXECUTABLE on the CMake command line.")
endif ()
I don't see a need for a bespoke env-var here... a user has a lot of standard options:
- They can directly set
NODE_JS_EXECUTABLEat the CMake command line (as suggested by the error message). - They can add the directory containing their desired
node[js]to theCMAKE_PROGRAM_PATHenv-var. - They can add a directory containing
[s]bin/node[js]to theCMAKE_PREFIX_PATHenv-var (or cache variable) - They can add the directory containing their desired
node[js]earlier in thePATHenv var.
If you really think none of the above options will work for you, then you could add HINTS "$ENV{HALIDE_NODE_JS_PATH}" to the find_program call. It's okay for that to expand to an empty string and it will be searched before the PATH env-var. Here are the docs for the find_program search procedure: https://cmake.org/cmake/help/v3.16/command/find_program.html
I also used the built-in VERSION_LESS comparison in if() and made the check always run on configure... otherwise you might get into a situation where you change NODE_JS_EXECUTABLE but then don't touch NODE_JS_VERSION_OK... asking node for its version isn't so expensive that it's worth the trouble to avoid.
There was a problem hiding this comment.
CMake code is a bit overwrought
I'd argue that this is generally true no matter what, but yes, thanks very much for the suggestions, PTAL :-)
bespoke env-var
The reason I wanted to offer that is because NODE_JS_EXECUTABLE will currently always need to be set when working with wasm: if you are working with wasm, you have activated the emsdk toolset, and doing so puts an old version of node (14.x) in PATH, which absolutely will fail. It's a lot easier to configure an env var as a retained configuration. That said... the Emscripten folks have committed to upgrading that version of Node to 16.13+ in a future EMSDK release, but it's not clear when that will be. But I guess it's easier to add an env var later than to remove one later, so let's leave it out for now.
built-in VERSION_LESS
Ah, nice, I didn't know that existed!
This is necessary for halide/Halide#6356 to land -- if we are building/testing wasm, we need to set NODE_JS_EXECUTABLE properly; this patch does so based on an env var ('HALIDE_NODE_JS_PATH') which is assumed to be defined on the relevant wokers.
* Define NODE_JS_EXECUTABLE based on an env var This is necessary for halide/Halide#6356 to land -- if we are building/testing wasm, we need to set NODE_JS_EXECUTABLE properly; this patch does so based on an env var ('HALIDE_NODE_JS_PATH') which is assumed to be defined on the relevant wokers. * Update master.cfg
This requires the right version of Node is installed on your system. Since EMSDK often puts a too-old version of Node in the path, allow overriding via an env var.