Open
Conversation
Collaborator
Author
|
@greptile review this PR and update the PR summary |
Collaborator
Author
|
@greptile |
Collaborator
Author
|
TODO: @zhanglei1949 Remove the |
Committed-by: xiaolei.zl from Dev container Committed-by: xiaolei.zl from Dev container minor Committed-by: xiaolei.zl from Dev container fix Committed-by: xiaolei.zl from Dev container Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container Committed-by: xiaolei.zl from Dev container
f058db5 to
74a0af0
Compare
Committed-by: nengli.ln from Dev container
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable macos building
Greptile Summary
This PR refactors the wheel-packaging CI to enable macOS builds by activating previously disabled
build_wheels_macos_x86_64andbuild_wheels_macos_aarch64jobs, upgrading their runner images frommacos-13/macos-14tomacos-15-intel/macos-15, and fixingnproc→sysctl -n hw.ncpufor macOS CPU detection. It also adds atags: 'v*'push trigger tobuild-wheel.yml, sprinkles arm -rf /__w/_tool/Pythoncleanup step across self-hosted runner jobs in multiple workflows, and fixes a Bash array syntax bug inscripts/install_deps.sh.Key concerns:
tags: 'v*'trigger is paired with the existingpathsfilter. GitHub evaluatespathsfor tag pushes against the tagged commit's diff — if a release tag points to a commit that only bumps a version file not in thepathslist (e.g.,NEUG_VERSION), the wheel workflow will silently not run and no release wheels will be published.if: true, which runs expensive GitHub-hosted macOS runners on every PR and every push tomain— not just on release tags or workflow_dispatch as the commented-out condition intended.MACOSX_DEPLOYMENT_TARGET=10.15is set in the ARM64 macOS job; macOS 10.15 predates Apple Silicon and the minimum valid target for ARM64 is11.0.build-nightly-wheel.ymlwere not updated and will be broken (wrong runner images,nprocon macOS, wrong deployment target) when they are re-enabled.Confidence Score: 2/5
paths+tagsinteraction that risks silently skipping release wheel builds, and theMACOSX_DEPLOYMENT_TARGET=10.15mismatch on ARM64.pathsfilter paired withtags: 'v*'can silently prevent wheel builds on version tags;MACOSX_DEPLOYMENT_TARGET=10.15is architecturally invalid for ARM64; andif: truewill incur unnecessary macOS runner costs on every PR. Thescripts/install_deps.shfix and self-hosted cleanup steps are correct improvements..github/workflows/build-wheel.ymlrequires the most attention — specifically thepaths/tagstrigger interaction and the ARM64 deployment target..github/workflows/build-nightly-wheel.ymlmacOS jobs also need to be brought in sync before they are re-enabled.Important Files Changed
if: false→if: true; introduces atags: 'v*'push trigger whosepathsfilter could silently suppress the workflow on release tag pushes;if: trueruns expensive macOS runners on every PR and push to main;MACOSX_DEPLOYMENT_TARGET=10.15is set for the ARM64 job where11.0is required.build-wheel.yml— they will be broken when enabled.rm -rf /__w/_tool/Pythoncleanup step beforeactions/setup-pythonin the two self-hosted Linux jobs; both jobs useruns-on: [self-hosted, linux, ...]so the step is correctly targeted.runs-on: [ubuntu-22.04](single-element array) →runs-on: ubuntu-22.04(scalar); no functional impact.rm -rf /__w/_tool/Pythoncleanup step to thebuild_and_testjob, which runs on a[self-hosted]runner; step is correctly scoped to self-hosted infra.("xsimd", "cmake")(which produced a stray trailing comma on the first element, causingbrew install xsimd,to fail) to("xsimd" "cmake").Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Push / PR / workflow_dispatch] --> B{Event type?} B -->|push to main + paths match| C[Linux x86_64\nself-hosted] B -->|push to main + paths match| D[Linux ARM64\nself-hosted] B -->|push v* tag + paths match| C B -->|push v* tag + paths match| D B -->|if: true — all events| E[macOS x86_64\nmacos-15-intel] B -->|if: true — all events| F[macOS ARM64\nmacos-15] B -->|pull_request + paths match| C B -->|pull_request + paths match| D C --> G[Clean /__w/_tool/Python] D --> H[Clean /__w/_tool/Python] G --> I[Setup Python 3.13] H --> J[Setup Python 3.13\nif runner.name != arm64-1] E --> K[Install deps\nscripts/install_deps.sh] F --> L[Install deps\nscripts/install_deps.sh] I --> M[cibuildwheel] J --> M K --> M L --> M M --> N[Upload .whl artifact] N --> O{Publish condition} O -->|main push OR v* tag| P[PyPI publish] O -->|otherwise| Q[skip] E --> R[test_wheels_macos_x86_64\nmacos-15-intel] F --> S[test_wheels_macos_aarch64\nmacos-15] N --> R N --> SComments Outside Diff (3)
.github/workflows/build-wheel.yml, line 221-227 (link)nprocunavailable on macOS — build will failThe "Build wheels" step for
build_wheels_macos_x86_64callsexport CMAKE_BUILD_PARALLEL_LEVEL=$(nproc), butnprocis a Linux-only utility and does not exist on macOS runners. When the shell encounters$(nproc), it will exit with "command not found" and the entire step will fail, preventing any macOS wheel from being built.The "Prepare for macos" step (a few steps above) already sets
CMAKE_BUILD_PARALLEL_LEVELcorrectly via$(sysctl -n hw.ncpu)and exports it into$GITHUB_ENV. Theexportline in this step both overrides that value and breaks the build. It should be removed.The same issue exists in
build_wheels_macos_aarch64at line 302–308..github/workflows/build-wheel.yml, line 210-215 (link)Invalid GitHub Actions expression syntax in
echoLine 213 uses
{{ env.MACOSX_DEPLOYMENT_TARGET }}(Jinja/Helm-style), but GitHub Actions expressions require the${{ ... }}syntax. As written, thisechowill literally printMACOSX_DEPLOYMENT_TARGET={{ env.MACOSX_DEPLOYMENT_TARGET }}instead of the actual value. While this is only a debug print and does not break the build, it produces misleading output.The same issue occurs at line 294 in the
build_wheels_macos_aarch64job..github/workflows/build-nightly-wheel.yml, line 173-197 (link)Nightly macOS jobs not updated to match
build-wheel.ymlThe
build_wheels_macos_x86_64andbuild_wheels_macos_aarch64jobs in this file still targetmacos-13/macos-14and usenproc(unavailable on macOS) in their "Build wheels" step. The equivalent jobs inbuild-wheel.ymlwere updated tomacos-15-intel/macos-15andsysctl -n hw.ncpu. While these jobs are currently disabled (if: false), they will be broken when re-enabled.The macOS aarch64 job at line 254 also still sets
MACOSX_DEPLOYMENT_TARGET=10.15— incorrect for ARM64 (should be11.0), and both macOS jobs still have the unfixed{{ env.MACOSX_DEPLOYMENT_TARGET }}echo (missing the `Enable macos buildingGreptile Summary
This PR refactors the wheel-packaging CI to enable macOS builds by activating previously disabled
build_wheels_macos_x86_64andbuild_wheels_macos_aarch64jobs, upgrading their runner images frommacos-13/macos-14tomacos-15-intel/macos-15, and fixingnproc→sysctl -n hw.ncpufor macOS CPU detection. It also adds atags: 'v*'push trigger tobuild-wheel.yml, sprinkles arm -rf /__w/_tool/Pythoncleanup step across self-hosted runner jobs in multiple workflows, and fixes a Bash array syntax bug inscripts/install_deps.sh.Key concerns:
tags: 'v*'trigger is paired with the existingpathsfilter. GitHub evaluatespathsfor tag pushes against the tagged commit's diff — if a release tag points to a commit that only bumps a version file not in thepathslist (e.g.,NEUG_VERSION), the wheel workflow will silently not run and no release wheels will be published.if: true, which runs expensive GitHub-hosted macOS runners on every PR and every push tomain— not just on release tags or workflow_dispatch as the commented-out condition intended.MACOSX_DEPLOYMENT_TARGET=10.15is set in the ARM64 macOS job; macOS 10.15 predates Apple Silicon and the minimum valid target for ARM64 is11.0.build-nightly-wheel.ymlwere not updated and will be broken (wrong runner images,nprocon macOS, wrong deployment target) when they are re-enabled.Confidence Score: 2/5
paths+tagsinteraction that risks silently skipping release wheel builds, and theMACOSX_DEPLOYMENT_TARGET=10.15mismatch on ARM64.pathsfilter paired withtags: 'v*'can silently prevent wheel builds on version tags;MACOSX_DEPLOYMENT_TARGET=10.15is architecturally invalid for ARM64; andif: truewill incur unnecessary macOS runner costs on every PR. Thescripts/install_deps.shfix and self-hosted cleanup steps are correct improvements..github/workflows/build-wheel.ymlrequires the most attention — specifically thepaths/tagstrigger interaction and the ARM64 deployment target..github/workflows/build-nightly-wheel.ymlmacOS jobs also need to be brought in sync before they are re-enabled.Important Files Changed
if: false→if: true; introduces atags: 'v*'push trigger whosepathsfilter could silently suppress the workflow on release tag pushes;if: trueruns expensive macOS runners on every PR and push to main;MACOSX_DEPLOYMENT_TARGET=10.15is set for the ARM64 job where11.0is required.build-wheel.yml— they will be broken when enabled.rm -rf /__w/_tool/Pythoncleanup step beforeactions/setup-pythonin the two self-hosted Linux jobs; both jobs useruns-on: [self-hosted, linux, ...]so the step is correctly targeted.runs-on: [ubuntu-22.04](single-element array) →runs-on: ubuntu-22.04(scalar); no functional impact.rm -rf /__w/_tool/Pythoncleanup step to thebuild_and_testjob, which runs on a[self-hosted]runner; step is correctly scoped to self-hosted infra.("xsimd", "cmake")(which produced a stray trailing comma on the first element, causingbrew install xsimd,to fail) to("xsimd" "cmake").Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Push / PR / workflow_dispatch] --> B{Event type?} B -->|push to main + paths match| C[Linux x86_64\nself-hosted] B -->|push to main + paths match| D[Linux ARM64\nself-hosted] B -->|push v* tag + paths match| C B -->|push v* tag + paths match| D B -->|if: true — all events| E[macOS x86_64\nmacos-15-intel] B -->|if: true — all events| F[macOS ARM64\nmacos-15] B -->|pull_request + paths match| C B -->|pull_request + paths match| D C --> G[Clean /__w/_tool/Python] D --> H[Clean /__w/_tool/Python] G --> I[Setup Python 3.13] H --> J[Setup Python 3.13\nif runner.name != arm64-1] E --> K[Install deps\nscripts/install_deps.sh] F --> L[Install deps\nscripts/install_deps.sh] I --> M[cibuildwheel] J --> M K --> M L --> M M --> N[Upload .whl artifact] N --> O{Publish condition} O -->|main push OR v* tag| P[PyPI publish] O -->|otherwise| Q[skip] E --> R[test_wheels_macos_x86_64\nmacos-15-intel] F --> S[test_wheels_macos_aarch64\nmacos-15] N --> R N --> Sprefix).
Consider applying the same runner image, CPU-count, deployment-target, and echo fixes here before re-enabling these jobs.
Last reviewed commit: 96e8ffa