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

Use JNI implementation of ipcsocket APIs on Arm Macs #6165

Merged
merged 1 commit into from
Nov 23, 2020
Merged

Use JNI implementation of ipcsocket APIs on Arm Macs #6165

merged 1 commit into from
Nov 23, 2020

Conversation

eatkins
Copy link
Contributor

@eatkins eatkins commented Nov 23, 2020

There isn't yet a version of the jna available that works with the new
apple silicon using arm64. To workaround this, we can use the jni
implementation by default. If the user wants to force the jna, which
seems unlikely unless there is an unknown bug in the jni implementation,
they can opt in with the sbt.ipcsocket.jna system property and/or by
setting the serverUseJna setting.

@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

See sbt/ipcsocket#11. I think that it may not be necessary to bump ipcsocket to make sbt work on apple silicon because there is a translation layer in osx: https://www.theverge.com/21304182/apple-arm-mac-rosetta-2-emulation-app-converter-explainer. Nevertheless, it would be nice to run code written for the apple silicon.

@catap
Copy link

catap commented Nov 23, 2020

@eatkins maybe switch JNA off for not supported platform by default?

@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

@eatkins maybe switch JNA off for not supported platform by default?

I did the opposite and only turn JNI on if it is known to be supported. That's easier to implement because we know there is a small set of supported JNI platforms.

There isn't yet a version of the jna available that works with the new
apple silicon using arm64. To workaround this, we can use the jni
implementation by default on arm64 macs. If the user wants to force the
jni implementation for any supported platform, they can opt in with the
`sbt.ipcsocket.jni` system property and/or by setting the serverUseJni
setting.
@@ -337,18 +336,24 @@ public void close() {
static ServerSocket newSocket(final String sock) throws ServerAlreadyBootingException {
ServerSocket socket = null;
String name = socketName(sock);
boolean jni = requiresJNI() || System.getProperty("sbt.ipcsocket.jni", "false").equals("true");
Copy link

Choose a reason for hiding this comment

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

I feel that this requiresJNI should be implemented inside ipcsocket.

sbt is client for ipcsocket and ipcsocket may include or may not include some library.

Copy link
Contributor Author

@eatkins eatkins Nov 23, 2020

Choose a reason for hiding this comment

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

Maybe, but I don't think it's necessary. The ipcsocket library is agnostic on the implementation and provides constructors that take a useJNI flag so downstream consumers can freely choose their implementation and copy this logic if they feel they need it.

Copy link

Choose a reason for hiding this comment

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

@eatkins right now it looks a bit strange. Library A doesn't support feature because it has limitation and all this limitation is implementing on client of this library.

From my point of view this check should be implemented inside, and API should have "force JNI" that end user can control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. You may be right but I'm not sure what the best approach is and having requiresJNI in sbt was easier to implement and has no downstream effect. All downstream consumers are likely using the jna implementation because it was a very recent change to support jni at all. You're welcome to open a pr in sbt ipcsocket but I'm not motivated to change this code or add it there.

@eed3si9n eed3si9n changed the title Use jni implementation of ipcsocket apis by default Use JNI implementation of ipcsocket APIs on Arm Macs Nov 23, 2020
@eed3si9n eed3si9n merged commit 3a7ab8c into sbt:develop Nov 23, 2020
@eatkins eatkins deleted the ipcsocket-jna branch November 23, 2020 21:14
@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

@catap since this was merged, there should be a new nightly published tonight. It would be great if you could try it and report back. The nightlies are found here: https://dl.bintray.com/sbt/maven-snapshots/org/scala-sbt/sbt/

@catap
Copy link

catap commented Nov 23, 2020

@eatkins can I just run jav -jar ... on some jar from this repository?

@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

You should be able to do sbt -Dsbt.version=1.5.0-bin-20201123T081238 but with the new version that is published tonight instead of last night's version. It is pretty slow to resolve all the necessary artifacts for nightlies the first time so it may take a while for sbt to start up.

@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

@eatkins can I just run jav -jar ... on some jar from this repository?

I think I misread this. Building sbt requires sbt so I think waiting for the nightly is the only option. You could run
java -Dsbt.version=1.5.0-bin-20201123T081238 -jar /opt/local/share/sbt/sbt-launch.jar rather than sbt -Dsbt.version... if you don't have the sbt script on your path.

@catap
Copy link

catap commented Nov 23, 2020

@eatkins thanks, I'll test it as soon as it builded a new version. Am I right that it is 1.5?

@eatkins
Copy link
Contributor Author

eatkins commented Nov 23, 2020

Yeah, it should be the last version in the list at this url: https://dl.bintray.com/sbt/maven-snapshots/org/scala-sbt/sbt/

@catap
Copy link

catap commented Nov 24, 2020

@eatkins let me build it as local snapshot and test it

@catap
Copy link

catap commented Nov 24, 2020

@eatkins this one is workig fine, but I enjoy the next one here: #6162 (comment)

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

Successfully merging this pull request may close these issues.

3 participants