gemini-code-assist[bot] commented on code in PR #337: URL: https://github.com/apache/tvm-ffi/pull/337#discussion_r2613174617
########## .github/actions/build-wheel-for-publish/action.yml: ########## @@ -0,0 +1,115 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Build Wheel For Publish +description: > + Build and test wheels (and optionally sdists) for a given OS/architecture/manylinux combination, + suitable for both validation and publish workflows. + +inputs: + os: + description: "Runner operating system (e.g., ubuntu-latest, macos-14, windows-latest)" + required: true + arch: + description: "Target architecture for cibuildwheel (e.g., x86_64, aarch64, arm64, AMD64)" + required: true + linux_image: + description: "Manylinux image tag to use on Linux runners (empty string for non-Linux runners)" + required: true + checkout_ref: + description: "Branch, tag, or SHA to check out before building" + required: true + build_wheels: + description: "Set to false to skip wheel builds (useful for sdist-only runs)" + default: "true" + required: false + build_sdist: + description: "Set to true to build a source distribution and run metadata checks" + default: "false" + required: false + +runs: + using: "composite" + steps: + # Special handling for macOS arm64 + python 3.8. + # Install Python 3.8 from `actions/setup-python`, which is an arm64 build. + - name: Install Python 3.8 for macOS arm64 + if: runner.os == 'macOS' && inputs.arch == 'arm64' + uses: actions/setup-python@v5 Review Comment:  For security and reproducibility, it's a best practice to pin GitHub Actions to a specific commit SHA instead of a major version tag. This prevents your workflow from breaking unexpectedly due to updates in the action and mitigates the risk of supply chain attacks. You've already done this for `astral-sh/setup-uv`, and it would be good to be consistent. ```yaml uses: actions/setup-python@82c7e631bb3cdc910f68e0081d67478d79c6982d # v5.1.0 ``` ########## .github/actions/build-wheel-for-publish/action.yml: ########## @@ -0,0 +1,115 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Build Wheel For Publish +description: > + Build and test wheels (and optionally sdists) for a given OS/architecture/manylinux combination, + suitable for both validation and publish workflows. + +inputs: + os: + description: "Runner operating system (e.g., ubuntu-latest, macos-14, windows-latest)" + required: true + arch: + description: "Target architecture for cibuildwheel (e.g., x86_64, aarch64, arm64, AMD64)" + required: true + linux_image: + description: "Manylinux image tag to use on Linux runners (empty string for non-Linux runners)" + required: true + checkout_ref: + description: "Branch, tag, or SHA to check out before building" + required: true + build_wheels: + description: "Set to false to skip wheel builds (useful for sdist-only runs)" + default: "true" + required: false + build_sdist: + description: "Set to true to build a source distribution and run metadata checks" + default: "false" + required: false + +runs: + using: "composite" + steps: + # Special handling for macOS arm64 + python 3.8. + # Install Python 3.8 from `actions/setup-python`, which is an arm64 build. + - name: Install Python 3.8 for macOS arm64 + if: runner.os == 'macOS' && inputs.arch == 'arm64' + uses: actions/setup-python@v5 + with: + python-version: 3.8 + + - name: Set up uv + if: ${{ !(runner.os == 'macOS' && inputs.arch == 'arm64') }} + uses: astral-sh/setup-uv@b75a909f75acd358c2196fb9a5f1299a9a8868a4 # v6.7.0 + + - name: Check out source + uses: actions/checkout@v5 Review Comment:  The `actions/checkout` action does not have a `v5` tag; the latest major version is `v4`. Using `@v5` will cause this workflow to fail. Please update it to `@v4`. For improved security, you might also consider pinning to a specific commit hash. ```yaml uses: actions/checkout@v4 ``` ########## .github/actions/build-wheel-for-publish/action.yml: ########## @@ -0,0 +1,115 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Build Wheel For Publish +description: > + Build and test wheels (and optionally sdists) for a given OS/architecture/manylinux combination, + suitable for both validation and publish workflows. + +inputs: + os: + description: "Runner operating system (e.g., ubuntu-latest, macos-14, windows-latest)" + required: true + arch: + description: "Target architecture for cibuildwheel (e.g., x86_64, aarch64, arm64, AMD64)" + required: true + linux_image: + description: "Manylinux image tag to use on Linux runners (empty string for non-Linux runners)" + required: true + checkout_ref: + description: "Branch, tag, or SHA to check out before building" + required: true + build_wheels: + description: "Set to false to skip wheel builds (useful for sdist-only runs)" + default: "true" + required: false + build_sdist: + description: "Set to true to build a source distribution and run metadata checks" + default: "false" + required: false + +runs: + using: "composite" + steps: + # Special handling for macOS arm64 + python 3.8. + # Install Python 3.8 from `actions/setup-python`, which is an arm64 build. + - name: Install Python 3.8 for macOS arm64 + if: runner.os == 'macOS' && inputs.arch == 'arm64' + uses: actions/setup-python@v5 + with: + python-version: 3.8 + + - name: Set up uv + if: ${{ !(runner.os == 'macOS' && inputs.arch == 'arm64') }} + uses: astral-sh/setup-uv@b75a909f75acd358c2196fb9a5f1299a9a8868a4 # v6.7.0 + + - name: Check out source + uses: actions/checkout@v5 + with: + ref: ${{ inputs.checkout_ref }} + submodules: recursive + fetch-depth: 0 + fetch-tags: true + + - uses: ./.github/actions/detect-env-vars + id: env_vars + + - name: Print current commit + shell: bash + run: git log -1 --oneline + + - name: Run cpp tests + if: runner.os != 'Windows' + env: + CMAKE_BUILD_PARALLEL_LEVEL: ${{ steps.env_vars.outputs.cpu_count }} + shell: bash + run: | + cmake . -B build_test -DTVM_FFI_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug + cmake --build build_test --clean-first --config Debug --target tvm_ffi_tests + ctest -V -C Debug --test-dir build_test --output-on-failure + + - name: Run cpp tests[windows] + if: ${{ matrix.os == 'windows-latest' }} Review Comment:  The `if` condition uses `matrix.os`, but this composite action is not guaranteed to be run within a matrix strategy. To ensure this step runs correctly on Windows runners, you should use the `runner.os` context, which is consistent with the non-Windows step (`if: runner.os != 'Windows'`). Also, the `${{...}}` wrapper is not necessary when the entire value is an expression. ```yaml if: runner.os == 'Windows' ``` -- 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]
