gemini-code-assist[bot] commented on code in PR #19659:
URL: https://github.com/apache/tvm/pull/19659#discussion_r3345340049
##########
.github/actions/build-wheel-for-publish/action.yml:
##########
@@ -129,16 +129,20 @@ runs:
# env overrides replace rather than merge, so there is no shared base
block.
CIBW_ENVIRONMENT_MACOS: >-
CMAKE_PREFIX_PATH="/opt/llvm"
- CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }} ${{ inputs.include_cuda_runtime == '1' &&
'-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so'
|| '' }}"
+ CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }}"
CIBW_ENVIRONMENT_LINUX: >-
CMAKE_PREFIX_PATH="/opt/llvm"
LIBRARY_PATH="/opt/llvm/lib"
- CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }} ${{ inputs.include_cuda_runtime == '1' &&
'-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so'
|| '' }}"
+ CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }}
-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so"
CIBW_ENVIRONMENT_WINDOWS: >-
CMAKE_PREFIX_PATH="C:/opt/llvm/Library"
PATH="C:/opt/llvm/Library/bin;$PATH"
- CMAKE_ARGS="-DUSE_LLVM='C:/opt/llvm/Library/bin/llvm-config.exe
--link-static' -DZLIB_USE_STATIC_LIBS=ON
-DCMAKE_PREFIX_PATH=C:/opt/llvm/Library ${{ inputs.cmake_defines }} ${{
inputs.include_cuda_runtime == '1' && format('-DTVM_PACKAGE_EXTRA_LIBS={0}',
env.TVM_CUDA_EXTRA_LIB) || '' }}"
- # Tells tests/python/wheel to assert the CUDA runtime is bundled
- # (only on the CUDA wheels; the value is "0" for CPU wheels).
+ CMAKE_ARGS="-DUSE_LLVM='C:/opt/llvm/Library/bin/llvm-config.exe
--link-static' -DZLIB_USE_STATIC_LIBS=ON
-DCMAKE_PREFIX_PATH=C:/opt/llvm/Library ${{ inputs.cmake_defines }}
-DTVM_PACKAGE_EXTRA_LIBS=${{ env.TVM_CUDA_EXTRA_LIB }}"
Review Comment:
Passing `-DTVM_PACKAGE_EXTRA_LIBS` unconditionally on Windows will result in
`-DTVM_PACKAGE_EXTRA_LIBS=` (an empty value) when building CPU-only wheels
(`inputs.include_cuda_runtime == '0'`), because `env.TVM_CUDA_EXTRA_LIB` is
only defined when `include_cuda_runtime` is `'1'`. This can lead to CMake
parsing errors or unexpected behavior.\n\nPlease restore the conditional check
so that this flag is only passed when `include_cuda_runtime` is enabled.
```yaml
CMAKE_ARGS="-DUSE_LLVM='C:/opt/llvm/Library/bin/llvm-config.exe
--link-static' -DZLIB_USE_STATIC_LIBS=ON
-DCMAKE_PREFIX_PATH=C:/opt/llvm/Library ${{ inputs.cmake_defines }} ${{
inputs.include_cuda_runtime == '1' && format('-DTVM_PACKAGE_EXTRA_LIBS={0}',
env.TVM_CUDA_EXTRA_LIB) || '' }}"
```
##########
.github/actions/build-wheel-for-publish/action.yml:
##########
@@ -129,16 +129,20 @@ runs:
# env overrides replace rather than merge, so there is no shared base
block.
CIBW_ENVIRONMENT_MACOS: >-
CMAKE_PREFIX_PATH="/opt/llvm"
- CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }} ${{ inputs.include_cuda_runtime == '1' &&
'-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so'
|| '' }}"
+ CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }}"
CIBW_ENVIRONMENT_LINUX: >-
CMAKE_PREFIX_PATH="/opt/llvm"
LIBRARY_PATH="/opt/llvm/lib"
- CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }} ${{ inputs.include_cuda_runtime == '1' &&
'-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so'
|| '' }}"
+ CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }}
-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so"
Review Comment:
Passing `-DTVM_PACKAGE_EXTRA_LIBS` unconditionally on Linux will cause CMake
to look for `/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so` even when
building CPU-only wheels (`inputs.include_cuda_runtime == '0'`). Since the CUDA
build step is skipped for CPU wheels, this file will not exist, which can cause
CMake configuration or build failures.\n\nPlease restore the conditional check
so that this flag is only passed when `include_cuda_runtime` is enabled.
```yaml
CMAKE_ARGS="-DUSE_LLVM='/opt/llvm/bin/llvm-config --link-static'
-DZLIB_USE_STATIC_LIBS=ON -DCMAKE_PREFIX_PATH=/opt/llvm ${{
inputs.cmake_defines }} ${{ inputs.include_cuda_runtime == '1' &&
'-DTVM_PACKAGE_EXTRA_LIBS=/project/build-wheel-cuda/lib/libtvm_runtime_cuda.so'
|| '' }}"
```
##########
pyproject.toml:
##########
@@ -213,14 +213,14 @@ docstring-code-line-length = "dynamic"
skip = "*-win32 *-manylinux_i686 *-musllinux*"
build-verbosity = 1
test-requires = ["pytest", "numpy"]
-test-command = "pytest -p no:tvm.testing.plugin -vvs
{project}/tests/python/wheel && pytest -vvs
{project}/tests/python/all-platform-minimal-test"
+test-command = "pytest -vvs {project}/tests/python/all-platform-minimal-test"
[tool.cibuildwheel.linux]
repair-wheel-command = "auditwheel repair --exclude libtvm_ffi.so --exclude
libtvm_runtime_cuda.so --exclude 'libcuda.so.*' --exclude 'libcudart.so.*'
--exclude 'libnvrtc.so.*' --exclude 'libnvrtc-builtins.so.*' -w {dest_dir}
{wheel}"
[tool.cibuildwheel.macos]
-repair-wheel-command = 'delocate-wheel --ignore-missing-dependencies --exclude
libtvm_ffi.dylib --require-archs {delocate_archs} -w {dest_dir} -v {wheel}'
+repair-wheel-command = '''bash -c 'set -euo pipefail; r="$(mktemp -d)";
delocate-wheel --ignore-missing-dependencies --exclude libtvm_ffi.dylib
--require-archs {delocate_archs} -w "$r" -v "{wheel}"; python -m pip install -q
wheel; u="$(mktemp -d)"; python -m wheel unpack "$r"/*.whl -d "$u"; find "$u"
-type f \( -name "*.dylib" -o -name "*.so" \) -print0 | xargs -0 -n1 codesign
--force --sign -; python -m wheel pack "$u"/*/ -d "{dest_dir}"' '''
Review Comment:
The PR description mentions a new script
`ci/scripts/package/macos_repair_wheel.sh` that handles the macOS wheel repair
and re-signing. However, this script is missing from the PR, and the entire
complex bash command has been inlined into `pyproject.toml`
instead.\n\nInlining a multi-step bash command (which creates temporary
directories, runs `delocate-wheel`, installs `wheel`, unpacks, codesigns, and
repacks) makes it very difficult to read, maintain, and debug.\n\nPlease
extract this logic into the script file
`ci/scripts/package/macos_repair_wheel.sh` as originally planned, and reference
it here.
```suggestion
repair-wheel-command = "bash
{project}/ci/scripts/package/macos_repair_wheel.sh {wheel} {dest_dir}
{delocate_archs}"
```
--
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]
