Use Java 22's foreign function API for Windows #61

soc · 2024-08-01T21:47:04Z

Drop all existing mechanisms for retrieving this info on Windows
This increases the required Java version of the library to 22

src/main/java/dev/dirs/impl/Windows.java

soc · 2024-08-01T23:07:39Z

src/main/java/dev/dirs/impl/Windows.java

+		try (var arena = Arena.ofConfined()) {
+			MemorySegment guidSegment = arena.allocate(GUID_LAYOUT);
+			if (CLSIDFromString(createSegmentFromString(folderId, arena), guidSegment) != 0) {
+				System.exit(-1);


return null or throw exception instead?

Yeah - definitely don't want to exit. That was just for my little utility program.

sideeffffect · 2024-08-02T10:27:53Z

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.
Maybe there's a way to have our cake and eat it too? We could make Multi-Release JAR https://github.com/sbt/sbt-multi-release-jar with fallback for < 22.

soc · 2024-08-02T10:52:00Z

@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.
(The Windows code in these versions is so sufficiently broken, that I don't consider it an acceptable fallback for a new version that comes with proper support through FFI.)

sideeffffect · 2024-08-05T10:09:46Z

Until then, they can stay on an older version.

The Windows code in these versions is so sufficiently broken, that I don't consider it an acceptable fallback

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.
But merging this without sbt-multi-release-jar would shut the door on that, because I don't think the project can depend on such recent version of Java (>=22). /cc @alexarchambault

@soc would you be open to a PR that sets up sbt-multi-release-jar?

soc · 2024-08-05T11:37:44Z

@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?

I don't think the project can depend on such recent version of Java (>=22)

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?

soc · 2024-08-05T12:01:59Z

src/main/java/dev/dirs/impl/Windows.java

+	private static String getDir(String folderId) {
+		try (var arena = Arena.ofConfined()) {
+			MemorySegment guidSegment = arena.allocate(GUID_LAYOUT);
+			if (CLSIDFromString(createSegmentFromString(folderId, arena), guidSegment) != 0) {


I wonder if it made sense to compute this in Java using java.util.UUID.fromString(x).

I'm not sure though how we could convert that UUID into the struct SHGetKnownFolderPath needs though ...

You can see what doing the conversion in java looks like here. https://github.com/maths22/directories-jvm/blob/ffm-poc/src/main/java22/com/maths22/directories/WindowsNativeUtil.java#L54

I'm not going to claim this is the best code for this-it could definitely be cleaned up substantially for readability, but it does function correctly.

sideeffffect · 2024-08-05T12:05:27Z

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.

reserve some versions

This would still lock (at least some) downstream dependencies on these "outdated" versions...

What downsides do you see in using sbt-multi-release-jar?

soc · 2024-08-05T12:13:39Z

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.

Yikes, they are still doing that? Ok.

This would still lock (at least some) downstream dependencies on these "outdated" versions...
What downsides do you see in using sbt-multi-release-jar?

That would still require an implementation that didn't require Java 22 for Windows, and I'm not seeing a good option there.

sideeffffect · 2024-08-05T13:43:45Z

implementation that didn't require Java 22 for Windows

It doesn't have to be perfect. Better half-broken than none at all. Very sad, but pragmatic approach IMHO.

soc · 2024-08-05T13:52:07Z

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.

maths22 · 2024-09-03T16:46:57Z

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)

sideeffffect · 2024-09-04T09:45:51Z

@maths22 would be lovely to incorporate this to upstream. What do you think @soc ?

soc · 2024-09-04T15:45:34Z

Some thoughts:

The commits look like a ton of work, code and effort (that I would have certainly appreciated more if I had known these existed before (@brcolow and me) duplicated parts of it).
I'm not sure if the additional build and test complexity is worth the temporary improvement, not to mention printing to System.out to tell people to add command line parameters leaves a sour taste.
Supporting Java 17 is certainly a benefit, but we also have code paths for 18, 19 and 20; that's doubling the (already too big) amount of code paths that would theoretically be tested, maintained and supported.
Regarding Java 18, 19, 20 code paths: I'm not sure if there is any reasonably-sized subset of people who balk at this version requiring Java 22, but using an experimental API in out-of-support Java 18 is just fine?

maths22 · 2024-09-04T16:24:47Z

Honestly I meant to share what I had done with upstream after we had done our first release using it and made sure it didn't crash and burn, and then I forgot about checking on upstream until I needed to fix it to work on java 21 yesterday.
I'm not going to claim the logging to System.out is something that should be upstreamed; it was just the easiest way for me to add diagnostics for my internal purposes. If I was to actually file a PR I would think of a different method
I think we could reasonably drop java 18/19/20 and just fall back to the existing implementation if you really tried to run things on one of those; I implemented them because once I had MR jar support working it was fairly trivial to add them, but that doesn't mean they are actually worth supporting.
Annoyingly if we want to support java 21 (FFM is in preview there, but it's the current LTS) in addition to java 22 we need a multi-release jar with separate compliation of identical files for java 21 and 22 because java 22 won't load classes compiled with java 21 preview features (for good reason, but mildly annoying in this case)
Note on command-line parameters: it looks like in the final implementation java went with for java22, they don't require --enable-native-access; instead they just log the following warning, as seen in this project's build build on this branch.

WARNING: A restricted method in java.lang.foreign.AddressLayout has been called
WARNING: java.lang.foreign.AddressLayout::withTargetLayout has been called by dev.dirs.impl.Windows in an unnamed module
WARNING: Use --enable-native-access=ALL-UNNAMED to avoid a warning for callers in this module
WARNING: Restricted methods will be blocked in a future release unless native access is enabled

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 --add-modules jdk.incubator.foreign flag and the --enable-native-access=ALL-UNNAMED, but java 21 only needs the --enable-preview flag (and will just then log the same warning).

maths22 · 2024-09-04T16:49:43Z

src/main/java/dev/dirs/impl/Windows.java

+	private static String getDir(String folderId) {
+		try (var arena = Arena.ofConfined()) {
+			MemorySegment guidSegment = arena.allocate(GUID_LAYOUT);
+			if (CLSIDFromString(createSegmentFromString(folderId, arena), guidSegment) != 0) {


You can see what doing the conversion in java looks like here. https://github.com/maths22/directories-jvm/blob/ffm-poc/src/main/java22/com/maths22/directories/WindowsNativeUtil.java#L54

I'm not going to claim this is the best code for this-it could definitely be cleaned up substantially for readability, but it does function correctly.

maths22 · 2024-09-04T16:51:30Z

src/main/java/dev/dirs/impl/Windows.java

+				throw new AssertionError("failed converting string " + folderId + " to KnownFolderId");
+			}
+			MemorySegment path = arena.allocate(C_POINTER);
+			SHGetKnownFolderPath(guidSegment, 0, MemorySegment.NULL, path);


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 )

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.

soc · 2024-09-05T11:02:14Z

Note on command-line parameters: it looks like in the final implementation java went with for java22, they don't require --enable-native-access; instead they just log the following warning, as seen in this project's build build on this branch.
[...]
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).

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...

soloturn · 2024-10-16T03:15:19Z

Note on command-line parameters: it looks like in the final implementation java went with for java22, they don't require --enable-native-access; instead they just log the following warning, as seen in this project's build build on this branch.
[...]
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).

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.

soc · 2024-10-17T14:27:12Z

Status:

first two commits are merged
third and fourth commit will happen after rebase

- Drop all existing mechanisms for retrieving this info on Windows - This increases the required Java version of the library to 22 Fixes #49.

soc mentioned this pull request Aug 1, 2024

Deal with Windows support being an ongoing shit show #49

Open

soc commented Aug 1, 2024

View reviewed changes

src/main/java/dev/dirs/impl/Windows.java Show resolved Hide resolved

soc force-pushed the topic/windows-ffi branch from 1a9a8f0 to 7c2ef55 Compare August 1, 2024 21:51

soc commented Aug 1, 2024

View reviewed changes

soc force-pushed the topic/windows-ffi branch 3 times, most recently from 4333b43 to b6332c3 Compare August 2, 2024 19:50

soc commented Aug 5, 2024

View reviewed changes

maths22 reviewed Sep 4, 2024

View reviewed changes

soloturn mentioned this pull request Oct 16, 2024

use dev.dirs to calculate terasology directories MovingBlocks/Terasology#5284

Open

soc force-pushed the main branch from 9e5e0e5 to 68b72ab Compare October 17, 2024 14:25

soc force-pushed the topic/windows-ffi branch 2 times, most recently from d25f61f to 91a3c6f Compare October 17, 2024 14:44

soc and others added 2 commits October 17, 2024 16:58

Split operating-specific functionality into separate files

d8b2093

Use Java 22's foreign function API for Windows

91a3c6f

- Drop all existing mechanisms for retrieving this info on Windows - This increases the required Java version of the library to 22 Fixes #49.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Java 22's foreign function API for Windows #61

Use Java 22's foreign function API for Windows #61

soc commented Aug 1, 2024

soc Aug 1, 2024 •

edited

Loading

brcolow Aug 1, 2024

sideeffffect commented Aug 2, 2024

soc commented Aug 2, 2024

sideeffffect commented Aug 5, 2024 •

edited

Loading

soc commented Aug 5, 2024

soc Aug 5, 2024 •

edited

Loading

maths22 Sep 4, 2024

sideeffffect commented Aug 5, 2024

soc commented Aug 5, 2024

sideeffffect commented Aug 5, 2024

soc commented Aug 5, 2024

maths22 commented Sep 3, 2024

sideeffffect commented Sep 4, 2024

soc commented Sep 4, 2024 •

edited

Loading

maths22 commented Sep 4, 2024

maths22 Sep 4, 2024

maths22 Sep 4, 2024 •

edited

Loading

brcolow Sep 5, 2024 •

edited

Loading

soc commented Sep 5, 2024 •

edited

Loading

soloturn commented Oct 16, 2024 •

edited

Loading

soc commented Oct 17, 2024 •

edited

Loading

Use Java 22's foreign function API for Windows #61

Are you sure you want to change the base?

Use Java 22's foreign function API for Windows #61

Conversation

soc commented Aug 1, 2024

soc Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

brcolow Aug 1, 2024

Choose a reason for hiding this comment

sideeffffect commented Aug 2, 2024

soc commented Aug 2, 2024

sideeffffect commented Aug 5, 2024 • edited Loading

soc commented Aug 5, 2024

soc Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

maths22 Sep 4, 2024

Choose a reason for hiding this comment

sideeffffect commented Aug 5, 2024

soc commented Aug 5, 2024

sideeffffect commented Aug 5, 2024

soc commented Aug 5, 2024

maths22 commented Sep 3, 2024

sideeffffect commented Sep 4, 2024

soc commented Sep 4, 2024 • edited Loading

maths22 commented Sep 4, 2024

maths22 Sep 4, 2024

Choose a reason for hiding this comment

maths22 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

brcolow Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

soc commented Sep 5, 2024 • edited Loading

soloturn commented Oct 16, 2024 • edited Loading

soc commented Oct 17, 2024 • edited Loading

soc Aug 1, 2024 •

edited

Loading

sideeffffect commented Aug 5, 2024 •

edited

Loading

soc Aug 5, 2024 •

edited

Loading

soc commented Sep 4, 2024 •

edited

Loading

maths22 Sep 4, 2024 •

edited

Loading

brcolow Sep 5, 2024 •

edited

Loading

soc commented Sep 5, 2024 •

edited

Loading

soloturn commented Oct 16, 2024 •

edited

Loading

soc commented Oct 17, 2024 •

edited

Loading