corrade_add_test() on Emscripten clashes with CMAKE_CROSSCOMPILING_EMULATOR

[pezcode]

When CMAKE_CROSSCOMPILING_EMULATOR is set to the path to nodejs, corrade_add_test() finds and prepends nodejs a second time manually to add_test(). This leads to node trying to load its own binary and failing:

/emsdk/node/14.18.2_64bit/bin/node:1
�ELF����
^
SyntaxError: Invalid or unexpected token

Not sure what "the" correct solution is here. What came to my mind:

• Use CMAKE_CROSSCOMPILING_EMULATOR in Corrade's toolchain and remove the special handling from UseCorrade.cmake. Might break for some users who rarely update their toolchains.
• put the code in UseCorrade.cmake in between if(NOT CMAKE_CROSSCOMPILING_EMULATOR), a bit ugly but it should work

The official Emscripten toolchain file sets CMAKE_CROSSCOMPILING_EMULATOR, I think compatibility with it is fairly important. We've been using the Corrade toolchain, but the official one has a few advantages and works well these days, so we'd prefer to use that.

[mosra]

I think compatibility with it is fairly important

Yes.

but the official one has a few advantages and works well these days

😢 Anything else I could add to mine, apart from the bloated load of automagic crap that's there?

remove the special handling from UseCorrade.cmake. Might break for some users who rarely update their toolchains.

I'm for this option. Toolchains breakage is fine, it breaks every second Emscripten update anyway. Plus, in many cases people are using a toolchain from within a Corrade submodule, so they get the update implicitly.

Additionally, if you're willing to make a PR for this, you could attempt to do the same treatment for the corrade-rc binary. It's been on my TODO list for a while, to not require an external build of it if CMAKE_CROSSCOMPILING_EMULATOR is set. Related is also #81.

[mosra]

Actually, I think the right solution is implementing both options. Especially in combination with corrade-rc being run through Node.js, which is anything but fast.

So, in particular:

