Skip to content
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

Java 9 #34

Merged
merged 3 commits into from
Nov 22, 2018
Merged

Java 9 #34

merged 3 commits into from
Nov 22, 2018

Conversation

hadim
Copy link
Member

@hadim hadim commented Nov 20, 2018

Build Java 9.

@conda-forge-linter
Copy link

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.

@hadim hadim mentioned this pull request Nov 20, 2018
@hadim
Copy link
Member Author

hadim commented Nov 20, 2018

@jakirkham Would you be ok to review this? Or ping someone who could?

@hadim hadim mentioned this pull request Nov 20, 2018
@sodre
Copy link
Member

sodre commented Nov 20, 2018

@hadim, I don't object to bringing openjdk up-to-date. Thank you for taking the time to do that.
Ping me back once the PR is passing.

@hadim hadim force-pushed the java9 branch 2 times, most recently from 4faed3a to 41dbbfb Compare November 21, 2018 01:11
@hadim hadim changed the title Bump to Java 9 Java 9 Nov 21, 2018
@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

HOURRAA !!!

Everything looks good. CircleCi takes forever but the build works locally on my machine. Same for #35 and #36.

It will take a few hours for CircleCi to start the build. After that, it will be ready for a review.

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

All green!

Copy link
Member

@sodre sodre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hadim great work! There were a few questions on my side some suggested minor changes.

It is ready to merge after that.

recipe/bld.bat Show resolved Hide resolved
recipe/bld.bat Outdated Show resolved Hide resolved
recipe/build.sh Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@hadim hadim force-pushed the java9 branch 2 times, most recently from 8628566 to df01c78 Compare November 21, 2018 15:31
@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [45]

@hadim hadim force-pushed the java9 branch 2 times, most recently from 303c4ab to 9fd33f6 Compare November 21, 2018 15:34
@conda-forge-linter
Copy link

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.

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

@ctrueden I tested the Linux build locally (conda build recipe/ after cloning this recipe) and I got the following error:

$ python -c "import jnius;System = jnius.autoclass('java.lang.System');print(System.getProperty('java.version'))"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/hadim/local/conda/envs/java/lib/python3.6/site-packages/jnius/__init__.py", line 13, in <module>
    from .reflect import *  # noqa
  File "/home/hadim/local/conda/envs/java/lib/python3.6/site-packages/jnius/reflect.py", line 15, in <module>
    class Class(with_metaclass(MetaJavaClass, JavaClass)):
  File "/home/hadim/local/conda/envs/java/lib/python3.6/site-packages/six.py", line 827, in __new__
    return meta(name, bases, d)
  File "jnius/jnius_export_class.pxi", line 111, in jnius.MetaJavaClass.__new__
  File "jnius/jnius_export_class.pxi", line 161, in jnius.MetaJavaClass.resolve_class
  File "jnius/jnius_env.pxi", line 11, in jnius.get_jnienv
  File "jnius/jnius_jvm_dlopen.pxi", line 90, in jnius.get_platform_jnienv
  File "jnius/jnius_jvm_dlopen.pxi", line 59, in jnius.create_jnienv
SystemError: Error calling dlopen(b'/home/hadim/local/conda/envs/java/jre/lib/amd64/server/libjvm.so': b'/home/hadim/local/conda/envs/java/jre/lib/amd64/server/libjvm.so: cannot open shared object file: No such file or directory'

The true location of libjvm.so is /home/hadim/local/conda/envs/java/lib/server/libjvm.so.

JAVA_HOME is supposed to be CONDA_PREFIX: /home/hadim/local/conda/envs/java

I feel like pyjnius is confused by the absence of the jre folder in Java >= 9.

Any idea?

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

Ok it's a bug in pyjnius: kivy/pyjnius#304

In consequence, we won't be able to use those builds with the scyjava ecosystem before it has been resolved.

That being said those builds are fine in my opinion and can be merged after the (hopefully!) last review by @sodre.

@ctrueden
Copy link
Member

@hadim Why is Java 9 in the mix? Why wasn't your test using OpenJDK 8?

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

Sorry, it wasn't clear. My test with all-already-released Conda packages including OpenJDK 8 is working fine. Here I was testing this OpenJDK 9 PR by building it locally with conda build.

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

So to summarize those OpenJDK 9, 10 and 11 PRs are working fine but we won't be able to use those with pyjnius and packages that depend on it before kivy/pyjnius#304 is fixed.

Is that clear?

@hadim
Copy link
Member Author

hadim commented Nov 21, 2018

@sodre I think #34, #35 and #36 are ready to be reviewed.

@sodre sodre merged commit f16da87 into conda-forge:master Nov 22, 2018
@hadim hadim deleted the java9 branch November 22, 2018 03:29
os=$(uname -s | tr '[:upper:]' '[:lower:]')
gcc -I$JAVA_HOME/include \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should add a compiler requirement in test and update this to $CC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broke out in issue ( #41 ).

@@ -29,19 +32,18 @@ build:

requirements:
build:
- curl

- curl # [unix]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still being used? Looks like it might not be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broke out as issue ( #40 ).

@jakirkham
Copy link
Member

Thanks again for working on this, @hadim. Definitely a much needed improvement for the recipe.

Made some small notes above about a few things we could cleanup. Though have broken these out as issues after the fact for easier tracking. Admittedly these may be carried over from my own PR. So sorry about that. They shouldn't be difficult to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants