sbt-jni icon indicating copy to clipboard operation
sbt-jni copied to clipboard

Add support for Windows

Open khanetor opened this issue 6 years ago • 15 comments

The command uname -sm only works for Unix-based OSes. Currently I have to manually set

nativePlatform := "win32-x86_64"

khanetor avatar Jul 18 '17 08:07 khanetor

Windows is currently not officially supported, however it shouldn't be too complicated to add. Here is the rough outline:

  • find a way to distinguish the windows architectures (I'm not sure if there are mutliple), similar to what uname can provide on unix
  • use that mechanism in JniNative and JniLoad to bundle and extract the corresponding native libraries

jodersky avatar Jul 18 '17 08:07 jodersky

What about System.getProperty("os.arch") and System.getProperty("os.name"). Could we change the code to use these instead of uname?

eaplatanios avatar Jul 18 '17 13:07 eaplatanios

That's a good idea and was also what was used in a very early version of this project. Unfortunately, the os.arch property is too vague. It is able to distinguish between certain architectures only, such as x86 and ARM for instance. The latter however has several, incompatible, architectures which are indistinguishable with that mechanism. For example armv6 (used in the original raspberry pi) and armv7l (used in subsequent models) are incompatible, yet they both show up as simply "arm" from os.arch.

jodersky avatar Jul 18 '17 18:07 jodersky

I see. How about we fall back to these two properties when uname is not available. Do you think that's acceptable behavior until we find a better solution?

eaplatanios avatar Jul 20 '17 12:07 eaplatanios

I think we could check the os.name property and in case it is windows handle it specifically. Otherwise we should use the standard uname method.

jodersky avatar Jul 25 '17 23:07 jodersky

My platform is neither Windows nor ARM, but I'm still inconvenienced by the need to spawn an extra process to run uname during JNI loading, since that's not allowed, causing my application to crash. My fix was also to use sys.props("os.name") and sys.props("os.arch") to avoid forking a process.

That's a good idea and was also what was used in a very early version of this project. Unfortunately, the os.arch property is too vague. It is able to distinguish between certain architectures only, such as x86 and ARM for instance. The latter however has several, incompatible, architectures which are indistinguishable with that mechanism. For example armv6 (used in the original raspberry pi) and armv7l (used in subsequent models) are incompatible, yet they both show up as simply "arm" from os.arch.

I may be jumping to conclusions too fast, but would it be correct to assume that sys.props("os.arch") == "arm' is the only problem case? Shouldn't this be handled via a trouble report back to Oracle to fix this in Java? That coarse-grain reporting of the underlying architecture is obviously(?) not very helpful and likely to cause pain for others as well...

I think we could check the os.name property and in case it is windows handle it specifically. Otherwise we should use the standard uname method.

I don't know, but IF dragging in uname is only needed on ARM, THEN -perhaps- special-casing Windows is not the optimal approach. In my use case, I'd much rather opt for only falling back to uname when sys.props("os.arch") == "arm'.

What do you think?

andraxin avatar Jan 11 '19 13:01 andraxin

I doubt that anything can be done about changing the property of os.arch upstream, as that could break user code. However, I think your proposal of using system properties by default and calling uname only on an arm platform is worth a shot!

jodersky avatar Jan 11 '19 16:01 jodersky

Great! Will you fix that or would you prefer a pull request?

andraxin avatar Jan 11 '19 23:01 andraxin

PR created.

andraxin avatar Apr 27 '19 17:04 andraxin

It's a bit unfortunate that the PR submitted by andraxin was rejected, because assuming the presence of a uname command is not necessarily justified even under Linux, such as when running in a minimal Docker container.

The only thing you can really rely on is that Java is installed. For edge-cases where os.arch is not enough, why not have the user set -Dos.arch=whatever or have some other way to override it. As far as I can tell neither JNA or JNR use this uname cludge.

fgoepel avatar Sep 15 '21 14:09 fgoepel

Hey @shado23 I think we can add an option to overrride the target arch via the java option passed, would you like to help with that?

My proposition would be to leave the current behavior as the default one, and to add an option to override it via the os.arch option passed, I see no reason why not to have it.

pomadchin avatar Sep 19 '21 21:09 pomadchin

@pomadchin and @jodersky proposing a alternate solution if it has not been considered already.

Regarding the platform identification,

  • currently we are relying on uname which doesn't work with windows and has to be manually overridden in the build.sbt as mentioned in the OP.
  • There were other solutions like relying on os.arch but you are correct, it is natively deciphered in java and can lead to non deterministic results.
  • My suggestion is to not do either. Actually we only need a value for nativePlatform to create directories so why not instead rely on the target triple directly from the 2 supported build tools instead of getting the value from uname and then parsing it.
  • For rust you can simply set the nativePlatform to rustc -vV | sed -n 's|host: ||p' and for cmake we can get it from various places like make -v, gcc -dumpmachine commands.
  • Basically the inference of the nativePlatform can be as per the build tool selected and not something that we should construct.

Some other smaller things I identified that block windows support (unrelated to where the nativePlatform comes from),

  • the path construction is a problem. String interpolation or appending has been done which works on *nix OS but not on windows.
  • This can be easily fixed with java.nio.....Paths but needs to be done in a few places across the repo.
  • Things like this need to be fixed, because for windows, .dll are generated.

chitralverma avatar Feb 02 '23 13:02 chitralverma

hey @chitralverma took some time to respond; I think it is a great idea and worth trying :+1:

pomadchin avatar Feb 23 '23 15:02 pomadchin

Ref hacky project on what this will look like, https://github.com/chitralverma/test-native-proj

chitralverma avatar Feb 23 '23 15:02 chitralverma

@chitralverma hmmm, I think it's worth trying making a PR against this repo just to start iterating on this idea.

pomadchin avatar Feb 24 '23 19:02 pomadchin