• Add an if(NOT CMAKE_CROSSCOMPILING_EMULATOR) to UseCorrade.cmake so it still works with older toolchains, or when one explicitly disables that. For example, it should be possible to compile for Emscripten even without having that damn bloaty thing installed, and thus CMAKE_CROSSCOMPILING_EMULATOR may not be set at all.
• Then, make corrade-rc built and used if CMAKE_CROSSCOMPILING_EMULATOR is set and if CORRADE_RC_EXECUTABLE is not present (or it's not found). I.e., prefer the native version when possible, to not make corrade_add_resource slow compile #94 even worse than it is with large files.
• Then, make the toolchain attempt to find Node.js and set it as CMAKE_CROSSCOMPILING_EMULATOR if found. CMAKE_DISABLE_FIND_PACKAGE_NodeJs should work in this case and do the expected thing.

[mosra]

I began implementing this but ultimately I cannot reproduce the behavior you're describing, at all. The current code in UseCorrade is the following:

add_test(NAME ${test_name} COMMAND NodeJs::NodeJs ${experimental_wasm_simd} $<TARGET_FILE:${test_name}> ${arguments})

Note the $<TARGET_FILE:${test_name}>. The add_test() documentation says that "if <command> is a target created by add_executable(), [the emulator gets used etc.]". The <command> here is NodeJs::NodeJs and in my case at least, NodeJs::NodeJs is created by add_executable(), but an IMPORTED one. Which apparently doesn't count, so it doesn't prepend the value of CMAKE_CROSSCOMPILING_EMULATOR to this. And the ${test_name} isn't put there verbatim, so it shouldn't trigger use of the emulator either.

In order to actually make CMake prepend the emulator, I have to change to the following:

add_test(NAME ${test_name} COMMAND ${test_name} ${arguments})

But with this I lose the capability to pass arbitrary flags to Node.js, like used to be that flag to enable the SIMD features, and which I suspect I'll need again at some point in the future.

So I don't know what exactly is going on in your case, but the behavior I'm seeing at least, feels correct.

• Does some other FindNodeJs module get used?
• Or maybe this was some strangeness in CMake behavior that got fixed since? I'm on 3.30.4.
• Or are you experiencing this when using emcmake and other Emscripten automagic? If that's the case, I'm afraid something in there is doing something fishy, which then unfortunately makes it blow up on my end.

[pezcode]

Or maybe this was some strangeness in CMake behavior that got fixed since? I'm on 3.30.4.

I'm using the same version here 🍇 We use cmake_minimum_required(VERSION 3.16), this might be some policy added in 3.16 or earlier being set to ON by that?

For completeness sake, I'm testing with Emscripten 3.1.33, and we're on this version of Corrade if it makes a difference.

Tests are run with just ctest --verbose -R TestName.

Or are you experiencing this when using emcmake and other Emscripten automagic?

No, just cmake with -DCMAKE_TOOLCHAIN_FILE=/emsdk/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake

Does some other FindNodeJs module get used?

We use a slightly extended FindNodeJs.cmake, but the part that finds the executable is identical to the one in Corrade, including the IMPORTED_LOCATION:

find_program(NODEJS_EXECUTABLE node)
mark_as_advanced(NODEJS_EXECUTABLE)

if(NODEJS_EXECUTABLE)
    execute_process(COMMAND ${NODEJS_EXECUTABLE} --version
        OUTPUT_VARIABLE NodeJs_VERSION
        ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE)
    if(NodeJs_VERSION MATCHES "^v[0-9]")
        string(SUBSTRING ${NodeJs_VERSION} 1 -1 NodeJs_VERSION)
    else()
        unset(NodeJs_VERSION)
    endif()
endif()

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(NodeJs
    REQUIRED_VARS NODEJS_EXECUTABLE
    VERSION_VAR NodeJs_VERSION)

if(NOT TARGET NodeJs::NodeJs)
    add_executable(NodeJs::NodeJs IMPORTED)
    set_property(TARGET NodeJs::NodeJs PROPERTY IMPORTED_LOCATION ${NODEJS_EXECUTABLE})
endif()

[pezcode]

The only thing that looks related at all is this: https://cmake.org/cmake/help/latest/policy/CMP0158.html But that would only matter if you have that policy enabled (e.g. via cmake_minimum_required(VERSION 3.30)) while testing on your end and not setting CMAKE_SYSTEM_NAME.

[mosra]

42: Test command: /usr/bin/node "/usr/bin/node" "/home/mosra/Code/corrade/build-emscripten-own-tool/bin/ContainersPairCpp14Test.js"
42: Working Directory: /home/mosra/Code/corrade/build-emscripten-own-tool/src/Corrade/Containers/Test
42: Test timeout computed to be: 10000000
42: /usr/bin/node:1
42: ELF
42: ^
42: 
42: SyntaxError: Invalid or unexpected token

Woo hoo. So it's indeed only when using their toolchain. Told ya

[mosra]

I have no idea where is the root cause, but something in their toolchain file makes CMake think that the NodeJs target is local to the project and needs to be run through an emulator, and in my case CMake doesn't think that. Actually I know now, the problem was that in my case find_package(NodeJs) was done in the toolchain file already, so very early on, which made CMake think NodeJs::NodeJs isn't a project-local target. With their toolchain file, the NodeJs::NodeJs target is created only after the project() call, and thus, well, is project-local. If I change my toolchain file to not create the NodeJs target but rather just find the nodejs executable, I can reproduce that as well.

In conclusion I think this is a CMake bug, because it shouldn't try to do this for imported targets. In add_custom_command() sources they already have such a check: https://github.com/Kitware/CMake/blob/6e569ad186ae1467efd13a26d6642cac300faa29/Source/cmCustomCommandGenerator.cxx#L295-L296 For add_test() seems like not, but I can't find where it's actually done, and don't feel like opening an issue for that either... Too much of a corner case.

The fix boils down to doing this, which replaces the target with the executable name.

diff --git a/modules/UseCorrade.cmake b/modules/UseCorrade.cmake
index 900a63c35..79619fa40 100644
--- a/modules/UseCorrade.cmake
+++ b/modules/UseCorrade.cmake
@@ -504,7 +504,7 @@ function(corrade_add_test test_name)
             if(NodeJs_VERSION VERSION_LESS 17)
                 set(experimental_wasm_simd --experimental-wasm-simd)
             endif()
-            add_test(NAME ${test_name} COMMAND NodeJs::NodeJs ${experimental_wasm_simd} $<TARGET_FILE:${test_name}> ${arguments})
+            add_test(NAME ${test_name} COMMAND $<TARGET_FILE:NodeJs::NodeJs> ${experimental_wasm_simd} $<TARGET_FILE:${test_name}> ${arguments})
 
             # Embed all files
             foreach(file ${files})

[pezcode]

Thanks for investigating, and great that the fix is so simple 👍 I can't even find much info on what IMPORTED executables should or should not be doing differently. CMake docs are very murky sometimes.

[mosra]

The above patch is pushed as d2dd8c4.

Besides that, as of 868a502 it's possible to build on Emscripten without having a native corrade-rc available, if CORRADE_CROSSCOMPILING_EMULATOR is set. The same case is for dependent projects, just be sure to update your copy of FindCorrade.cmake (in case you have a copy, that is).