ci: remove unnecessary Python setup from macOS CI#584
Conversation
Fix macOS-latest CI failure in demo tests caused by missing python-3.12-embed. The test_demo.sh script requires python-3.12-embed for pkg-config to create the python3-embed symlink, but macOS-latest defaults to Python 3.13. This change ensures python@3.12 is installed via Homebrew in the setup-llcppg action for macOS runners, allowing the demo tests to complete successfully. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @luoliwoshang, 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 aims to stabilize the macOS CI pipeline by rectifying a version mismatch issue. The demo tests, which rely on Python 3.12 for embedding, were failing because the macOS-latest environment defaulted to Python 3.13, causing critical Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the macOS CI failure by installing python@3.12. The change is simple and effective. I've added one suggestion to consolidate two of the brew install commands for better conciseness. Overall, this is a good fix.
| brew install zlib # for llgo test . | ||
| brew install python@3.12 # for pydemo tests |
|
Overall Assessment: Approved with minor documentation suggestion This PR correctly addresses the macOS CI failure by installing Python 3.12. The change is minimal, well-placed, and solves the immediate issue. I've left one inline comment suggesting a more accurate comment to better reflect the actual use case (python-3.12-embed pkg-config support rather than just "pydemo tests"). Note: The |
| run: | | ||
| brew install llvm@${{inputs.llvm}} bdw-gc openssl libffi libuv lld@${{inputs.llvm}} | ||
| brew install zlib # for llgo test . | ||
| brew install python@3.12 # for pydemo tests |
There was a problem hiding this comment.
The comment "for pydemo tests" is somewhat misleading. The actual purpose is to provide the python-3.12-embed pkg-config module, as shown in go.yml lines 79-88 where it's used to create a python3-embed symlink workaround for PyTorch compatibility.
Consider updating to be more specific:
| brew install python@3.12 # for pydemo tests | |
| brew install python@3.12 # required for python-3.12-embed pkg-config module |
This better reflects the actual use case and aligns with the TODO comment in go.yml explaining the PyTorch/Python 3.13 compatibility issue.
|
Overall Assessment: Approved with minor documentation suggestion This PR correctly addresses the macOS CI failure by installing Python 3.12. The change is minimal, well-placed, and solves the immediate issue. I've left one inline comment suggesting a more accurate comment to better reflect the actual use case (python-3.12-embed pkg-config support rather than just "pydemo tests"). Note: The |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
==========================================
+ Coverage 84.19% 84.74% +0.54%
==========================================
Files 27 27
Lines 2746 2314 -432
==========================================
- Hits 2312 1961 -351
+ Misses 391 310 -81
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Since python@3.12 is not automatically linked by Homebrew when other Python versions exist, we need to explicitly add its pkgconfig directory to PKG_CONFIG_PATH before running pkg-config. This ensures pkg-config can find python-3.12-embed.pc even when python@3.12 is installed but not symlinked to /usr/local. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes: - Add || true to brew install python@3.12 for error tolerance - Use brew link --overwrite to force symlink python@3.12 to /usr/local - This ensures pkg-config can find python-3.12-embed without additional PKG_CONFIG_PATH setup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Since python@3.12 is now force-linked with brew link --overwrite, pkg-config can find it directly without additional PKG_CONFIG_PATH setup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Since the project doesn't have _pydemo directory and doesn't require Python dependencies, remove all Python-related setup code: - Remove python@3.12 installation from macOS dependencies - Remove python3-embed symlink workaround from test_demo step This simplifies the CI configuration and removes unused dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Fixes #585
Remove unnecessary Python 3.12 setup from macOS CI that was causing demo tests to fail with exit code 1.
Changes
.github/actions/setup-llcppg/action.ymlbrew install python@3.12 || truebrew link --overwrite python@3.12.github/workflows/go.ymlWhy This Works
The original code attempted to create a symlink for
python3-embedpointing topython-3.12-embedfor PyTorch compatibility. However:_pydemodirectory_demo/cjsondemoexists, which doesn't require Pythonpkg-config --variable=libdir python-3.12-embedcommand was failing because Python 3.12 wasn't installedBenefits
Test Plan
🤖 Generated with Claude Code