-
Notifications
You must be signed in to change notification settings - Fork 261
chore(infra): remove build-macos-x86_64 #1325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@milenkovicm please take a look |
milenkovicm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't think we should care about Intel Macs at this stage, latest makes more sense to me
.github/workflows/build.yml
Outdated
| needs: [generate-license] | ||
| name: Mac x86_64 | ||
| runs-on: macos-latest | ||
| runs-on: macos-15-intel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, why would we test it on intel chipset, arm (latest) makes more sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making this change because this was originally using macos-13
macos-15-intel is the direct replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of the step "Mac x86_64" suggest that we should use an intel based processor.
macos-latest is arm64
https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened this pr to correct the mistake from #1324
we can follow up with removing this github action step is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe intel architecture for Mac is on its way out, let's keep arm, and just rename action, Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i remove the build-macos-x86_64 step entirely, the step above, build-python-mac-win, already covers mac-latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename
build-macos-x86_64:
needs: [generate-license]
name: Mac x86_64
runs-on: macos-latest
build-macos-x86_64 -> build-macos-arm64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so instead of removing build-macos-x86_64, we want to just rename to build-macos-arm64?
|
@milenkovicm i updated the pr to split out windows and mac. lmk if this is what you were thinking about |
| path: python/target/wheels/* | ||
|
|
||
| build-macos-x86_64: | ||
| build-macos-arm64: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what i was asking, as name changed after you push update to use arm64 👍🏻
| path: LICENSE.txt | ||
|
|
||
| build-python-mac-win: | ||
| build-windows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you removed mac from here? is there other job which tests this python on mac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, build-macos-arm64.
build-python-mac-win used to run for windows and macos. but since build-macos-arm64 already runs for macos, im chaning this to just run windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build-python-mac-win builds ballista/python/ extension, on the other hand build-macos-arm64 builds core ballista/ or I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heres a side by side diff of build-python-mac-win and build-macos-arm64 taken from latest main
datafusion-ballista/.github/workflows/build.yml
Lines 108 to 213 in 400f818
| build-python-mac-win: | |
| needs: [generate-license] | |
| name: Mac/Win | |
| runs-on: ${{ matrix.os }} | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| python-version: ["3.10"] | |
| os: [macos-latest, windows-latest] | |
| steps: | |
| - uses: actions/checkout@v5 | |
| - uses: actions/setup-python@v5 | |
| with: | |
| python-version: ${{ matrix.python-version }} | |
| - uses: dtolnay/rust-toolchain@stable | |
| - run: rm LICENSE.txt | |
| - name: Download LICENSE.txt | |
| uses: actions/download-artifact@v4 | |
| with: | |
| name: python-wheel-license | |
| path: . | |
| - name: Install Protoc | |
| uses: arduino/setup-protoc@v3 | |
| with: | |
| version: "27.4" | |
| repo-token: ${{ secrets.GITHUB_TOKEN }} | |
| - uses: astral-sh/setup-uv@v6 | |
| with: | |
| enable-cache: true | |
| - name: Build Python package | |
| run: | | |
| cd python | |
| uv sync --dev --no-install-package ballista | |
| uv run --no-project maturin build --release --strip | |
| - name: List Windows wheels | |
| if: matrix.os == 'windows-latest' | |
| run: dir python\target\wheels\ | |
| # since the runner is dynamic shellcheck (from actionlint) can't infer this is powershell | |
| # so we specify it explicitly | |
| shell: powershell | |
| - name: List Mac wheels | |
| if: matrix.os != 'windows-latest' | |
| run: find python/target/wheels/ | |
| - name: Archive wheels | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: dist-${{ matrix.os }} | |
| path: python/target/wheels/* | |
| build-macos-x86_64: | |
| needs: [generate-license] | |
| name: Mac x86_64 | |
| runs-on: macos-latest | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| python-version: ["3.10"] | |
| steps: | |
| - uses: actions/checkout@v5 | |
| - uses: actions/setup-python@v5 | |
| with: | |
| python-version: ${{ matrix.python-version }} | |
| - uses: dtolnay/rust-toolchain@stable | |
| - run: rm LICENSE.txt | |
| - name: Download LICENSE.txt | |
| uses: actions/download-artifact@v4 | |
| with: | |
| name: python-wheel-license | |
| path: . | |
| - name: Install Protoc | |
| uses: arduino/setup-protoc@v3 | |
| with: | |
| version: "27.4" | |
| repo-token: ${{ secrets.GITHUB_TOKEN }} | |
| - uses: astral-sh/setup-uv@v6 | |
| with: | |
| enable-cache: true | |
| - name: Build Python package | |
| run: | | |
| cd python | |
| uv sync --dev --no-install-package ballista | |
| uv run --no-project maturin build --release --strip | |
| - name: List Mac wheels | |
| run: find python/target/wheels/ | |
| - name: Archive wheels | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: dist-macos-aarch64 | |
| path: python/target/wheels/* |
the only difference is which os is running. the refactor here is to get rid of the matrix.os in build-python-mac-win since build-macos-x86_64 is already running for macos-latest
Which issue does this PR close?
Closes #.
Rationale for this change
Follow up to #1324, the correct replacement formacos-13should bemacos-15-intelfor Intel-based architectureaccording tohttps://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories
Since both
build-python-mac-winandbuild-macos-x86_64usemacos-latest, we can consolidate and refactor the steps to run for individual osWhat changes are included in this PR?
Are there any user-facing changes?