gemini-code-assist[bot] commented on code in PR #114:
URL: https://github.com/apache/tvm-ffi/pull/114#discussion_r2427959042
##########
rust/tvm-ffi-sys/build.rs:
##########
@@ -61,6 +61,7 @@ fn main() {
println!("cargo:rustc-link-search=native={}", lib_dir);
// link the library
println!("cargo:rustc-link-lib=dylib=tvm_ffi");
+ println!("cargo:rustc-link-lib=dylib=tvm_ffi_testing");
Review Comment:
Linking `tvm_ffi_testing` unconditionally in the `tvm-ffi-sys` crate seems
incorrect. The purpose of splitting `testing.cc` into a separate library is to
make it optional for consumers. By linking it here, any Rust crate that depends
on `tvm-ffi-sys` will also have a dependency on `libtvm_ffi_testing.so`, which
is likely not intended.
This link should probably be conditional, enabled by a Cargo feature (e.g.,
"testing"). This would allow crates that need the testing functions to enable
the feature, without forcing the dependency on all users.
For example:
```rust
// In build.rs
println!("cargo:rustc-link-lib=dylib=tvm_ffi");
#[cfg(feature = "testing")]
println!("cargo:rustc-link-lib=dylib=tvm_ffi_testing");
// In Cargo.toml
[features]
testing = []
```
##########
CMakeLists.txt:
##########
@@ -132,6 +131,30 @@ target_link_libraries(tvm_ffi_objs PUBLIC tvm_ffi_header)
target_link_libraries(tvm_ffi_shared PUBLIC tvm_ffi_header)
target_link_libraries(tvm_ffi_static PUBLIC tvm_ffi_header)
+# ######### Target: `tvm_ffi_testing` ##########
+# Build testing utilities as a separate shared library that can be loaded on
demand
+target_sources(tvm_ffi_testing PRIVATE
"${CMAKE_CURRENT_SOURCE_DIR}/src/ffi/extra/testing.cc")
+target_compile_features(tvm_ffi_testing PRIVATE cxx_std_17)
+set_target_properties(
+ tvm_ffi_testing
+ PROPERTIES CXX_EXTENSIONS OFF
+ CXX_STANDARD_REQUIRED ON
+ PREFIX "lib"
+ LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+)
+target_link_libraries(tvm_ffi_testing PRIVATE tvm_ffi_shared)
+target_link_libraries(tvm_ffi_testing PUBLIC tvm_ffi_header)
+tvm_ffi_add_msvc_flags(tvm_ffi_testing)
+tvm_ffi_add_target_from_obj(tvm_ffi tvm_ffi_objs)
Review Comment:
This call to `tvm_ffi_add_target_from_obj` appears to be a copy-paste error.
The function is already called on line 113 to create the `tvm_ffi_static`,
`tvm_ffi_shared`, and `tvm_ffi_testing` targets. Calling it again here is
redundant and incorrect. Please remove this line.
##########
cmake/Utils/Library.cmake:
##########
@@ -100,6 +100,14 @@ function (tvm_ffi_add_target_from_obj target_name
obj_target_name)
LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
)
+ add_library(${target_name}_testing SHARED)
+ set_target_properties(
+ ${target_name}_testing
+ PROPERTIES OUTPUT_NAME "${target_name}_testing"
+ ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+ LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+ RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+ )
Review Comment:
The function `tvm_ffi_add_target_from_obj`'s name implies that it creates
targets from an object library. While this is true for `_static` and `_shared`
targets, the new `_testing` target is created as an empty shared library here,
with its sources added elsewhere. This makes the function's purpose less clear.
Consider creating the `tvm_ffi_testing` target directly in `CMakeLists.txt`
using `add_library(tvm_ffi_testing SHARED ...)` where its sources are defined.
This would keep the logic for this special-case target self-contained and
maintain the clarity of the `tvm_ffi_add_target_from_obj` helper function.
--
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]
