Skip to content

add native support for Apple Silicon macs #310

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 12, 2023
Merged

add native support for Apple Silicon macs #310

merged 2 commits into from
Oct 12, 2023

Conversation

DimitrisStaratzis
Copy link
Contributor

@DimitrisStaratzis DimitrisStaratzis commented Oct 3, 2023 •
edited
Loading

This PR adds native support for Apple Silicon macs using the TileDB-Java API. It also adds the ARM native libs in the release jar.

I have modified the CI to test against Apple Silicon. I have also added an additional step just before the "release" to test the functionality of the fatJar on all platforms.

A complete run can be found here: https://github.com./TileDB-Inc/TileDB-Java/actions/runs/6495589089. This run created a draft release. You can inspect the final jar here: https://github.com./TileDB-Inc/TileDB-Java/releases/tag/untagged-8e30ff0d9545eb7048bb

[sc-11462]

@DimitrisStaratzis DimitrisStaratzis marked this pull request as draft October 3, 2023 12:35
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 2 times, most recently from 2920ff7 to 118766a Compare October 3, 2023 12:42
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #11462: ARM support.

ihnorton
ihnorton reviewed Oct 3, 2023
View reviewed changes
cmake/Modules/FindTileDB_EP.cmake Outdated
@@ -51,8 +51,15 @@ if (NOT TILEDB_FOUND)
SET(DOWNLOAD_URL "https://github.com./TileDB-Inc/TileDB/releases/download/2.16.3/tiledb-windows-x86_64-2.16.3-194b5ae.zip")
SET(DOWNLOAD_SHA1 "8e986b6143ef9509201c7f2b3c0a7a53ff41095a")
elseif(APPLE) # macOS
SET(DOWNLOAD_URL "https://github.com./TileDB-Inc/TileDB/releases/download/2.16.3/tiledb-macos-x86_64-2.16.3-194b5ae.tar.gz")
SET(DOWNLOAD_SHA1 "b4d35306771d1c30b49bf2a98651e8e24c91037e")
if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work correctly if we are "cross-compiling" from x86_64, we need to also check CMAKE_OSX_ARCHITECTURES. See: TileDB-Inc/TileDB-Vector-Search@b3c70a9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ihnorton
Copy link
Member

ihnorton commented Oct 3, 2023

  1. I think we need to adjust the packaged lib locations?
  2. We should probably only run on the arm64 runners occasionally, because they are very expensive (maybe: only for tags, nightly tests, and release packaging?)

@DimitrisStaratzis
Copy link
Contributor Author

  1. I think we need to adjust the packaged lib locations?

    1. We should probably only run on the arm64 runners occasionally, because they are very expensive (maybe: only for tags, nightly tests, and release packaging?)

Yes as soon as I solve the compilation errors I will work on the lib locations/names.

We can certainly disable runs for PRs and master merges

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 10 times, most recently from 6a91483 to 365b4f7 Compare October 6, 2023 16:11
@mosabdullah
Copy link

Hey @DimitrisStaratzis,

Will this fix be applied to the 17.x version?

@DimitrisStaratzis
Copy link
Contributor Author

Hey @DimitrisStaratzis,

Will this fix be applied to the 17.x version?

Hi @mosabdullah ,
This will be applied to the upcoming 0.19.0. Is there a specific reason that you need it in 0.17.X?

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 7 times, most recently from 8515e43 to 322de16 Compare October 10, 2023 15:58
@DimitrisStaratzis
Copy link
Contributor Author

DimitrisStaratzis commented Oct 10, 2023 •
edited
Loading

Hey @DimitrisStaratzis,
Will this fix be applied to the 17.x version?

Hi @mosabdullah , This will be applied to the upcoming 0.19.0. Is there a specific reason that you need it in 0.17.X?

If you meant 2.17.X then yes. It will be released with TileDB-Java-0.19.0 which corresponds to TileDB-2.17.0

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 3 times, most recently from 63ea1cd to aebbc8d Compare October 10, 2023 17:08
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 4 times, most recently from c49dfd0 to 1b3e556 Compare October 12, 2023 10:07
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch 15 times, most recently from e7a704b to 935a573 Compare October 12, 2023 13:02
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/add_support_for_apple_silicon branch from 935a573 to fe5135c Compare October 12, 2023 13:24
@DimitrisStaratzis DimitrisStaratzis marked this pull request as ready for review October 12, 2023 13:30
ihnorton
ihnorton approved these changes Oct 12, 2023
View reviewed changes
src/main/java/io/tiledb/libtiledb/NativeLibLoader.java

if (System.getProperty("os.name").contains("mac")
&& System.getProperty("os.arch").equals("aarch64")) {
libDir = "arm" + libDir;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could use arm- for more consistency with other target-naming styles

@DimitrisStaratzis DimitrisStaratzis merged commit d8e4c85 into master Oct 12, 2023
@DimitrisStaratzis DimitrisStaratzis deleted the dstara/add_support_for_apple_silicon branch October 12, 2023 13:47
@mosabdullah
Copy link

mosabdullah commented Oct 12, 2023 •
edited
Loading

Hey @DimitrisStaratzis,
Will this fix be applied to the 17.x version?

Hi @mosabdullah , This will be applied to the upcoming 0.19.0. Is there a specific reason that you need it in 0.17.X?

If you meant 2.17.X then yes. It will be released with TileDB-Java-0.19.0, which corresponds to TileDB-2.17.0

No, I mean the tiledb-java 0.17.0 version, as that is the one we are using. I can check if it is feasible to upgrade to the 0.19 version.

If we moved to the 0.19 version, will it be backwards compatible with arrays created with 2.15.X versions?

@DimitrisStaratzis DimitrisStaratzis mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihnorton ihnorton ihnorton approved these changes

@Shelnutt2 Shelnutt2 Awaiting requested review from Shelnutt2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DimitrisStaratzis @ihnorton @mosabdullah