Use Java 22's foreign function API for Windows #61
Conversation
soc
commented
Aug 1, 2024
soc
commented
- Drop all existing mechanisms for retrieving this info on Windows
- This increases the required Java version of the library to 22
Isn't this a deal breaker? I would imagine many people would like to run this library on Java 21 or Java 17 or even Java 11. |
|
@sideeffffect I totally agree that this may be a problem for many people at the moment. The hope is that Java 22 will become an acceptable baseline in a few years and people will be able to adopt this version of the library. Until then, they can stay on an older version. |
soc
force-pushed
the
topic/windows-ffi
branch
3 times, most recently
from
4333b43 to
b6332c3
Compare
But what about other fixes/improvements which are not related to Windows? For example, Coursier (and/or scala-cli) needs to use a fork of directories-jvm. My hope was the by upstreaming the changes from the fork, the project could eventually come back to the upstream of directories-jvm. @soc would you be open to a PR that sets up sbt-multi-release-jar? |
|
@sideeffffect I think we could reserve some versions (e. g. v27 - v29) for fixes to the old codebase, and release the one with working Windows support as v30, would that help?
How is the the JVM delivered these days? The JVM version needed for the build tool shouldn't have any impact on the JVM version used for building users' code, right? |
|
I think e.g. Coursier (a project using directories-jvm) needs to be able to run on whatever JVM the user has installed (>= 8), AFAIK.
This would still lock (at least some) downstream dependencies on these "outdated" versions... What downsides do you see in using |
Yikes, they are still doing that? Ok.
That would still require an implementation that didn't require Java 22 for Windows, and I'm not seeing a good option there. |
It doesn't have to be perfect. Better half-broken than none at all. Very sad, but pragmatic approach IMHO. |
I think just staying on v2x of the library should be the thing to do in this case. |
|
FWIW I have a multi-release jar implementation of approximately the same idea here: https://github.com/maths22/directories-jvm/tree/ffm-poc (though with implementations for the preview versions of all the intermediate java versions as well that had a usable preview) |
|
Some thoughts:
|
Eventually that will need a solution, but that means for java 22+ you don't actually need any flags at the moment, and java itself logs the necessary warning (so we wouldn't need to do the equivalent ourselves). Java 17 needs both the |
| throw new AssertionError("failed converting string " + folderId + " to KnownFolderId"); | ||
| } | ||
| MemorySegment path = arena.allocate(C_POINTER); | ||
| SHGetKnownFolderPath(guidSegment, 0, MemorySegment.NULL, path); |
There was a problem hiding this comment.
Per windows API docs,
The calling process is responsible for freeing this resource once it is no longer needed by calling CoTaskMemFree, whether SHGetKnownFolderPath succeeds or not.
Therefore, as currently implemented there would appear to be a minor memory leak.
(see https://learn.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetknownfolderpath )
There was a problem hiding this comment.
We can fix this:
Call CoTaskMemFree(path); after calling SHGetKnownFolderPath.
Add the following:
private static class CoTaskMemFree {
public static final FunctionDescriptor DESC = FunctionDescriptor.ofVoid(C_POINTER);
public static final MethodHandle HANDLE = Linker.nativeLinker().downcallHandle(
findOrThrow("CoTaskMemFree"), DESC);
}
/**
* {@snippet lang=c :
* extern void CoTaskMemFree(LPVOID pv)
* }
*/
public static void CoTaskMemFree(MemorySegment pv) {
var handle = CoTaskMemFree.HANDLE;
try {
handle.invokeExact(pv);
} catch (Throwable throwable) {
throw new AssertionError("should not reach here", throwable);
}
}System.loadLibrary("combase"); may be necessary.
There was a problem hiding this comment.
@brcolow Made the changes!
(I'll let the CI run without System.loadLibrary("combase") first, will add it too if it complains.)
I really wonder what's the "real" solution here, I can't imagine everyone just silencing the warning is the goal the Java devs had in mind... |
JNA seems to work with graalvm nowadays:
but my preferred approach would be to have 2 options in the library, and the dir-dev library knows this by its own: first, and preferred, JNA. and if it is compiled by graalvm or so, FFI. i created a [https://github.com/microsoft/openjdk/issues/628 ticket here with microsofts openjdk], as i failed making a pull request [https://github.com/MovingBlocks/Terasology/pull/5284 using this library in terasology, because of the windows support]. @BenjaminAmos thinks terasology's approach to use JNA is more rubust, and i share that opinion. |
|
Status:
|
soc
force-pushed
the
topic/windows-ffi
branch
2 times, most recently
from
d25f61f to
91a3c6f
Compare
soc
force-pushed
the
topic/windows-ffi
branch
from
91a3c6f to
8285814
Compare
soc
force-pushed
the
topic/windows-ffi
branch
from
8285814 to
2009d9e
Compare
Not sure what this is about ... |
soc
force-pushed
the
topic/windows-ffi
branch
from
2009d9e to
fcf0332
Compare
This comment was marked as outdated.
This comment was marked as outdated.
that looks like the jvm crashed. not too sure how to display it, but when you not let it fork, it might be easier to spot: see #64. |
Great tip. Looks like |
|
@karianna can you do something about |
- Drop all existing mechanisms for retrieving this info on Windows - This increases the required Java version of the library to 22 Fixes #49.
soc
force-pushed
the
topic/windows-ffi
branch
from
fcf0332 to
50ce28c
Compare
|
I know this is already merged, but this seems like the best place to comment. Java 22 is a blocker for me on using the library. Is there any possibility that the java 22 components could make like a "driver" and is an implementation that is used/preffered under the hood? Like we have log4j api but via adapters could have a number of implementations. There could also be a dumber (less correct) windows implementation that returns results? My use case is stuck on 11 bytecode (I want to use it for a gradle plugin, and gradle is on v8 but I think it needs to move forward and that 11 is acceptable). I do not understand the limitations that this would create. oh, and when I say dumb, I mean really dumb, like just guess/make something up, only windows that are not EOL 10/11 and maybe not flexible the equivalent of not looking at XDG env var but just sticking things in the home directories as otherwise define. |
|
@xenoterracide If an older version works for you, stay on that. |
|
I wouldn't assume to know 🤷🏻♂️ and there's no guarantee an older version is going to get a maintenance release if there was a security vulnerability as the releases don't look like you're maintaining multiple versions. Good luck then! |
that sentence has a lot of assumptions. the lib is not a springboot maven project which downloads the internet to build. it is a couple of java classes. might be better to first have a security vulnerability before addressing it @xenoterracide :) |
