brettwooldridge / NuProcess

Low-overhead, non-blocking I/O, external Process implementation for Java
Apache License 2.0
712 stars 84 forks source link

Handle SystemRoot case-insensitively (Windows). #103

Closed bturner closed 5 years ago

bturner commented 5 years ago

This handling emulates java.lang.ProcessEnvironment.toEnvironmentBlock, from the Windows version of the JDK, which also uses a case-insensitive check to detect "SystemRoot" in the supplied environment and copies the value from the running application's environment if it's not set.

bturner commented 5 years ago

At the end of pull request #62, @ilya-klyuchnikov noted that the change as merged caused failures in the AppVeyor build. In the build output, you can see that the ProcessBuilder process only has "SYSTEMROOT", but the NuProcess process has both "SYSTEMROOT" and "SystemRoot". When I build NuProcess on Windows, I get exactly that failure.

In his comment, @ilya-klyuchnikov linked to a commit which fixed the issue. That link is now dead (perhaps he deleted his fork?), but fortunately, with how Github handles fork hierarchies, it's actually possible to view the commit using this repository instead (even though said commit isn't actually referenced in this repository). My changes here are essentially the changes he proposed then, with a few minor alterations.

I'm happy to make any adjustments, if something isn't right. Just let me know.

(For context, we're working on submitting a pull request for issue #95, as part of moving forward with adopting NuProcess for Bitbucket Server. As part of that work, we've set up internal CI to build our changes while we're developing them. However, the Windows job always fails due to this issue.)

brettwooldridge commented 5 years ago

Thanks for the contribution! I will publish a release in a few hours.

bturner commented 5 years ago

Big thanks for the quick turnaround, @brettwooldridge!