gemini-code-assist[bot] commented on code in PR #351:
URL: https://github.com/apache/tvm-ffi/pull/351#discussion_r2636598126
##########
cmake/Utils/Library.cmake:
##########
@@ -137,3 +136,176 @@ function (tvm_ffi_add_target_from_obj target_name
obj_target_name)
tvm_ffi_add_apple_dsymutil(${target_name}_shared)
tvm_ffi_add_apple_dsymutil(${target_name}_testing)
endfunction ()
+
+# cmake-lint: disable=C0301,R0912,R0915
+# ~~~
+# tvm_ffi_configure_target(target_name [LINK_SHARED ON|OFF] [LINK_HEADER
ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])
+# Configure a target to integrate with TVM-FFI CMake utilities:
+# - Optionally link against tvm_ffi_header and/or tvm_ffi_shared
+# - Always apply tvm_ffi_add_prefix_map(target_name <current source dir>)
+# - Optionally enable Apple dSYM generation via
tvm_ffi_add_apple_dsymutil(target_name)
+# - Optionally apply MSVC-specific flags via
tvm_ffi_add_msvc_flags(target_name)
+#
+# Parameters:
+# target_name: Existing CMake target to modify (positional, required)
+#
+# Keyword parameters (all accept ON/OFF-like values; defaults shown):
+# LINK_SHARED: Whether to link tvm_ffi_shared into the target (default: ON)
+# LINK_HEADER: Whether to link tvm_ffi_header into the target (default: ON)
+# DEBUG_SYMBOL: Whether to enable debug symbol post-processing hooks.
+# On Apple this calls tvm_ffi_add_apple_dsymutil(target_name)
(default: ON)
+# On non-Apple platforms this is currently a no-op unless you
extend it. (default: ON)
+# MSVC_FLAGS: Whether to call tvm_ffi_add_msvc_flags(target_name) to apply
MSVC-specific flags (default: ON)
+#
+# Notes:
+# - Installation is intentionally split out. Use tvm_ffi_install(target_name
DESTINATION <dir>) to install artifacts.
+# - This function requires tvm_ffi_add_prefix_map() to be defined/included.
+# - If LINK_SHARED/LINK_HEADER are ON, the corresponding targets
(tvm_ffi_shared/tvm_ffi_header) must exist.
+# ~~~
+function (tvm_ffi_configure_target target)
+ if (NOT target)
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target: missing target name. "
+ "Usage: tvm_ffi_configure_target(<target> [LINK_SHARED ON|OFF]
[LINK_HEADER ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])"
+ )
+ endif ()
+
+ if (NOT TARGET "${target}")
+ message(FATAL_ERROR "tvm_ffi_configure_target: '${target}' is not an
existing CMake target.")
+ endif ()
+
+ # Parse keyword args after the positional target name.
+ set(tvm_ffi_add_options) # none; require explicit ON/OFF style values
+ set(tvm_ffi_add_oneValueArgs LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS)
+ set(tvm_ffi_add_multiValueArgs)
+
+ cmake_parse_arguments(
+ tvm_ffi_add_ "${tvm_ffi_add_options}" "${tvm_ffi_add_oneValueArgs}"
+ "${tvm_ffi_add_multiValueArgs}" ${ARGN}
+ )
+
+ # Defaults
+ foreach (arg IN ITEMS LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS)
+ if (NOT DEFINED tvm_ffi_add__${arg})
+ set(tvm_ffi_add__${arg} ON)
+ endif ()
+ endforeach ()
+
+ # Always-on prefix map
+ if (COMMAND tvm_ffi_add_prefix_map)
+ tvm_ffi_add_prefix_map("${target}" "${CMAKE_CURRENT_SOURCE_DIR}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): required function
'tvm_ffi_add_prefix_map' is not defined/included."
+ )
+ endif ()
+
+ # LINK_HEADER
+ if (tvm_ffi_add__LINK_HEADER)
+ if (TARGET tvm_ffi_header)
+ target_link_libraries("${target}" PRIVATE tvm_ffi_header)
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): LINK_HEADER requested but
target 'tvm_ffi_header' does not exist."
+ )
+ endif ()
+ endif ()
+
+ # LINK_SHARED
+ if (tvm_ffi_add__LINK_SHARED)
+ if (TARGET tvm_ffi_shared)
+ target_link_libraries("${target}" PRIVATE tvm_ffi_shared)
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): LINK_SHARED requested but
target 'tvm_ffi_shared' does not exist."
+ )
+ endif ()
+ endif ()
+
+ # DEBUG_SYMBOL (default ON). Apple behavior only (hook only; installation
handled by
+ # tvm_ffi_install()).
+ if (tvm_ffi_add__DEBUG_SYMBOL)
+ if (APPLE)
+ if (COMMAND tvm_ffi_add_apple_dsymutil)
+ tvm_ffi_add_apple_dsymutil("${target}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): DEBUG_SYMBOL=ON but
'tvm_ffi_add_apple_dsymutil' is not defined/included."
+ )
+ endif ()
+ endif ()
+ endif ()
+
+ # Optional: MSVC flags
+ if (tvm_ffi_add__MSVC_FLAGS)
+ if (COMMAND tvm_ffi_add_msvc_flags)
+ tvm_ffi_add_msvc_flags("${target}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): MSVC_FLAGS=ON but
'tvm_ffi_add_msvc_flags' is not defined/included."
+ )
+ endif ()
+ endif ()
+endfunction ()
+
+# ~~~
+# tvm_ffi_install(target_name [DESTINATION <dir>])
+# Install TVM-FFI related artifacts for a configured target.
+#
+# Parameters:
+# target_name: Existing CMake target whose artifacts should be installed
+#
+# Keyword parameters:
+# DESTINATION: Install destination directory relative to
CMAKE_INSTALL_PREFIX (default: ".")
+#
+# Behavior:
+# - On Apple, installs the target's dSYM bundle if it exists.
+# This uses generator expressions and OPTIONAL so it does not fail if the
dSYM is absent.
+# - On non-Apple platforms, currently no-op (extend as needed for PDB/DWARF
packaging).
+#
+# Notes:
+# - This function does not create dSYMs; it only installs them if present.
+# Pair it with tvm_ffi_configure_target(... DEBUG_SYMBOL ON) to enable
dSYM generation hooks.
+# ~~~
+function (tvm_ffi_install tvm_ffi_install_target)
+ if (NOT tvm_ffi_install_target)
+ message(
+ FATAL_ERROR
+ "tvm_ffi_install: missing target name. Usage: tvm_ffi_install(<target>
[DESTINATION <dir>])"
+ )
+ endif ()
+
+ if (NOT TARGET "${tvm_ffi_install_target}")
+ message(
+ FATAL_ERROR "tvm_ffi_install: '${tvm_ffi_install_target}' is not an
existing CMake target."
+ )
+ endif ()
+
+ set(tvm_ffi_install_options) # none
+ set(tvm_ffi_install_oneValueArgs DESTINATION)
+ set(tvm_ffi_install_multiValueArgs)
+
+ cmake_parse_arguments(
+ tvm_ffi_install_ "${tvm_ffi_install_options}"
"${tvm_ffi_install_oneValueArgs}"
+ "${tvm_ffi_install_multiValueArgs}" ${ARGN}
+ )
+
+ if (NOT DEFINED tvm_ffi_install__DESTINATION)
+ set(tvm_ffi_install__DESTINATION ".")
+ endif ()
+
+ if (APPLE)
+ # Install target dSYM bundle if present.
+ install(
+ DIRECTORY "$<TARGET_FILE:${tvm_ffi_install_target}>.dSYM"
+ DESTINATION "${tvm_ffi_install__DESTINATION}"
+ OPTIONAL
+ )
+ endif ()
+endfunction ()
Review Comment:
The argument name `tvm_ffi_install_target` is quite verbose. For consistency
with other functions in this file (like `tvm_ffi_configure_target` which uses
`target`), consider using a shorter name like `target`. This improves
readability.
```
function (tvm_ffi_install target)
if (NOT target)
message(
FATAL_ERROR
"tvm_ffi_install: missing target name. Usage:
tvm_ffi_install(<target> [DESTINATION <dir>])"
)
endif ()
if (NOT TARGET "${target}")
message(
FATAL_ERROR "tvm_ffi_install: '${target}' is not an existing CMake
target."
)
endif ()
set(tvm_ffi_install_options) # none
set(tvm_ffi_install_oneValueArgs DESTINATION)
set(tvm_ffi_install_multiValueArgs)
cmake_parse_arguments(
tvm_ffi_install_ "${tvm_ffi_install_options}"
"${tvm_ffi_install_oneValueArgs}"
"${tvm_ffi_install_multiValueArgs}" ${ARGN}
)
if (NOT DEFINED tvm_ffi_install__DESTINATION)
set(tvm_ffi_install__DESTINATION ".")
endif ()
if (APPLE)
# Install target dSYM bundle if present.
install(
DIRECTORY "$<TARGET_FILE:${target}>.dSYM"
DESTINATION "${tvm_ffi_install__DESTINATION}"
OPTIONAL
)
endif ()
endfunction ()
```
##########
cmake/Utils/Library.cmake:
##########
@@ -137,3 +136,176 @@ function (tvm_ffi_add_target_from_obj target_name
obj_target_name)
tvm_ffi_add_apple_dsymutil(${target_name}_shared)
tvm_ffi_add_apple_dsymutil(${target_name}_testing)
endfunction ()
+
+# cmake-lint: disable=C0301,R0912,R0915
+# ~~~
+# tvm_ffi_configure_target(target_name [LINK_SHARED ON|OFF] [LINK_HEADER
ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])
+# Configure a target to integrate with TVM-FFI CMake utilities:
+# - Optionally link against tvm_ffi_header and/or tvm_ffi_shared
+# - Always apply tvm_ffi_add_prefix_map(target_name <current source dir>)
+# - Optionally enable Apple dSYM generation via
tvm_ffi_add_apple_dsymutil(target_name)
+# - Optionally apply MSVC-specific flags via
tvm_ffi_add_msvc_flags(target_name)
+#
+# Parameters:
+# target_name: Existing CMake target to modify (positional, required)
+#
+# Keyword parameters (all accept ON/OFF-like values; defaults shown):
+# LINK_SHARED: Whether to link tvm_ffi_shared into the target (default: ON)
+# LINK_HEADER: Whether to link tvm_ffi_header into the target (default: ON)
+# DEBUG_SYMBOL: Whether to enable debug symbol post-processing hooks.
+# On Apple this calls tvm_ffi_add_apple_dsymutil(target_name)
(default: ON)
+# On non-Apple platforms this is currently a no-op unless you
extend it. (default: ON)
+# MSVC_FLAGS: Whether to call tvm_ffi_add_msvc_flags(target_name) to apply
MSVC-specific flags (default: ON)
+#
+# Notes:
+# - Installation is intentionally split out. Use tvm_ffi_install(target_name
DESTINATION <dir>) to install artifacts.
+# - This function requires tvm_ffi_add_prefix_map() to be defined/included.
+# - If LINK_SHARED/LINK_HEADER are ON, the corresponding targets
(tvm_ffi_shared/tvm_ffi_header) must exist.
+# ~~~
+function (tvm_ffi_configure_target target)
+ if (NOT target)
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target: missing target name. "
+ "Usage: tvm_ffi_configure_target(<target> [LINK_SHARED ON|OFF]
[LINK_HEADER ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])"
+ )
+ endif ()
+
+ if (NOT TARGET "${target}")
+ message(FATAL_ERROR "tvm_ffi_configure_target: '${target}' is not an
existing CMake target.")
+ endif ()
+
+ # Parse keyword args after the positional target name.
+ set(tvm_ffi_add_options) # none; require explicit ON/OFF style values
+ set(tvm_ffi_add_oneValueArgs LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS)
+ set(tvm_ffi_add_multiValueArgs)
+
+ cmake_parse_arguments(
+ tvm_ffi_add_ "${tvm_ffi_add_options}" "${tvm_ffi_add_oneValueArgs}"
+ "${tvm_ffi_add_multiValueArgs}" ${ARGN}
+ )
+
+ # Defaults
+ foreach (arg IN ITEMS LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS)
+ if (NOT DEFINED tvm_ffi_add__${arg})
+ set(tvm_ffi_add__${arg} ON)
+ endif ()
+ endforeach ()
+
+ # Always-on prefix map
+ if (COMMAND tvm_ffi_add_prefix_map)
+ tvm_ffi_add_prefix_map("${target}" "${CMAKE_CURRENT_SOURCE_DIR}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): required function
'tvm_ffi_add_prefix_map' is not defined/included."
+ )
+ endif ()
+
+ # LINK_HEADER
+ if (tvm_ffi_add__LINK_HEADER)
+ if (TARGET tvm_ffi_header)
+ target_link_libraries("${target}" PRIVATE tvm_ffi_header)
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): LINK_HEADER requested but
target 'tvm_ffi_header' does not exist."
+ )
+ endif ()
+ endif ()
+
+ # LINK_SHARED
+ if (tvm_ffi_add__LINK_SHARED)
+ if (TARGET tvm_ffi_shared)
+ target_link_libraries("${target}" PRIVATE tvm_ffi_shared)
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): LINK_SHARED requested but
target 'tvm_ffi_shared' does not exist."
+ )
+ endif ()
+ endif ()
+
+ # DEBUG_SYMBOL (default ON). Apple behavior only (hook only; installation
handled by
+ # tvm_ffi_install()).
+ if (tvm_ffi_add__DEBUG_SYMBOL)
+ if (APPLE)
+ if (COMMAND tvm_ffi_add_apple_dsymutil)
+ tvm_ffi_add_apple_dsymutil("${target}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): DEBUG_SYMBOL=ON but
'tvm_ffi_add_apple_dsymutil' is not defined/included."
+ )
+ endif ()
+ endif ()
+ endif ()
+
+ # Optional: MSVC flags
+ if (tvm_ffi_add__MSVC_FLAGS)
+ if (COMMAND tvm_ffi_add_msvc_flags)
+ tvm_ffi_add_msvc_flags("${target}")
+ else ()
+ message(
+ FATAL_ERROR
+ "tvm_ffi_configure_target(${target}): MSVC_FLAGS=ON but
'tvm_ffi_add_msvc_flags' is not defined/included."
+ )
+ endif ()
+ endif ()
+endfunction ()
+
+# ~~~
+# tvm_ffi_install(target_name [DESTINATION <dir>])
+# Install TVM-FFI related artifacts for a configured target.
+#
+# Parameters:
+# target_name: Existing CMake target whose artifacts should be installed
+#
+# Keyword parameters:
+# DESTINATION: Install destination directory relative to
CMAKE_INSTALL_PREFIX (default: ".")
+#
+# Behavior:
+# - On Apple, installs the target's dSYM bundle if it exists.
+# This uses generator expressions and OPTIONAL so it does not fail if the
dSYM is absent.
+# - On non-Apple platforms, currently no-op (extend as needed for PDB/DWARF
packaging).
+#
+# Notes:
+# - This function does not create dSYMs; it only installs them if present.
+# Pair it with tvm_ffi_configure_target(... DEBUG_SYMBOL ON) to enable
dSYM generation hooks.
Review Comment:
This documentation refers to `tvm_ffi_configure_target`. If that function is
renamed to `tvm_ffi_add` as suggested in another comment, please update this
line accordingly.
```
Pair it with tvm_ffi_add(... DEBUG_SYMBOL ON) to enable dSYM generation
hooks.
```
##########
examples/packaging/CMakeLists.txt:
##########
@@ -14,62 +14,16 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
cmake_minimum_required(VERSION 3.18)
project(my_ffi_extension)
-option(TVM_FFI_EXT_FROM_SOURCE "Build tvm_ffi from source, useful for cross
compilation." ON)
-option(TVM_FFI_EXT_SHIP_DEBUG_SYMBOLS "Ship debug symbols" ON)
-
-# There are two ways to include tvm_ffi
-#
-# 1. Build tvm_ffi from source, which is reasonably cheap since tvm ffi is
small
-# 2. Use the pre-built tvm_ffi shipped from the pip
-#
-# This example shows both options, you only need to pick a specific one.
-#
-# * For common build cases, using pre-built and link tvm_ffi_shared is
sufficient.
-# * For cases where you may want to cross-compile or bundle part of
tvm_ffi_objects directly into
-# your project, opt for building tvm_ffi from source path. Note that it is
always safe to build
-# from source and extra cost of building tvm_ffi is small. So when in doubt,
you can always choose
-# to the building tvm_ffi from source route.
-#
-# In python or other cases when we dynamically load libtvm_ffi_shared. Even
when you build from
-# source, you do not need to ship libtvm_ffi.so built here as they are only
used to supply the
-# linking information. first find python related components
find_package(
Python
COMPONENTS Interpreter
REQUIRED
)
-if (TVM_FFI_BUILD_FROM_SOURCE)
- execute_process(
- COMMAND "${Python_EXECUTABLE}" -m tvm_ffi.config --sourcedir
- OUTPUT_STRIP_TRAILING_WHITESPACE
- OUTPUT_VARIABLE tvm_ffi_ROOT
- )
- message(STATUS "Building tvm_ffi from source: ${tvm_ffi_ROOT}")
- add_subdirectory(${tvm_ffi_ROOT} tvm_ffi)
-else ()
- # tvm_ffi is installed via pip. Try to find tvm_ffi automatically
- find_package(tvm_ffi CONFIG REQUIRED)
-endif ()
-
-# use the projects as usual
+find_package(tvm_ffi CONFIG REQUIRED)
add_library(my_ffi_extension SHARED src/extension.cc)
-target_link_libraries(my_ffi_extension tvm_ffi_header)
-target_link_libraries(my_ffi_extension tvm_ffi_shared)
-
-if (TVM_FFI_EXT_SHIP_DEBUG_SYMBOLS)
- # ship debugging symbols for backtrace on macos
- tvm_ffi_add_prefix_map(my_ffi_extension ${CMAKE_CURRENT_SOURCE_DIR})
- tvm_ffi_add_apple_dsymutil(my_ffi_extension)
- install(
- DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/
- DESTINATION .
- FILES_MATCHING
- PATTERN "*.dSYM"
- )
-endif ()
-
+tvm_ffi_configure_target(my_ffi_extension)
Review Comment:
Following the suggestion to rename `tvm_ffi_configure_target` to
`tvm_ffi_add` for consistency, this call should be updated.
```
tvm_ffi_add(my_ffi_extension)
```
##########
cmake/Utils/Library.cmake:
##########
@@ -137,3 +136,176 @@ function (tvm_ffi_add_target_from_obj target_name
obj_target_name)
tvm_ffi_add_apple_dsymutil(${target_name}_shared)
tvm_ffi_add_apple_dsymutil(${target_name}_testing)
endfunction ()
+
+# cmake-lint: disable=C0301,R0912,R0915
+# ~~~
+# tvm_ffi_configure_target(target_name [LINK_SHARED ON|OFF] [LINK_HEADER
ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])
+# Configure a target to integrate with TVM-FFI CMake utilities:
+# - Optionally link against tvm_ffi_header and/or tvm_ffi_shared
+# - Always apply tvm_ffi_add_prefix_map(target_name <current source dir>)
+# - Optionally enable Apple dSYM generation via
tvm_ffi_add_apple_dsymutil(target_name)
+# - Optionally apply MSVC-specific flags via
tvm_ffi_add_msvc_flags(target_name)
+#
+# Parameters:
+# target_name: Existing CMake target to modify (positional, required)
+#
+# Keyword parameters (all accept ON/OFF-like values; defaults shown):
+# LINK_SHARED: Whether to link tvm_ffi_shared into the target (default: ON)
+# LINK_HEADER: Whether to link tvm_ffi_header into the target (default: ON)
+# DEBUG_SYMBOL: Whether to enable debug symbol post-processing hooks.
+# On Apple this calls tvm_ffi_add_apple_dsymutil(target_name)
(default: ON)
+# On non-Apple platforms this is currently a no-op unless you
extend it. (default: ON)
+# MSVC_FLAGS: Whether to call tvm_ffi_add_msvc_flags(target_name) to apply
MSVC-specific flags (default: ON)
+#
+# Notes:
+# - Installation is intentionally split out. Use tvm_ffi_install(target_name
DESTINATION <dir>) to install artifacts.
+# - This function requires tvm_ffi_add_prefix_map() to be defined/included.
+# - If LINK_SHARED/LINK_HEADER are ON, the corresponding targets
(tvm_ffi_shared/tvm_ffi_header) must exist.
+# ~~~
+function (tvm_ffi_configure_target target)
Review Comment:
The function name `tvm_ffi_configure_target` is inconsistent with the pull
request description and title, which refer to it as `tvm_ffi_add`. The PR also
draws parallels to `pybind11_add_module`, making `tvm_ffi_add` a more idiomatic
name. Furthermore, the `cmake_parse_arguments` call within this function uses
the prefix `tvm_ffi_add_`, which is confusing.
I recommend renaming the function to `tvm_ffi_add` for better clarity and
consistency. This would require updating:
- The function definition here.
- The documentation block (lines 142-164).
- All error messages within this function.
```
function (tvm_ffi_add target)
```
--
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]
