-
Notifications
You must be signed in to change notification settings - Fork 23
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
Deal with Windows support being an ongoing shit show #49
Comments
pardon my ignorance but what is the issue with |
I think embedding a tiny JNI library could work too, as we're only calling simple system calls. I had started toying with that some time ago. IIRC, the main hurdle I ran into was charset conversions, from what |
@phongngtuan It's discussed below #26 (comment), these environment variables are not always in sync with the system call, which returns the true values. |
I feel strongly that precompiled native code with fallback to shelling out is the way to go but I also understand why @soc has skepticism. My experience is that while JNI is definitely a pain and does come with risks, it can be incorporated successfully. For sbt, I wrote a library that speeds up recursive directory listing by bundling pre-compiled code for some platforms. If the library is unable to load the native code for any reason it falls back to jvm built-ins. This code shipped with sbt 1.3.0 and there has only been one issue that came up in pre-release with the windows code: sbt/sbt#4690. Perhaps unsurprisingly given @alexarchambault's comment above, it was also related to Window's use of wide characters by default: swoval/swoval@e425e4c and was basically resolved by switching from NewStringUTF to NewString. Loading a small dll and running it is generally much faster than shelling out to a process. There are certainly safety issues with using c but I think for this specific use case they are minimal because naively it seems as though the JNI code would just make a single system call and return a java value back to the jvm. This means that it wouldn't need to use the heap, mitigating one of the biggest worries with native code. I am not personally interested in working on this but I would be happy to share my advice and review any changes. I have spent a lot of time incorporating native code into jvm libraries. In addition to swoval, I also added jni support for unix domain sockets and windows named pipes so that we could use them in a graalvm native image for the sbt thin client (JNA uses reflection in a way that the graalvm could not handle): sbt/ipcsocket#8. There are a few avoidable gotchas if the project decides to go down that road. |
So I have this working. I also manually checked that it works with non-ASCII characters. I think I'm going to try to have it built and published from GitHub actions, and have |
For context, I think the JNI / Powershell approach is slightly better than just reading env vars, but more importantly, it should be useful in other places in coursier (terminal-related stuff, Windows env var stuff), which is why I'm trying to stick to it. |
@eatkins My main concern is that I'd really like to avoid having yet-another layer on top of the already existing layers that can fail (or not apply due to obscure reasons/on obscure platforms). I'll be migrating this library to the new Java FFI as soon as Project Panama ships, but in the meantime I think a stop-gap solution that is less brittle than what we currently have is direly needed. |
… unit tests on windows to avoid hanging ProjectImportTest This is required to workaround sbt/sbt#5128; The bug is reproduced on Teamcity, on Windows agents. ProjectImportingTest is stuck indefinitely when the test is run from sbt. It's also reproduces locally when running the test from sbt. But for some reason is not reproduced when running from IDEA test runners. Environment variables which have to be mocked are inferred from - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeCacheDirectory - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeJvmCacheDirectory see also dirs-dev/directories-jvm#49 see also https://github.com/ScoopInstaller/Main/pull/878/files
… unit tests on windows to avoid hanging ProjectImportTest This is required to workaround sbt/sbt#5128; The bug is reproduced on Teamcity, on Windows agents. ProjectImportingTest is stuck indefinitely when the test is run from sbt. It's also reproduces locally when running the test from sbt. But for some reason is not reproduced when running from IDEA test runners. Environment variables which have to be mocked are inferred from - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeCacheDirectory - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeJvmCacheDirectory see also dirs-dev/directories-jvm#49 see also https://github.com/ScoopInstaller/Main/pull/878/files
… unit tests on windows to avoid hanging ProjectImportTest This is required to workaround sbt/sbt#5128; The bug is reproduced on Teamcity, on Windows agents. ProjectImportingTest is stuck indefinitely when the test is run from sbt. It's also reproduces locally when running the test from sbt. But for some reason is not reproduced when running from IDEA test runners. Environment variables which have to be mocked are inferred from - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeCacheDirectory - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeJvmCacheDirectory see also dirs-dev/directories-jvm#49 see also https://github.com/ScoopInstaller/Main/pull/878/files
… unit tests on windows to avoid hanging ProjectImportTest This is required to workaround sbt/sbt#5128; The bug is reproduced on Teamcity, on Windows agents. ProjectImportingTest is stuck indefinitely when the test is run from sbt. It's also reproduces locally when running the test from sbt. But for some reason is not reproduced when running from IDEA test runners. Environment variables which have to be mocked are inferred from - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeCacheDirectory - lmcoursier.internal.shaded.coursier.paths.CoursierPaths.computeJvmCacheDirectory see also dirs-dev/directories-jvm#49 see also https://github.com/ScoopInstaller/Main/pull/878/files
https://i.imgflip.com/5refek.jpg (had to do this, this issue pops up on every Scala tool). |
@RayKoopa welcome! |
Hey there, crossposting my thoughts on this. The scenario here seems to be to remove the PowerShell invocation as it is causing trouble. These options seem viable, which you've mentioned in part above:
I don't know security implications of JNI as I'm not a Java developer, but since you effectively call a C WinAPI method, the call itself wouldn't be more or less secure than doing it in .NET. If you have considered using .NET's base library method
Given this, I'd personally recommend just using JNI / C for this. It seems to be the natural thing Java does for native OS-specific API calls anyway, I presume? If you still want to P/Invoke in .NET, you may like my CodeProject article on that. As mentioned in the dotnet issue, your call in the PS script seems effectively correct, though you can / should use automatic string marshaling to not have to deal with manually converting the returned buffer to a .NET string, and freeing it in all cases afterwards. Also, a side note on the "Public" folder: On Windows, this folder is per-system, not per-user. It also has subfolders for Documents, Downloads, Pictures, etc., but these subfolders can be redirected, even outside of the parent "Public" folder. If a user wants to store a "public document" and expects it to be in Public > Documents, they would have to explicitly query the "PublicDocuments" path, since appending "\Documents" to the public path is out of the question due to the redirection scenario. |
@RayKoopa Thank you! My concern with JNI/C is that I have to maintain/keep/compile a piece of code for every platform Windows runs on. I own a Windows license for exactly zero platforms, and cross-compiling with C seems to be painful as well. Which brings us to using C (compilers), which I'd rather just not. "It can be written safely" is probably true for this use-case, but too many people making this verdict in too many situations is exactly what brought computing into disrepute. I'm starting to wonder whether the registry values are generally reliable. So
|
Hmm I see. I hoped there would be some kind of interoperability offered by JNI that would not require to compile raw C and introduce lots of complexity to your project; kinda like P/Invoke in .NET. Maybe you can find another way; effectively you "just" need to do two standard C calls made by something preinstalled playing together better with Java, and convert the returned memory buffer storing a 0-terminated UTF16 LE string to a Java As you already know, the registry method is unsupported. |
@soc your concerns about maintainability are quite well warranted and I am sympathetic. I can't remember if this came up on other threads but is there a reason why JNA is not considered an option here? I personally kind of hate JNA and would rather just write a JNI library but I think it may alleviate the cross compilation problem. Adding a dependency would be unfortunate but sbt, which I'm guessing is the biggest consumer of this library, already depends on the JNA anyway. Either way, I will reiterate that I don't think the JNI solution is as bad as it may seen. Microsoft for all its many flaws religiously preserves backward compatibility so you just would need a system for compiling the binary once and then checking it in (though I will acknowledge that checking binaries in to a git repo is icky). I have had success cross-compiling jni libraries with mingw, which is available for mac and linux, for x86_64 though I have never tried it for an arm platform. It also would be possible to compile on a CI platform like appveyor (I have built and distributed binaries using appveyor) or github actions (I haven't actually tried this but would assume that github actions would have a mechanism for exporting artifacts). It would indeed be unreasonable for you to be expected to maintain code for platforms that you cannot easily test so you could reasonably draw a line in the sand that any supported windows platform must have freely available cloud vms to build any needed binaries. I am very sympathetic to the desire to avoid JNI or any windows specific building. I don't have a windows license either. The way I handled it was to install windows on virtualbox, which does not require a paid license. It was never pleasant but it worked. I also agree with you ideologically that c is terrible but if the only api the OS provides is a c api, I'm not sure what option there is but to submit to working directly with that api. |
I am going to unsubscribe from this thread. Every time there is a comment, it is like a mini ddos attack on my brain. It is frustrating that from my perspective the only viable solution is being rejected for ideological reasons. This is a disservice to the downstream users of the library who are affected. If someone needs help implementing a jni solution, I have experience and would be happy to offer assistance. Please email me directly and don’t @ me here. |
Sadly JNA is rather big, it would turn this < 10kB library into a 3.7MB one. :-/ (see #16) |
If ever people stumble upon this issue, this is addressed in coursier, via JNI, by:
coursier/directories-jvm isn't published on Maven Central. coursier uses it as a source dependency, then shades it. So if you depend on coursier, you can access it via I didn't test it on Windows ARM64, so I have no idea how it works there (but coursier has a fallback to former powershell stuff in that case). |
@alexarchambault good work! Do you know how large the binaries turned out? I'm experimenting with an approach in dirs-cli and end up with 30KiB (16KiB with Another thought is building an x86 binary and using that on both x86-64 and x86. (Not sure what's the situation on ARM...) |
@soc If it helps, you can implement JNI functions in Rust without too much trouble (see e.g. this code here) so you could call through to your directories-sys-rs crate rather than implementing something in C, and it would then support the same platforms as your Rust code does? |
@DavidGregory084 Interesting, thanks! |
Found some instructions in dirs-dev/directories-jvm#49 (comment)
Found some instructions in dirs-dev/directories-jvm#49 (comment)
Found some instructions in dirs-dev/directories-jvm#49 (comment)
@soc this library might help with the loading native libs part. I found that ZeroMQ uses this library to load a bunch of libraries in its JNI bindings: https://github.com/zeromq/czmq/tree/master/bindings/jni/czmq-jni. That project uses gradle to create the final JAR file in the structure expected by the native-lib-loader. |
Thanks @DavidGregory084, I'll have a look! |
I have some experimental code using the new FFI API of Java 22. That sadly doesn't help all those you are going to be stuck on Java < 22 for the next few years, but perhaps it can serve as a "known good" implementation. |
I posted this in another PR but this may be more a appropriate place: I made a proof-of-concept using Java 22 Foreign Function & Memory API of how to extract a LocalAppData (as an example) known folder id, in case it is helpful for you. https://gist.github.com/brcolow/e6c2e59a3aa29d32d3332bcf10313031 |
Hi @brcolow, thanks for the code! That looks way more fleshed out than my efforts. I'll give it a go next week. |
Hey everyone, does the output on Windows here look sensible?
|
Looks good to me. |
Anyone interested in reviewing the code, it is here: #61 |
- Drop all existing mechanisms for retrieving this info on Windows - This increases the required Java version of the library to 22 Fixes #49.
Really appreciate your work. Sadly, I can not use Java 22. So I did a quick hack to omit powershell.exe and calling pwsh.exe only while omitting the -v -2 params. That worked for me with pwsh >7.4.3. |
That generally might or might not work depending on various Windows Defender settings. |
Many contributors have spent heroic efforts to keep Windows support working, for which I'm greatly thankful.
Though it appears to me that Windows support keeps breaking time and time again – perhaps it's time to think whether a different approach could be less painful and more respectful to the time & efforts of contributors?
I'm short on actual ideas though:
I'm open to suggestions, thoughts, ideas, etc. – what do people (@alexarchambault, @eatkins, @fthomas. @phongngtuan, ...) think?
The text was updated successfully, but these errors were encountered: