fix: prevent SIGSEGV crash on Homebrew#118
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSplit cross-platform wheel build/test into OS-specific GitHub Actions jobs: Linux uses explicit Go+Python setup and manual wheel build/testing; macOS uses cibuildwheel (v2.21.3) with pre-build hooks and validates wheels under Homebrew Python. Makefile enables dynamic linking and performs macOS install-name fixups and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant L as build-linux job
participant M as build-macos job (cibuildwheel)
participant S as Artifact Storage
participant P as publish job
Dev->>GH: push / tag
GH->>L: start Linux build
L->>L: setup-go@v5, setup-python@v5\ninstall OS deps, make gopy_build\npython -m build, run distribution test
L->>S: upload wheels-linux
GH->>M: start macOS build
M->>M: run cibuildwheel@v2.21.3\npre-build hooks install Go/Python deps\nbuild macOS wheel(s), validate in Homebrew Python venv
M->>S: upload wheels-macos
GH->>P: trigger publish (needs: build-linux, build-macos)
P->>S: download wheels-*
P->>P: validate & publish dist/*.whl (on tag)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 20 seconds.Comment |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
Bug Report Checklist
If this commit or author is incorrect, reply in this PR with:
Use git blame to identify the right commit and author before posting. See available commands to help comply with our Guidelines. |
|
@holdex pr submit-time 4h |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/distribution-test.yml (2)
33-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
actions/setup-goto v5.The static analysis flagged
actions/setup-go@v4as potentially too old for current GitHub Actions runners. The same applies to line 79. Update both to v5 for better compatibility.Proposed fix
- name: Set up Go - uses: actions/setup-go@v4 + uses: actions/setup-go@v5 with: go-version: '1.24.1'Apply the same change at line 79 in the macOS job.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/distribution-test.yml around lines 33 - 36, Update the GitHub Actions step usages of actions/setup-go from v4 to v5: replace both occurrences of "uses: actions/setup-go@v4" (one in the linux job around the first setup step and the second in the macOS job) with "uses: actions/setup-go@v5" so the workflow uses the latest major release for Go setup in both places.
49-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
actions/setup-pythonto v5.For consistency and better runner compatibility, update to the current major version.
Proposed fix
- name: Set up Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: '3.12'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/distribution-test.yml around lines 49 - 52, Update the GitHub Actions step that sets up Python (the step named "Set up Python" which currently uses actions/setup-python@v4) to use actions/setup-python@v5; replace the uses value with actions/setup-python@v5 and keep the existing python-version input ('3.12') as-is, ensuring the workflow continues to supply the same python-version parameter to the new major version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-build.yml:
- Line 11: The workflow currently uses an overly broad permission setting
(`permissions: write-all`); update the workflow permissions to the least
privilege needed for publishing releases by replacing the global write-all
permission with a scoped permission like `permissions: contents: write` (i.e.,
change the `permissions` entry in the release workflow to only grant contents
write access).
- Around line 22-25: Update the GitHub Actions usage entries for the setup
actions to their v5 majors: replace uses: actions/setup-go@v4 with
actions/setup-go@v5 and replace uses: actions/setup-python@v4 with
actions/setup-python@v5 for each occurrence in the workflow (the three places
where setup-go and setup-python are referenced), ensuring the other with: keys
(e.g., go-version or python-version) remain unchanged.
---
Outside diff comments:
In @.github/workflows/distribution-test.yml:
- Around line 33-36: Update the GitHub Actions step usages of actions/setup-go
from v4 to v5: replace both occurrences of "uses: actions/setup-go@v4" (one in
the linux job around the first setup step and the second in the macOS job) with
"uses: actions/setup-go@v5" so the workflow uses the latest major release for Go
setup in both places.
- Around line 49-52: Update the GitHub Actions step that sets up Python (the
step named "Set up Python" which currently uses actions/setup-python@v4) to use
actions/setup-python@v5; replace the uses value with actions/setup-python@v5 and
keep the existing python-version input ('3.12') as-is, ensuring the workflow
continues to supply the same python-version parameter to the new major version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6809bc97-fca7-439a-937c-d5e72a7d1cb5
📒 Files selected for processing (3)
.github/workflows/distribution-test.yml.github/workflows/release-build.ymlMakefile
|
@holdex bug commit e2859bc && bug author @georgeciubotaru |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
3-3: ⚡ Quick winEnforce the minimum
gopyversion for this new flag.
setup.py:16-23still shells out tomake gopy_buildduring install, so this target now depends on agopy genoption that upstream documents on thegencommand. Ifcheck_dependencies()doesn't reject oldergopybinaries first, source installs will fail at build time with an opaque CLI error. Please either pin/document the minimum supportedgopyversion or validate it before invoking this target. (github.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 3, The Makefile's new gopy gen usage requires a minimum gopy version so add a runtime check in setup.py's check_dependencies() (which is called before the Makefile gopy_build target is invoked) to call `gopy --version`, parse the semantic version, and fail with a clear error instructing the user to upgrade if the installed gopy is older than the minimum supported version (use e.g. >=0.5.0); ensure the error message mentions the gopy version requirement and suggests installation/upgrading steps so source installs fail fast with a helpful message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 19-21: Add a hard assertion after the otool -L checks to fail the
build if either binary still references Python by searching the otool output for
"Python.framework" or "libpython"; run `otool -L
src/trufnetwork_sdk_c_bindings/_trufnetwork_sdk_c_bindings.so` and `otool -L
src/trufnetwork_sdk_c_bindings/trufnetwork_sdk_c_bindings_go.so`, pipe each
output to grep -E for "Python.framework|libpython" and if grep finds a match
exit with non-zero status (causing make to fail) and print a clear error message
indicating which .so still contains Python references so CI cannot pass when
those references remain.
---
Nitpick comments:
In `@Makefile`:
- Line 3: The Makefile's new gopy gen usage requires a minimum gopy version so
add a runtime check in setup.py's check_dependencies() (which is called before
the Makefile gopy_build target is invoked) to call `gopy --version`, parse the
semantic version, and fail with a clear error instructing the user to upgrade if
the installed gopy is older than the minimum supported version (use e.g.
>=0.5.0); ensure the error message mentions the gopy version requirement and
suggests installation/upgrading steps so source installs fail fast with a
helpful message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…assert no libpython link
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-build.yml:
- Around line 88-111: Add a smoke-test step between the "Build wheels with
cibuildwheel" step and the "Upload wheels" step that installs the macOS wheel
from wheelhouse/*.whl into a matching interpreter (use actions/setup-python with
the same Python version as CIBW_BUILD / cp312) and runs a quick import and basic
runtime check (pip install wheelhouse/*.whl && python -c "import
<your_package_module>; print('ok')") so the job fails if the wheel cannot be
loaded on the built interpreter; ensure the step runs per-matrix (uses
matrix.arch/matrix.goarch) and returns a non-zero exit on failure so Upload
wheels only runs for validated wheels.
In `@Makefile`:
- Around line 34-56: The Darwin fixup block in the Makefile (the sequence
calling install_name_tool, otool, and setting GO_SO_OLD) can continue after a
failing command because it runs in one shell with simple semicolons; change it
to fail fast by enabling errexit for that shell or checking each command's exit
status: wrap the Darwin branch commands with "set -e" (or prefix the block with
"set -o errexit") or chain critical calls (install_name_tool, otool, the
install_name_tool -change and -add_rpath invocations) with && so any failure
immediately triggers exit and the make target fails; ensure the GO_SO_OLD
determination and its subsequent install_name_tool -change are guarded so a
missing/failed otool causes an immediate exit rather than continuing with broken
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d46fb917-ed46-4168-8ed0-54abe79b118d
📒 Files selected for processing (3)
.github/workflows/distribution-test.yml.github/workflows/release-build.ymlMakefile
…e block fail-fast
2 participants
resolves: https://github.com/truflation/augustus/issues/2
Summary by CodeRabbit