-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-41435: [CI][MATLAB] Add job to build and test MATLAB Interface on macos-14
#41592
Merged
Conversation
This file contains 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
raulcd
approved these changes
May 8, 2024
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.
Thanks @sgilmore10 !
github-actions
bot
added
awaiting merge
Awaiting merge
and removed
awaiting review
Awaiting review
labels
May 8, 2024
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 3046501. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
This was referenced May 15, 2024
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
May 25, 2024
…ce on `macos-14` (apache#41592) ### Rationale for this change Currently, the MATLAB interface is built and tested on `macos-12` - not `macos-14` - because the version of `mathworks/libmexclass` depends on used to not support `macos-14`. However, now that apache#41400 is closed, the version of `mathworks/libmexclass` the MATLAB interface depends on works on `macos-14`, so we will be able to build and test the MATLAB interface on `macos-14`. **Note**: When adding support for ARM-based macOS builds, we discovered an issue with the way in which we package the MLTBX files for the MATLAB Interface to Arrow. Currently, we bundle all shared libraries for all platforms (.dll, .dylib, and .so) into one large "monolithic" MLTBX file. Unfortunately, putting all platform-specific files into one MLTBX file poses an issue when we support multiple ISAs (e.g. x86 and ARM) because builds for the same operating system with different ISAs will have the same shared library file names. In other words, we will have a library named libarrowproxy.dylib for both ARM and x86 macOS builds. Therefore, we are going to hold off on adding ARM-based macOS builds to the crossbow packaging workflow for now until we have a chance to properly explore alternative packaging approaches. For example, we may want to consider having platform-specific MLTBX files. However, we still think it is worthwhile to add CI support for `macos-14` in the meantime. ### What changes are included in this PR? 1. Added workflow to build and test the MATLAB interface on `macos-14` as well as `macos-12`. ### Are these changes tested? N/A. ### Are there any user-facing changes? No. ### Future Directions 1. Add crossbow packaging workflow on `macos-14` once we determine how to package the interface for both ARM-based and Intel-based mac ISAs. * GitHub Issue: apache#41435 Authored-by: Sarah Gilmore <[email protected]> Signed-off-by: Sarah Gilmore <[email protected]>
sgilmore10
added a commit
that referenced
this pull request
May 29, 2024
…rface on macos-14 (#41677) ### Rationale for this change In #41592, we added a CI job to build and test the MATLAB interface on `macos-14`, which is ARM-based. We currently only package the interface on `macos-12`, which is Intel-based. We should add a crossbow job to package the MATLAB interface on `macos-14` so that we package the interface for both Intel-based and ARM-based macoS. ### What changes are included in this PR? 1. Parameterized the `macos` job in the MATLAB crossbow workflow file to run on both `macos-12` and `macos-14`. 2. Added the bash script `dev/tasks/matlab/rename_macos_dynamic_libraries.sh`. This script is used to uniquify the shared library names generated for the Intel/AMD-based macOS and ARM-based macOS interface installations. This is required because the crossbow job generates one "monolithic" MLTBX file that contains all shared libraries for all platforms. See the [comment](https://github.com/apache/arrow/pull/41677/files#diff-99731016fc39dd96d2d239cf9cc132a8dacb9569c49bea4a64d69911b4dcc8c4R82) at the beginning of `dev/tasks/matlab/rename_macos_dynamic_libraries.sh` for a more in-depth explanation. ### Are these changes tested? 1. Installed the MATLAB Arrow Interface on both Intel-based and ARM-based macOS machines using the MLTBX file generated by [this crossbow job](https://github.com/ursacomputing/crossbow/actions/runs/9274264286/job/25516244719). All unit tests passed. ### Are there any user-facing changes? Users on ARM-based macOS machines will be able to install the MATLAB Arrow Interface via a "one-click" install workflow using the MLTBX file. * GitHub Issue: #41675 Lead-authored-by: Sarah Gilmore <[email protected]> Co-authored-by: Sarah Gilmore <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sarah Gilmore <[email protected]>
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.
Rationale for this change
Currently, the MATLAB interface is built and tested on
macos-12
- notmacos-14
- because the version ofmathworks/libmexclass
depends on used to not supportmacos-14
. However, now that #41400 is closed, the version ofmathworks/libmexclass
the MATLAB interface depends on works onmacos-14
, so we will be able to build and test the MATLAB interface onmacos-14
.Note: When adding support for ARM-based macOS builds, we discovered an issue with the way in which we package the MLTBX files for the MATLAB Interface to Arrow.
Currently, we bundle all shared libraries for all platforms (.dll, .dylib, and .so) into one large "monolithic" MLTBX file.
Unfortunately, putting all platform-specific files into one MLTBX file poses an issue when we support multiple ISAs (e.g. x86 and ARM) because builds for the same operating system with different ISAs will have the same shared library file names. In other words, we will have a library named libarrowproxy.dylib for both ARM and x86 macOS builds.
Therefore, we are going to hold off on adding ARM-based macOS builds to the crossbow packaging workflow for now until we have a chance to properly explore alternative packaging approaches. For example, we may want to consider having platform-specific MLTBX files. However, we still think it is worthwhile to add CI support for
macos-14
in the meantime.What changes are included in this PR?
macos-14
as well asmacos-12
.Are these changes tested?
N/A.
Are there any user-facing changes?
No.
Future Directions
macos-14
once we determine how to package the interface for both ARM-based and Intel-based mac ISAs.macos-14
#41435