Drop GraalVM 19.2.1 support and deprecate enableJni option #6792

gwenneg · 2020-01-25T23:44:08Z

gsmet

This looks very good. I added one question inline and I have another one: I wonder if adding the SunEC library is still necessary to get SSL working? But that's material for another PR if it ends up being useless.

(Marking as Request changes so that we can discuss things before it being merged)

gsmet · 2020-01-26T15:05:36Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

+                } else if (prop.getKey().equals("quarkus.jni.enable") && prop.getValue().equals("false")) {
+                    log.warn("Your application is setting the deprecated 'quarkus.jni.enable' configuration key to false."
+                            + " Please consider removing this configuration key as it is ignored (JNI is always enabled) and it"
+                            + " will be removed in a future Quarkus version.");


Just to be sure: have you tested that you don't have several identical messages when setting quarkus.jni.enable=false?

I did it with #6306 but I'll restest it again with this PR just to be sure.

Unless I missed something, this warning is never displayed with the current Quarkus codebase. I suppose this is an old property that is no longer used by the core extensions, but it can still be used by 3rd parties extensions.

On the other hand, the warning from JniProcessor is displayed when quarkus.jni.enable is set to false.

I don't think we should worry about duplicate warnings here since it's very unlikely to happen.

gwenneg

Including the SunEC lib should not be required anymore, but I agree this should be treated separately.

I need to test this PR with JDK 11 in native mode (btw, thanks @geoand for your indications about how to do it with GitHub actions!), I'll promote the PR when I'm done with the tests.

geoand · 2020-01-26T17:53:21Z

Looking forward to it! Thanks @gwenneg!

gwenneg · 2020-01-26T21:01:19Z

For the record, the GitHub action got triggered with these two changes in native-cron-build.yml:

on:
  pull_request:
    branches:
      - master

and

jobs:
  build:
[...]
    steps:
[...]
      - name: Checkout Quarkus
        uses: actions/checkout@v2
        with:
          repository: gwenneg/quarkus
          ref: issue-6709-drop-graalvm-19.2.1-compatibility

gwenneg · 2020-01-26T21:32:07Z

It's really great to have this GH action available, I don't have to run JDK 11 native tests locally for an entire night anymore 😆

gwenneg · 2020-01-27T08:19:47Z

@geoand It seems the JDK 11 native tests got canceled because of their duration. Is there a way to configure the GitHub action and prevent this? I tried something in c8d7090 but it didn't do anything.

geoand · 2020-01-27T08:24:06Z

I think 6 hours is the max. Did you try executing "only" the integration tests and nothing else like the current cron job does?

gwenneg · 2020-01-27T12:41:20Z

The native tests got canceled after 3h 34m 19s at step 47/75 so it needs to run roughly for another couple hours (JDK 11 tests take so much time compared with the JDK 8 version...). I didn't change anything apart from the branch and job launch condition so it should behave like the current cron job.

gwenneg · 2020-01-27T12:44:11Z

It seems the usual cron job runs these tests in approximately 5 hours so mine shouldn't be interrupted like that. Maybe there's an interruption cause unrelated with the tests duration.

gsmet · 2020-01-27T12:55:05Z

If you have one of the tests of the matrix failing, it auto-cancels the others AFAICS.

gsmet · 2020-01-27T12:56:43Z

We want to set that one to false: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

gwenneg · 2020-01-27T13:44:43Z

Oh then the cancelation could be caused by the failure in the Report step of the JDK 8 action! It happened at the same time.

gsmet · 2020-01-27T14:34:10Z

BTW, there's little change the report part can work in a PR as you're not supposed to have the token.

And things shouldn't be reported to the same issues anyway. So I would drop that part.

geoand · 2020-01-27T14:36:49Z

Like @gsmet mentioned, please change the reporting for your testing so we don't get mixed information in the issue :)

gwenneg · 2020-01-28T06:20:30Z

The native integration tests run finally ended successfully for both GH actions. I'll check the absence of duplicate warnings about quarkus.jni.enable=false tonight.

geoand · 2020-01-28T06:23:40Z

Excellent news!

gastaldi · 2020-01-28T10:53:05Z

Looks like this Draft PR is ready to be promoted to a real PR? :)

geoand · 2020-01-28T10:54:19Z

As long as the changes to the github actions are reverted :)

gwenneg · 2020-01-28T11:07:26Z

@gastaldi It’s not ready to be promoted yet, I need to check for duplicate warnings locally first. And I will revert the actions changes of course.

gwenneg · 2020-01-28T22:43:22Z

The JDK 11 native build and tests all passed successfully (see details here), the GitHub actions changes were removed and I'm done looking for duplicate warnings, so I'm promoting this PR.

core/deployment/src/main/java/io/quarkus/deployment/pkg/NativeConfig.java

gastaldi

Looks good to me. I'll let @gsmet review and decide when to land this PR

gsmet · 2020-01-29T10:53:42Z

Merged, thanks.

