WIP: Add `jvm.dll` to the `%PATH%` on Windows #30

jakirkham · 2018-06-15T01:31:42Z

When building code that relies on Java's JNI (Java Native Interface), which is the case for any C/C++ library that interacts with Java, the jvm library needs to be loadable. On Unix, this seems to work out of the box thanks to the existence of rpaths. However on Windows, we need to explicitly add jvm.dll to the path or code requiring it will fail with a loading error.

To fix this, we add the path to jvm.dll to the %PATH% on Windows as part of the activation script. This ensures that jvm.dll can be loaded by libraries built with JNI that look for it. Whenever deactivation occurs, we strip this location from the %PATH%.

cc @hanslovsky

conda-forge-linter · 2018-06-15T01:31:47Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

hanslovsky · 2018-06-15T03:22:38Z

Good idea to fix the root of the problem instead of the symptoms.

The install steps for the activate scripts used a mixture of copying and echoing the contents. This tries to use a standard method used by other activate scripts in the conda-forge recipes to simply copy over the scripts while still respecting the layout here.

jakirkham · 2018-06-15T14:14:06Z

Hmm...so the compilation step proceeds ok with the test C file, but running it somehow causes the build to fail. Not totally sure why. 🤔

ref: https://ci.appveyor.com/project/conda-forge/openjdk-feedstock/build/1.0.105/job/c6se8l95qjo3wdyh

mingwandroid · 2018-06-15T14:31:18Z

I recommend not changing PATH in activation scripts. It will create a load of problems.

mingwandroid · 2018-06-15T14:32:45Z

Also, you are claiming to add something then remove it later. This is not at all what's being done. PATH alterations made during activation get lost with this full backup and full restore scheme.

jakirkham · 2018-06-15T14:35:42Z

One step at a time, @mingwandroid, I'd like to see us fix the issue first and then we can figure out what other options we'd like to try.

mingwandroid · 2018-06-15T14:37:11Z

Maybe you should just switch to IntelliJ JDK instead like I did? It does not suffer from this bug.

mingwandroid · 2018-06-15T14:38:39Z

And I worry about your 'one step at a time' statement, fix things correctly first time, it's far from guaranteed I'll spot or remember such a mistake if I don't comment upon it immediately.

jakirkham · 2018-06-15T14:42:38Z

Maybe you should just switch to IntelliJ JDK instead like I did? It does not suffer from this bug.

Don't have time to investigate that. If someone wants to start that PR, great. For now, I think we can solve this much more simply.

And I worry about your 'one step at a time' statement, fix things correctly first time, it's far from guaranteed I'll spot or remember such a mistake if I don't comment upon it immediately.

By which I mean let's see the tests pass. Just getting things to compile on Windows in the test phase was being a bit of a chore.

Now that it is working we can explore other options. Already have a few in mind.

As we don't actually use `curl` on Windows, just drop it as a dependency.

Include `errorlevel` checks and lower case commands as Windows is not case sensitive anyways.

Since we are done with this temporary variable, just unset it when we deactivate.

As `move *` misses subdirectories, which appears to result in lots of things end up missed on Windows, just switch to `xcopy` to make sure we get everything we need.

Since manipulating the `%PATH%` is a bad idea and we need the loader to find `jvm.dll` and its dependencies, just copy `jvm.dll` and related DLLs to some place that is already on the `%PATH%` on Windows namely `%LIBRARY_BIN%`.

This is being set for the Unix builds to point out where the `jvm` shared object is, but not on Windows. So go ahead and add it to the Windows activation/deactivation scripts.

Basically copies over the test from Unix and tries to implement it for Windows. Add missing include path to the Windows test Fix up compiler options

Now that the compilers are in use for the test phase, re-render the feedstock to properly select them for each platform.

Rebuild `openjdk` to get a new package with the fix to the activation script needed by libraries using JNI.

jakirkham · 2018-06-15T16:22:09Z

recipe/bld.bat

