gemini-code-assist[bot] commented on code in PR #397: URL: https://github.com/apache/tvm-ffi/pull/397#discussion_r2679635814
########## CMakeLists.txt: ########## @@ -124,8 +122,13 @@ endif () tvm_ffi_add_msvc_flags(tvm_ffi_objs) tvm_ffi_add_target_from_obj(tvm_ffi tvm_ffi_objs) -target_link_libraries(tvm_ffi_shared PRIVATE Threads::Threads) -target_link_libraries(tvm_ffi_static INTERFACE Threads::Threads) +find_package(Threads) +if (Threads_FOUND) + # link to threads library if found on some cross-compilation platforms, the threads library maynot + # be found but downstream may still be able to build extra alternatives via source bundle + target_link_libraries(tvm_ffi_shared PRIVATE Threads::Threads) + target_link_libraries(tvm_ffi_static INTERFACE Threads::Threads) +endif () Review Comment:  While making thread support optional is a good goal for cross-compilation, this implementation could cause issues. By changing `find_package(Threads REQUIRED)` to be optional, a configuration failure will now become a less obvious link-time failure on platforms where a thread library is necessary. The `tvm_ffi` library uses `std::mutex` in several files (e.g., `src/ffi/init_once.cc`, `src/ffi/backtrace.cc`), which requires thread support. A more robust solution would be to introduce an explicit CMake option to control this. Here's a suggested approach: 1. **Add a new option** at the top of `CMakeLists.txt`: ```cmake option(TVM_FFI_WITH_THREADS "Build with thread support" ON) ``` 2. **Conditionally find and link threads** based on this option. This would replace the current block: ```cmake if(TVM_FFI_WITH_THREADS) find_package(Threads REQUIRED) target_link_libraries(tvm_ffi_shared PRIVATE Threads::Threads) target_link_libraries(tvm_ffi_static INTERFACE Threads::Threads) endif() ``` 3. **Add a compile definition** to propagate this setting to the C++ code: ```cmake if(TVM_FFI_WITH_THREADS) target_compile_definitions(tvm_ffi_objs PRIVATE TVM_FFI_WITH_THREADS=1) else() target_compile_definitions(tvm_ffi_objs PRIVATE TVM_FFI_WITH_THREADS=0) endif() ``` 4. **Conditionally compile thread-dependent C++ code.** For example, in `src/ffi/init_once.cc`, you would guard the `std::mutex` usage: ```cpp #if TVM_FFI_WITH_THREADS // Current implementation with std::mutex #else // A single-threaded fallback implementation #endif ``` This makes the choice explicit for users cross-compiling for targets without threads (they can set `-DTVM_FFI_WITH_THREADS=OFF`), while preserving the configure-time checks for everyone else. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