gwenneg force-pushed the issue-6709-drop-graalvm-19.2.1-compatibility branch from 166ab30 to 05516c4 Compare January 25, 2020 23:54

gwenneg mentioned this pull request Jan 25, 2020

[WIP] Deprecate enableJni option and always consider it true #6306

Closed

pivovarit mentioned this pull request Jan 26, 2020

Add hazelcast-client extension #6758

Closed

gsmet requested changes Jan 26, 2020

View reviewed changes

gsmet added this to the 1.3.0 milestone Jan 26, 2020

gwenneg commented Jan 26, 2020

View reviewed changes

gwenneg force-pushed the issue-6709-drop-graalvm-19.2.1-compatibility branch from a78b6f6 to 493d1aa Compare January 27, 2020 00:52

Drop GraalVM 19.2.1 support and deprecate enableJni option

4c193ae

gwenneg force-pushed the issue-6709-drop-graalvm-19.2.1-compatibility branch from 8df9d6b to 4fe96c9 Compare January 27, 2020 17:30

gwenneg force-pushed the issue-6709-drop-graalvm-19.2.1-compatibility branch from 925005d to 4c193ae Compare January 28, 2020 20:39

gwenneg marked this pull request as ready for review January 28, 2020 22:45

geoand requested a review from gsmet January 29, 2020 08:49

gastaldi reviewed Jan 29, 2020

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/NativeConfig.java Show resolved Hide resolved

gastaldi approved these changes Jan 29, 2020

View reviewed changes

gsmet approved these changes Jan 29, 2020

View reviewed changes

gsmet merged commit b61f69d into quarkusio:master Jan 29, 2020

gwenneg deleted the issue-6709-drop-graalvm-19.2.1-compatibility branch January 29, 2020 12:50

This was referenced Jan 30, 2020

Deprecate enableJni option as GraalVM 19.3 requires JNI support #6101

Closed

Remove the GraalVM workarounds that are no longer required with 19.3.1 #6179

Closed

gwenneg mentioned this pull request Feb 10, 2020

Remove Caffeine ForkJoinPool#commonPool fix #7113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop GraalVM 19.2.1 support and deprecate enableJni option #6792

Drop GraalVM 19.2.1 support and deprecate enableJni option #6792

gwenneg commented Jan 25, 2020

gsmet left a comment

gsmet Jan 26, 2020

gwenneg Jan 26, 2020 •

edited

Loading

gwenneg Jan 28, 2020

gwenneg left a comment •

edited

Loading

geoand commented Jan 26, 2020

gwenneg commented Jan 26, 2020

gwenneg commented Jan 26, 2020

gwenneg commented Jan 27, 2020

geoand commented Jan 27, 2020

gwenneg commented Jan 27, 2020 •

edited

Loading

gwenneg commented Jan 27, 2020

gsmet commented Jan 27, 2020

gsmet commented Jan 27, 2020

gwenneg commented Jan 27, 2020 •

edited

Loading

gsmet commented Jan 27, 2020

geoand commented Jan 27, 2020

gwenneg commented Jan 28, 2020

geoand commented Jan 28, 2020

gastaldi commented Jan 28, 2020

geoand commented Jan 28, 2020

gwenneg commented Jan 28, 2020

gwenneg commented Jan 28, 2020 •

edited

Loading

gastaldi left a comment

gsmet commented Jan 29, 2020

Drop GraalVM 19.2.1 support and deprecate enableJni option #6792

Drop GraalVM 19.2.1 support and deprecate enableJni option #6792

Conversation

gwenneg commented Jan 25, 2020

gsmet left a comment

Choose a reason for hiding this comment

gsmet Jan 26, 2020

Choose a reason for hiding this comment

gwenneg Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

gwenneg Jan 28, 2020

Choose a reason for hiding this comment

gwenneg left a comment • edited Loading

Choose a reason for hiding this comment

geoand commented Jan 26, 2020

gwenneg commented Jan 26, 2020

gwenneg commented Jan 26, 2020

gwenneg commented Jan 27, 2020

geoand commented Jan 27, 2020

gwenneg commented Jan 27, 2020 • edited Loading

gwenneg commented Jan 27, 2020

gsmet commented Jan 27, 2020

gsmet commented Jan 27, 2020

gwenneg commented Jan 27, 2020 • edited Loading

gsmet commented Jan 27, 2020

geoand commented Jan 27, 2020

gwenneg commented Jan 28, 2020

geoand commented Jan 28, 2020

gastaldi commented Jan 28, 2020

geoand commented Jan 28, 2020

gwenneg commented Jan 28, 2020

gwenneg commented Jan 28, 2020 • edited Loading

gastaldi left a comment

Choose a reason for hiding this comment

gsmet commented Jan 29, 2020

gwenneg Jan 26, 2020 •

edited

Loading

gwenneg left a comment •

edited

Loading

gwenneg commented Jan 27, 2020 •

edited

Loading

gwenneg commented Jan 27, 2020 •

edited

Loading

gwenneg commented Jan 28, 2020 •

edited

Loading