Refactor of env.py

[cmacdonald]

Please consider this PR for review and testing.

I couldn't see an easy way to fix the challenge posed in #537, so I refactored env.py. There is now a base class for JavaLocation, which is overridden in platform specific manners. The role of each method is clearly specified. We also use a common, one-time logic for identifying the location of Java.

I have tested on:

• Mac with no JAVA_HOME set, OpenJDK13
• Linux without JAVA_HOME set
• Linux with JAVA_HOME set to Java 8 JDK
• Linux with JAVA_HOME set to Java 11 JDK
• Google Colab linux, OpenJDK 11, no JAVA_HOME set

It requires testing on:

• Windows
• Android. NB: A TODO for Android.
• Linux and Mac with adoptjdk? And also running using JREs after building with JDKs.

Semantics changes:

• JAVA_HOME env var takes precedence over everything else now, systematically
• JVM_PATH env var takes an absolute path rather than a relative one
• All methods return absolute paths, rather than the confusion of relative paths

We should also write a unit test for env.py

[tshirtman]

Thanks, this looks promising, and indeed wrapping up the logic into classes each with the specifics of the platform they care about will make things much clearer.

For android, i'm not sure how much we need, as it's not the JVM running, but instead we connect to the Android Runtime (ART) that is already running, through SDL function, as a comment hints at, but there is still what happens in setup.py, that needs checking.

For missing platforms, i wouldn't be too concerned, windows/osx/linux should cover most of our users, and we can add missing ones if people complain.

[cmacdonald]

Thanks. I agree with your comments. For Android, I couldn’t see any code setting arch in pyjnius, perhaps somewhere else in Kivy calls this? And could you test for Android please?

Getting it through the GitHub Actions would also give me comfort.

[cmacdonald]

I'm also tempted to add /usr/java/latest into the search path as a last resort.

[cmacdonald]

@tshirtman I enabled Github Actions on my fork, and Python 2 failed. Would you be able to give a look:
https://github.com/cmacdonald/pyjnius/runs/739092388?check_suite_focus=true#step:9:58

[tshirtman]

It's probably not hard to fix, i can have a look later, but i think we said we'd drop python2 in next version, so we could also just remove python2 in another PR and rebase this one.

[cmacdonald]

Thanks chief, I'll leave that up to you.

[cmacdonald]

I have updated this patch and its now passing on Windows too. I haven't addressed Python 2, and disabled that from the Github Workflow.

[cmacdonald]

Though I note this possible failure on Windows? https://github.com/cmacdonald/pyjnius/runs/795134983?check_suite_focus=true#step:8:41

[cmacdonald]

I have rebased this patch. All CI pass including the x86 Linux one:

• https://github.com/cmacdonald/pyjnius/actions/runs/321674296
• https://github.com/cmacdonald/pyjnius/actions/runs/321674294

Though the Windows segfault of #561 is still exhibited.

What's needed to get this in?

• A review
• some attention from an Andriod dev - c.f. https://github.com/cmacdonald/pyjnius/blob/java_home/jnius/env.py#L271
• https://github.com/kivy/pyjnius/blob/master/jnius/env.py#L94 uses arch. Looking back, I think that

return [join(self.home, 'libs', self.platform)]

should be enough?

[dogatekin]

@tshirtman Are there any plans to merge this PR in the future before a new release? It also solves a problem for us and would be great to know for planning!

[tshirtman]

Sorry, i’ve been really unattentive to pyjnius lately, i’m ok with merging it as-is if it helps, as it seems i’m not going to find the focus to completely understand it, it seems not to break things, and certainly help with further maintenance.

edit: ah right, we need to ensure it doesn’t break android, i’ll test it there later.

[cmacdonald]

Thanks @tshirtman. I am still subscribed if there are questions on this PR.

The #561 issue is unrelated. I experienced this in PyTerrier - pytest has a faulthandler that tries to identify segfaults - I /think/ these are false positives for pyjnius on Windows, and it appears it can be safely ignored (c.f. https://github.com/terrier-org/pyterrier/blob/master/.github/workflows/push.yml#L82)

[dogatekin]

Thanks for the quick response! If the Android tests turn out ok too, would you also consider making a new release after this merge? You mentioned you don’t have much time for pyjnius these days, but just asking since that would be very useful for us.

[tshirtman]

This was merged on master through a local rebase. Closing.

[cmacdonald]

Thanks for merging @tshirtman !

[tshirtman]

Also forgot to mention and you might have missed it, but i released 1.4.0 with it last night. So many of your improvements are live now :) thanks for the patience !

[cmacdonald]

Yes. I saw, thank you for your efforts too.