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

[GH-1313] Move default darwin prefix to darwin-$arch and support darwin as fallback #1316

Conversation

matthiasblaesing
Copy link
Member

The darwin platform traditionally used fatbinaries to support multiple
architectures. This works, but is a problem:

  1. when apple decides, that only certain architectures may be bundled (it was reported, that the presence of x86 caused appstore validation to fail)
  2. when upstream does not bundle the artifacts, but builds them in isolation.

Given, that JNA also hit (b) and needed post processing to merge
binaries, moving darwin to the same scheme as other OSes (<os>-<arch>)
is considered sensible.

While the resource prefix is changed, the old "darwin" prefix is still
considered for extraction and thus old libraries will continue to work.

@matthiasblaesing
Copy link
Member Author

@tresf @fkistner @dbwiddis it would be good if you could have a look at this/test this.

@tresf
Copy link
Contributor

tresf commented Feb 17, 2021

@matthiasblaesing I have not yet done a full review, but a few early observations/questions:

  • Why is this considered a breaking change? If the .dylib is bundled with JNA, what component would break in user space? I would expect no breaking changes.
  • What is the purpose of the backwards compatibility if the .dylib is bundled with JNA? Wouldn't this only affect newer versions, thus versions with the newer convention?
  • After this change, does the project still work for building x86 or ppc binaries? It does now. Although this use-case is obscure and support has been officially dropped, I'm not aware of any reason to specifically kill it for those who choose to. Do you agree? If so, we may want to add those in too, even if they're unused 99% of the time.

@matthiasblaesing
Copy link
Member Author

matthiasblaesing commented Feb 17, 2021

Why is this considered a breaking change? If the .dylib is bundled with JNA, what component would break in user space? I would expect no breaking changes.

The breaking part is, that the value of RESOURCE_PREFIX for darwin changes. If external code relies on the content, it will break. For JNA itself it is not breaking, so maybe "potentially breaking" would be a better term.

What is the purpose of the backwards compatibility if the .dylib is bundled with JNA? Wouldn't this only affect newer versions, thus versions with the newer convention?

The backwards compatibility for JNA itself is not that important, but for other libraries. If you supply your native library bundled with the jar, JNA can extract it for you. Before this change the default path was darwin/dummy.dylib now it is darwin-x86-$arch/dummy.dylib, but the old path is check in addition.

After this change, does the project still work for building x86 or ppc binaries? It does now. Although this use-case is obscure and support has been officially dropped, I'm not aware of any reason to specifically kill it for those who choose to. Do you agree? If so, we may want to add those in too, even if they're unused 99% of the time.

Yes - the binaries will be placed in the corresponding darwin-$arch directory.

@tresf
Copy link
Contributor

tresf commented Feb 17, 2021

Yes - the binaries will be placed in the corresponding darwin-$arch directory.

Right, but I don't think we'll be preparing them for distribution unless they're included, right?

jna/build.xml

Lines 570 to 575 in a19f127

<zipfileset src="${lib.native}/darwin-x86-64.jar"
includes="*jnidispatch*"
prefix="com/sun/jna/darwin-x86-64"/>
<zipfileset src="${lib.native}/darwin-aarch64.jar"
includes="*jnidispatch*"
prefix="com/sun/jna/darwin-aarch64"/>

@matthiasblaesing
Copy link
Member Author

Yes - the binaries will be placed in the corresponding darwin-$arch directory.

Right, but I don't think we'll be preparing them for distribution unless they're included, right?

jna/build.xml

Lines 570 to 575 in a19f127

<zipfileset src="${lib.native}/darwin-x86-64.jar"
includes="*jnidispatch*"
prefix="com/sun/jna/darwin-x86-64"/>
<zipfileset src="${lib.native}/darwin-aarch64.jar"
includes="*jnidispatch*"
prefix="com/sun/jna/darwin-aarch64"/>

If someone comes up with a workable and testable way to build the ppc and/or x86 binaries, I'm willing to do the work, until then, it is useless work IMHO. I'm pretty sure, that won't happen.

@tresf
Copy link
Contributor

tresf commented Feb 17, 2021

If someone comes up with a workable and testable way to build the ppc and/or x86 binaries, I'm willing to do the work, until then, it is useless work IMHO. I'm pretty sure, that won't happen.

We built them as part of #1297, but they were intentionally left out due to Apple prohibiting them from notarized packages. Note, the notarizing and bundling issue changes as a result of this PR. Apps need to re-sign binaries with their own developer IDs, so extraction and re-jaring is mandatory. This means this PR could hypothetically re-introduce these binaries without significant inconvenience (e.g. lipo, etc) to the packagers as moving forward they'd have the ability to remove them from the final archive after signing.

