gemini-code-assist[bot] commented on code in PR #122: URL: https://github.com/apache/tvm-ffi/pull/122#discussion_r2430872140
########## CMakeLists.txt: ########## @@ -148,6 +148,15 @@ target_link_libraries(tvm_ffi_testing PUBLIC tvm_ffi_header) tvm_ffi_add_msvc_flags(tvm_ffi_testing) tvm_ffi_add_apple_dsymutil(tvm_ffi_testing) +# Set the install RPATH for tvm_ffi_testing so it can find tvm_ffi_shared.so relatively +if (APPLE) + # macOS uses @loader_path + set_target_properties(tvm_ffi_testing PROPERTIES INSTALL_RPATH "@loader_path") +elseif (LINUX) + # Linux uses $ORIGIN + set_target_properties(tvm_ffi_testing PROPERTIES INSTALL_RPATH "\$ORIGIN") +endif () Review Comment:  This block for setting `INSTALL_RPATH` is a good addition. However, there are a couple of ways it could be improved for better maintainability and portability. 1. **Portability**: The `elseif (LINUX)` check is specific to Linux. Many other Unix-like systems (e.g., BSDs) also support `$ORIGIN` in RPATH. It would be more portable to use `elseif (UNIX AND NOT APPLE)`. 2. **Code Duplication**: This logic is duplicated from lines 259-265, where `INSTALL_RPATH` is set for the `tvm_ffi_cython` target. To avoid this duplication, you could abstract the logic into a reusable function in `cmake/Utils/Library.cmake`. This would also make it easier to maintain if support for more platforms is added in the future. I'd recommend at least applying the portability fix. Here is a suggestion for that: ``` if (APPLE) # macOS uses @loader_path set_target_properties(tvm_ffi_testing PROPERTIES INSTALL_RPATH "@loader_path") elseif (UNIX AND NOT APPLE) # Most Unix-like systems use $ORIGIN set_target_properties(tvm_ffi_testing PROPERTIES INSTALL_RPATH "\$ORIGIN") endif () ``` -- 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]