+copy %LIBRARY_PREFIX%\jre\bin\server\jvm.dll %LIBRARY_BIN%\
+if errorlevel 1 exit 1
+copy %LIBRARY_PREFIX%\jre\bin\*.dll %LIBRARY_BIN%\
+if errorlevel 1 exit 1


This second copy is probably the wrong approach as it is not targeted enough. Was adding it in as jvm.dll said it was missing dependencies, but didn't say which ones. However this is causing new problems, where it is looking for jvm.cfg in the wrong location and not finding it. Links to the relevant logs demonstrating these issues in order below.

ref: https://ci.appveyor.com/project/conda-forge/openjdk-feedstock/build/1.0.112/job/8eux8vxmsrb4bcql
ref: https://ci.appveyor.com/project/conda-forge/openjdk-feedstock/build/1.0.115/job/nemxfkj4135v2ies

jakirkham · 2018-06-15T16:51:00Z

recipe/scripts/activate.bat

@@ -1,2 +1,5 @@
 set "JAVA_HOME_CONDA_BACKUP=%JAVA_HOME%"
 set "JAVA_HOME=%CONDA_PREFIX%\Library"
+
+set "JAVA_LD_LIBRARY_PATH_BACKUP=%JAVA_LD_LIBRARY_PATH%"
+set "JAVA_LD_LIBRARY_PATH=%JAVA_HOME%\jre\bin\server"