it is useless work IMHO

I can't speak on behalf of the usefulness. I'm aware that PPC is alive and well in impoverished and niche hobbyist communities (not so much the x86 stuff). I only wanted to mention this, because the currently lipo technique on master does currently support these architectures for distribution, which will stop working once this is merged.

@matthiasblaesing matthiasblaesing force-pushed the darwin-normalization branch 2 times, most recently from e861e53 to 84ab3d2 Compare February 18, 2021 20:27
@matthiasblaesing
Copy link
Member Author

@tresf I pushed an update that should address your concerns, I reworded the "Breaking Changes" section and added definitions for other darwin version - could you please see if you manage to build the x86/ppc/ppc64 library and see if it ends up in the final jna.jar? Xcode and I don't like each other and currently I fail to build architectures that are not x86-64 or aarch64.

@matthiasblaesing
Copy link
Member Author

I managed to install XCode 9.4 on mac OS High Sierra and build JNA there and also rebuild the native library for x86 (I fixed the build, I still can't test it and don't have matching JNI headers, so bundled libraries stay as they are). I literally put hours into this and will not repeat it. I tried to build for PPC, but not even XCode 9.4 is old enough for that.

@matthiasblaesing
Copy link
Member Author

I created a small test programm using java-argon2 (as a sample that bundles a native library) and tested it with 5.8.0-SNAPSHOT on mac OS x86-64. It loaded correctly ss is (with the native library in the darwin prefix) and also with the native library moved to the new primary prefix darwin-x86-64.

phxql added a commit to phxql/argon2-jvm that referenced this pull request Feb 22, 2021
@phxql
Copy link

phxql commented Feb 23, 2021

Soooo, @cricketsamya did some test runs and this works! we have moved the aarch64 library to darwin-aarch64 and it is picked up. Thanks a lot!

…h and support darwin as fallback

The darwin platform traditionally used fatbinaries to support multiple
architectures. This works, but is a problem:

a) when apple decides, that only certain architectures may be
   bundled (it was reported, that the presence of x86 caused
   appstore validation to fail)

b) when upstream does not bundle the artifacts, but builds them
   in isolation.

Given, that JNA also hit (b) and needed post processing to merge
binaries, moving darwin to the same scheme as other OSes (<os>-<arch>)
is considered sensible.

While the resource prefix is changed, the old "darwin" prefix is still
considered for extraction and thus old libraries will continue to work.
@matthiasblaesing matthiasblaesing merged commit 95923c1 into java-native-access:master Feb 26, 2021
@matthiasblaesing matthiasblaesing deleted the darwin-normalization branch August 10, 2021 23:12
@tresf
Copy link
Contributor

tresf commented Sep 10, 2021

@tresf I pushed an update that should address your concerns, I reworded the "Breaking Changes" section and added definitions for other darwin version - could you please see if you manage to build the x86/ppc/ppc64 library and see if it ends up in the final jna.jar? Xcode and I don't like each other and currently I fail to build architectures that are not x86-64 or aarch64.

Sorry for the late reply here. I've had the ppc/ppc64 on my todo list and finally was able to kick the tires on it. In short, there's no way to support this configuration, but I'll go into specific detail as to why:

It could be argued that with the right Intel machine with PPC compiler support targeting PPC binaries is possible, but obtaining Java 8 is still hard on these old platforms.

For these reasons, supporting PPC/PPC64 (and likely Intel 32-bit) seems to be a difficult task, one requiring hacks to the jna codebase (or a fork) which re-introduces older JDK support just to fire a build.

For that reason, I think any attempt to ever support Darwin x86/PPC/PPC64 binaries is futile. The PPC Macs do have a small, somewhat active community, but for those needing JNA, a Java 5 target should be the standard for that audience (which in my opinion is too far out of reach), and fir the projects requiring it, they can always revert to a version which contained support for their architecture.

In regards to 32-bit Intel, a similar but different fate exists. This architecture was very short-lived (2006 - 2007) with only a 6-month "32-bit only" market.. Note, the 32-bit Java 6 from Apple will work, due to the lack of popularity, targeting this rare combination seems too like a wasted effort.

@matthiasblaesing
Copy link
Member Author

@tresf thank you for the followup. I agree on keeping PPC/PPC64 and x86 out of support.

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

Successfully merging this pull request may close these issues.

3 participants