Skip to content
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

build(binding): MacOS ARM64 native binaries #223

Merged
merged 2 commits into from
Aug 4, 2024

Conversation

rexfleischer
Copy link
Contributor

@rexfleischer rexfleischer commented Feb 19, 2024

Description

Change to add the native binaries for Mac OS ARM64. This is based on #112 and #190. I am not able to test all the different binaries, but I was able to test the non-freetype macos-arm64.

Type of change

  • Minor changes or tweaks (quality of life stuff)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@Displee
Copy link

Displee commented May 6, 2024

Yes please! I need this so bad!

@zly2006
Copy link

zly2006 commented May 11, 2024

kindly reminds that you could use * [X] instead of * [ X ] (remove whitespaces) so it looks like ☑️ in your description

Copy link

@zly2006 zly2006 left a comment

Choose a reason for hiding this comment

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

Also have you checked the imgui-app-all jar? There is some issues in the build script:

  • arm64 nativa lib is not found in /bin folder
  • the lib is also not found in :imgui-app:shadowJar task (reason might be above)

@rexfleischer
Copy link
Contributor Author

I haven't checked the all one because I didn't know there was one! I will do that as well. Thanks for the reviews.

@rexfleischer
Copy link
Contributor Author

Update will be in soon.

Ok, this was a couple months ago so I'm not sure if I'm remembering correctly. Here are some notes (and maybe a follow up question):

  • I originally assumed that I didn't need to include the built natives in bin. I forgot why I thought that... However, I'm copying them into bin now. Is there a standard way of doing this that I'm missing? I just got the ./gradlew imgui-binding:generateLibs -Denvs=macosarm64 -Dfreetype=true/false working and copied /tmp/imgui/libsNative/macosxarm64/libimgui-javaarm64.dylib to bin.
  • I couldn't get the freetype build working until I executed the FreeType - Install (MacOS-arm64) from .github/workflows/ci.yaml step. This sound reasonable?
  • After executing ./gradlew :imgui-app:shadowJar, I executed jar tf imgui-app/build/libs/imgui-app-686c27-all.jar and now see the libimgui-javaarm64.dylib library.

I do have one follow up question. I see in the image-app build script for the jar, it copies in the non-freetype linux and mac natives, but only the freetype windows native. What is the reason for this?

@zly2006
Copy link

zly2006 commented May 16, 2024

However, I'm copying them into bin now. Is there a standard way of doing this that I'm missing?

There is a github ci script for that: .github/workflows/ci.yaml, but was disabled for PRs, please commit native libs by hand

  • I couldn't get the freetype build working until I executed the FreeType - Install (MacOS-arm64) from .github/workflows/ci.yaml step. This sound reasonable?

There's an wrong if condition, I fixed this issue in #190, merge the upstream to fix

  • After executing ./gradlew :imgui-app:shadowJar, I executed jar tf imgui-app/build/libs/imgui-app-686c27-all.jar and now see the libimgui-javaarm64.dylib library.

I am not quite sure... I have not looked carefully into your code.

I do have one follow up question. I see in the image-app build script for the jar, it copies in the non-freetype linux and mac natives, but only the freetype windows native. What is the reason for this?

see readme: https://github.com/SpaiR/imgui-java?tab=readme-ov-file#freetype
freetype was better in rendering, and there is no dependency issues on windows (unlike linux and macos you should install freetype first)

After all, your PR still cannot be compiled and seems far away from being merged, I am neither the maintainer of this project nor an expert in imgui, I am not asking you to close the pr but it really has some problems and I think #190 works better.

@rexfleischer rexfleischer force-pushed the macosarm64 branch 2 times, most recently from 588aa1a to a9f822c Compare May 16, 2024 19:49
@rexfleischer
Copy link
Contributor Author

I opened this because both #112 and #190 seemed to be abandoned and had either changes requested by the maintainer or had other issues. If you can get 190 in, I don't care either way. However, 190 still has changes requested by the maintainer in 112.

your PR still cannot be compiled

how do you figure this? It compiles: https://github.com/rexfleischer/imgui-java/actions/runs/9118399802

seems far away from being merged

If you "neither the maintainer of this project nor an expert in imgui", how do you figure this? I'm sorry if this PR caused some turmoil, but both the other PRs seemed far away from being merged.

@rexfleischer
Copy link
Contributor Author

Either way, @SpaiR if there is anything that can help move this PR or #190 forward, please let us know.

@phraktle phraktle mentioned this pull request Jul 4, 2024
5 tasks
@SpaiR SpaiR added the feat New feature or request label Aug 4, 2024
@SpaiR
Copy link
Owner

SpaiR commented Aug 4, 2024

First of all, I apologize for the inconvenience to everyone who was waiting for the macOS ARM support and specifically for this PR to be merged. 🙏

There were several PRs, but for various reasons, I wasn't able to review them until now. However, the time has finally come!

I will merge this PR since it includes some minor CI cleanups. I also want to thank everyone who has contributed (#112 and #190). I will mention all authors in the upcoming changelog, which I believe will be released very soon (within 1-2 weeks or even sooner).

@SpaiR SpaiR merged commit be47340 into SpaiR:main Aug 4, 2024
11 checks passed
@SpaiR
Copy link
Owner

SpaiR commented Aug 6, 2024

Ok, I revamped the whole concept. In PR #239, a universal binary for the ARM64 architecture was introduced. So, one dylib for all - which is great, I believe. Additionally, while working on the build scripts, I decided to resolve the longstanding issue with FreeType. As a result, FreeType is now statically compiled and enabled by default, which basically improves font quality for free. More details can be found in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants