Update workflow to release spannerlib-python to PyPI#682
Update workflow to release spannerlib-python to PyPI#682
Conversation
…directories and update artifact naming conventions.
… Git Bash on Windows.
…#675) * feat: Add GitHub Actions workflows for building shared SpannerLib binaries and a Python wrapper, including support for Linux ARM64 and macOS AMD64. * feat: Add GitHub Actions workflows for Python Spanner library wrapper linting, unit, and system tests. * feat: Refactor shared library build process to support additional architectures, improve cross-compilation, and centralize build logic. (#677)
Summary of ChangesHello @bhatt4982, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the build and release workflow for the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the shared library build process for spannerlib, expanding support to include linux/arm64 and darwin/amd64 in addition to existing linux/x64, darwin/arm64, and windows/x64 targets. The build-binaries.sh script was modularized with log and build_artifact functions, and improved logic for native vs. cross-compilation, including checks for required cross-compilers. The build-shared-lib.sh script for the Python wrapper was updated to delegate fully to build-binaries.sh and now copies artifacts directly to an architecture-specific google/cloud/spannerlib/internal/lib directory, removing an intermediate ARTIFACTS_DIR. Correspondingly, spannerlib.py was modified to dynamically determine the shared library filename based on both OS and architecture (e.g., linux-x64/spannerlib.so), and unit tests were updated to reflect these new paths. The noxfile.py was refactored to use a new _build_artifacts helper, which calls the updated build-shared-lib.sh script, and includes a run_bash_script helper for Windows compatibility. The pyproject.toml file was updated to change the project name to spannerlib-python. A review comment highlighted a potential issue in setup.py where the removal of has_ext_modules=lambda: True might lead to the generation of platform-agnostic Python wheels, which is problematic for a package containing platform-specific binaries. Another review comment suggested simplifying the Linux cross-compilation logic in build-binaries.sh to only attempt cross-compilation if the toolchain is present, avoiding confusing fallback attempts.
| setup( | ||
| has_ext_modules=lambda: True, | ||
| include_package_data=True, | ||
| ) |
There was a problem hiding this comment.
The removal of has_ext_modules=lambda: True is likely to cause setuptools to generate a platform-agnostic 'pure Python' wheel (e.g., ...-py3-none-any.whl). Since this package bundles a pre-compiled, platform-specific shared library, this is incorrect. Such a wheel would install on any platform but only run on the one it was built for, causing runtime errors for other users.
To ensure pip can install the correct wheel for a user's OS and architecture, the generated wheel must be tagged as platform-specific. The has_ext_modules argument is a known way to force this behavior. Please restore it or ensure that platform-specific wheels are being generated through another mechanism.
| setup( | |
| has_ext_modules=lambda: True, | |
| include_package_data=True, | |
| ) | |
| setup( | |
| has_ext_modules=lambda: True, | |
| include_package_data=True, | |
| ) |
| if [ -z "$SKIP_LINUX_CROSS_COMPILE" ]; then | ||
| # Check for cross-compiler | ||
| if command -v x86_64-unknown-linux-gnu-gcc >/dev/null 2>&1; then | ||
| build_artifact "linux" "amd64" "binaries/linux-x64" "spannerlib.so" "x86_64-unknown-linux-gnu-gcc" | ||
| else | ||
| log "WARNING: x86_64-unknown-linux-gnu-gcc not found. Skipping Linux x64 cross-compile." | ||
| fi | ||
| elif [ -z "$SKIP_LINUX" ]; then | ||
| # Fallback to standard build if cross-compile explicitly skipped but Linux not skipped | ||
| # This might fail if no suitable compiler is found, but we'll try. | ||
| build_artifact "linux" "amd64" "binaries/linux-x64" "spannerlib.so" "" | ||
| fi |
There was a problem hiding this comment.
The logic in this block for handling Linux cross-compilation can be simplified and made more robust. The elif block attempts a fallback build that will likely fail on a non-Linux host (like macOS) if a C cross-compiler isn't available, as go build for a c-shared library requires one. This can lead to confusing build failures.
A better approach is to only attempt cross-compilation if the toolchain is present and SKIP_LINUX_CROSS_COMPILE is not set. If cross-compilation is skipped on a non-Linux host, the Linux build for that architecture should also be skipped.
| if [ -z "$SKIP_LINUX_CROSS_COMPILE" ]; then | |
| # Check for cross-compiler | |
| if command -v x86_64-unknown-linux-gnu-gcc >/dev/null 2>&1; then | |
| build_artifact "linux" "amd64" "binaries/linux-x64" "spannerlib.so" "x86_64-unknown-linux-gnu-gcc" | |
| else | |
| log "WARNING: x86_64-unknown-linux-gnu-gcc not found. Skipping Linux x64 cross-compile." | |
| fi | |
| elif [ -z "$SKIP_LINUX" ]; then | |
| # Fallback to standard build if cross-compile explicitly skipped but Linux not skipped | |
| # This might fail if no suitable compiler is found, but we'll try. | |
| build_artifact "linux" "amd64" "binaries/linux-x64" "spannerlib.so" "" | |
| fi | |
| if [ -z "$SKIP_LINUX_CROSS_COMPILE" ]; then | |
| # Check for cross-compiler | |
| if command -v x86_64-unknown-linux-gnu-gcc >/dev/null 2>&1; then | |
| build_artifact "linux" "amd64" "binaries/linux-x64" "spannerlib.so" "x86_64-unknown-linux-gnu-gcc" | |
| else | |
| log "WARNING: x86_64-unknown-linux-gnu-gcc not found. Skipping Linux x64 cross-compile." | |
| fi | |
| fi |
1 participant
Verified on