Stream: git-wasmtime

Topic: wasmtime / PR #9933 Add return code checking in CMakeList...

Wasmtime GitHub notifications bot (Jan 06 2025 at 17:38):

ifsheldon opened PR #9933 from ifsheldon:fix-silent-error-part-1 to bytecodealliance:main:

This attempts to partially solve #9932, adding checking return code of the executed command cargo build, which can fail, for example, if a target has not been added.

I don't know whether there are similar cases somewhere else, in which the return code of an executed command is simply ignored.

In this file, however, when I asked Claude "are there anything else that will cause silent failures in this file", it says the following. I think what it says makes sense, but I don't know much about cmake, so I don't know if it's correct.
<details>
Yes, there are a few potential sources of silent failures in this CMake file:

1. The create_rust_test function also uses cargo run without checking return codes:

function(CREATE_RUST_TEST EXAMPLE)
    # These tests might fail silently during test execution
    if(ARGC GREATER 1)
        add_test(NAME ${EXAMPLE}-rust COMMAND cargo run --example ${EXAMPLE} --features ${ARGV1} WORKING_DIRECTORY ../..)
    else()
        add_test(NAME ${EXAMPLE}-rust COMMAND cargo run --example ${EXAMPLE} WORKING_DIRECTORY ../..)
    endif()
endfunction()

2. The add_subdirectory command at the beginning might fail silently if the directory doesn't exist:

add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/../crates/c-api ${CMAKE_CURRENT_BINARY_DIR}/wasmtime)

3. In the CREATE_TARGET function, the linking against CoreFoundation on Apple platforms doesn't verify if the framework exists:

if(APPLE)
    target_link_libraries(wasmtime-${TARGET} PRIVATE "-framework CoreFoundation")
endif()

To make the file more robust, you could:

1. Add existence checks for required directories and files
2. Add REQUIRED to framework findings
3. Set test properties to fail on non-zero return codes:

function(CREATE_RUST_TEST EXAMPLE)
    if(ARGC GREATER 1)
        add_test(NAME ${EXAMPLE}-rust
                COMMAND cargo run --example ${EXAMPLE} --features ${ARGV1}
                WORKING_DIRECTORY ../..)
    else()
        add_test(NAME ${EXAMPLE}-rust
                COMMAND cargo run --example ${EXAMPLE}
                WORKING_DIRECTORY ../..)
    endif()
    # Make the test fail if cargo returns non-zero
    set_tests_properties(${EXAMPLE}-rust PROPERTIES WILL_FAIL FALSE)
endfunction()

Also, it's worth noting that the relative paths (../..) in WORKING_DIRECTORY could be fragile if the directory structure changes. It would be more robust to use absolute paths or CMake variables to reference directories.
</details>

Wasmtime GitHub notifications bot (Jan 06 2025 at 17:38):

ifsheldon requested wasmtime-core-reviewers for a review on PR #9933.

Wasmtime GitHub notifications bot (Jan 06 2025 at 17:38):

ifsheldon requested alexcrichton for a review on PR #9933.

Wasmtime GitHub notifications bot (Jan 06 2025 at 21:29):

alexcrichton commented on PR #9933:

Thanks! I think that execute_process probably isn't the right thing here but rather it's best to set up a "target" (I think that's the right CMake terminology) which is based on executing the process instead. That would also defer execution to the test step rather than running it during the configure step as well.

If you're not comfortable with CMake though I'm happy to make this change too. (I don't know what/who Claude is myself)

Wasmtime GitHub notifications bot (Jan 07 2025 at 02:29):

ifsheldon commented on PR #9933:

Yeah, please help, as this change might affect many test cases checking examples working.

Wasmtime GitHub notifications bot (Jan 07 2025 at 18:04):

alexcrichton commented on PR #9933:

I've done some refactoring in https://github.com/bytecodealliance/wasmtime/pull/9940

Wasmtime GitHub notifications bot (Jan 07 2025 at 22:41):

alexcrichton closed without merge PR #9933.

Last updated: Jan 24 2025 at 00:11 UTC