Was hoping this would help the issue on Windows (as it does on Unix), but it seems to be ignored AFAICT. If requested, we can drop it, but would slightly prefer to keep (though don't have strong feelings on it).

As far as I know there is no LD_LIBRARY_PATH on Windows, so what would be the purpose of adding a JAVA_LD_LIBRARY_PATH?

True. Wasn't sure if Java still defined it's own JAVA_LD_LIBRARY_PATH for Windows anyways. Can drop it if that is not the case.

jakirkham · 2018-07-20T15:30:24Z

Unfortunately I lack a Windows machine to debug this further. So if someone can try this and advise me how to proceed, that would be very helpful.

hadim · 2018-11-20T22:44:47Z

I have used some of your commits from this PR to #34. Hope it's ok for you. I didn't include the Windows JNI thing since it does not seem to be ready yet.

jakirkham · 2018-11-22T06:01:04Z

Of course, happy to hear this is useful and to have help integrating it no matter how much was used.

Completely understand punting on the Windows JNI thing. Based on my notes, it looks like commit ( 901125b ) fixed the issue as demonstrated in this build. While that changeset functionally worked, the fact that it hacks the PATH is at best inelegant and at worst error prone. Not aware of a better solution. Though I'm not a Windows JNI expert. Maybe this is useful should you and/or others decide to investigate further.

Edit: Have updated the PR's contents to match those of commit ( 901125b ) for clarity. These are now under the new commit ( 0b3e686 ).

Edit 2: Perhaps hard-linking would be a reasonable solution to address the Windows JNI issues.

Resurrects commit ( 901125b )'s contents, which hacks the `PATH` on Windows to get JNI working. This is by no means ideal and almost certainly should not be used as is. However it is a good starting point where things have been tested and verified to be functional.

conda-forge-linter · 2018-11-22T06:06:09Z

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

jakirkham mentioned this pull request Jun 15, 2018

Add PyJNIus recipe conda-forge/staged-recipes#6091

Merged

jakirkham force-pushed the add_jvm_to_path_win branch from 3a1030f to 2eb75e9 Compare June 15, 2018 02:22

jakirkham changed the title ~~Add jvm.dll to the %PATH% on Windows~~ WIP: Add jvm.dll to the %PATH% on Windows Jun 15, 2018

jakirkham force-pushed the add_jvm_to_path_win branch 3 times, most recently from 644d0ad to d12b30e Compare June 15, 2018 03:19

jakirkham force-pushed the add_jvm_to_path_win branch from d12b30e to 5ef7179 Compare June 15, 2018 04:44

jakirkham force-pushed the add_jvm_to_path_win branch 2 times, most recently from 60bcf09 to 2cec88c Compare June 15, 2018 06:05

jakirkham force-pushed the add_jvm_to_path_win branch 6 times, most recently from bfb6dcc to ccc5fb0 Compare June 15, 2018 15:57

jakirkham added 5 commits June 15, 2018 12:06

Drop curl requirement on Windows

7a82fac

As we don't actually use `curl` on Windows, just drop it as a dependency.

Tidy up Windows build script

f042024

Include `errorlevel` checks and lower case commands as Windows is not case sensitive anyways.

Undefine %JAVA_HOME_CONDA_BACKUP% in deactivate

6b3fffc

Since we are done with this temporary variable, just unset it when we deactivate.

Include some missing files on Windows

62ab94a

As `move *` misses subdirectories, which appears to result in lots of things end up missed on Windows, just switch to `xcopy` to make sure we get everything we need.

Copy DLLs needed for JNI to %LIBRARY_BIN%

0389ac4

Since manipulating the `%PATH%` is a bad idea and we need the loader to find `jvm.dll` and its dependencies, just copy `jvm.dll` and related DLLs to some place that is already on the `%PATH%` on Windows namely `%LIBRARY_BIN%`.

jakirkham added 4 commits June 15, 2018 12:10

Set JAVA_LD_LIBRARY_PATH on Windows

51ca440

This is being set for the Unix builds to point out where the `jvm` shared object is, but not on Windows. So go ahead and add it to the Windows activation/deactivation scripts.

Test JNI on Windows

ddf9c98

Basically copies over the test from Unix and tries to implement it for Windows. Add missing include path to the Windows test Fix up compiler options

MNT: Re-rendered with conda-smithy 3.1.5 and pinning 2018.05.22

9573493

Now that the compilers are in use for the test phase, re-render the feedstock to properly select them for each platform.

Bump build number to 2

f7de6ad

Rebuild `openjdk` to get a new package with the fix to the activation script needed by libraries using JNI.

jakirkham force-pushed the add_jvm_to_path_win branch from ccc5fb0 to f7de6ad Compare June 15, 2018 16:10

jakirkham commented Jun 15, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add `jvm.dll` to the `%PATH%` on Windows #30

WIP: Add `jvm.dll` to the `%PATH%` on Windows #30

jakirkham commented Jun 15, 2018

conda-forge-linter commented Jun 15, 2018

hanslovsky commented Jun 15, 2018

jakirkham commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

jakirkham commented Jun 15, 2018 •

edited

Loading

mingwandroid commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

jakirkham commented Jun 15, 2018

jakirkham Jun 15, 2018

jakirkham Jun 15, 2018

hanslovsky Jul 17, 2018

jakirkham Jul 20, 2018

jakirkham commented Jul 20, 2018

hadim commented Nov 20, 2018

jakirkham commented Nov 22, 2018 •

edited

Loading

conda-forge-linter commented Nov 22, 2018

WIP: Add jvm.dll to the %PATH% on Windows #30

Are you sure you want to change the base?

WIP: Add jvm.dll to the %PATH% on Windows #30

Conversation

jakirkham commented Jun 15, 2018

conda-forge-linter commented Jun 15, 2018

hanslovsky commented Jun 15, 2018

jakirkham commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

jakirkham commented Jun 15, 2018 • edited Loading

mingwandroid commented Jun 15, 2018

mingwandroid commented Jun 15, 2018

jakirkham commented Jun 15, 2018

jakirkham Jun 15, 2018

Choose a reason for hiding this comment

jakirkham Jun 15, 2018

Choose a reason for hiding this comment

hanslovsky Jul 17, 2018

Choose a reason for hiding this comment

jakirkham Jul 20, 2018

Choose a reason for hiding this comment

jakirkham commented Jul 20, 2018

hadim commented Nov 20, 2018

jakirkham commented Nov 22, 2018 • edited Loading

conda-forge-linter commented Nov 22, 2018

WIP: Add `jvm.dll` to the `%PATH%` on Windows #30

WIP: Add `jvm.dll` to the `%PATH%` on Windows #30

jakirkham commented Jun 15, 2018 •

edited

Loading

jakirkham commented Nov 22, 2018 •

edited

Loading