-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[WIP] Support both GraalVM 19.2.1 and 19.3.0.2 - 19.2.1 SDK & 19.2.1 GraalVM image #6503
[WIP] Support both GraalVM 19.2.1 and 19.3.0.2 - 19.2.1 SDK & 19.2.1 GraalVM image #6503
Conversation
I dunno what is wrong with CI and it's not testing this... Can you try one more force push please @gwenneg ? |
I got it @geoand. I think it's because it has "WIP" in the title. |
Oh! Never mind. It immediately cancels when I try to trigger it manually. I have no idea why... |
I thought maybe the WIP check was preventing the rest of the checks from proceeding. Still tinkering a little... |
Going to try one more thing.... bear with me... |
Nope, I give up. I have no idea why this won't test. |
Thanks for trying! This one is super weird indeed... |
9de1dd4
to
10c4083
Compare
I'm promoting the PR to see if it helps, but it's not meant to be reviewed or merged for now. |
ee628d9
to
5c6c26c
Compare
Well, now I failed a rebase... this PR is cursed! Let me clean that up :) |
5c6c26c
to
fa59b6c
Compare
fa59b6c
to
9b8585a
Compare
This PR shares with #6487 a
|
@gwenneg you should try to enable JNI for the JSCH extension and see how it goes. |
@gsmet JNI is actually already enabled during the image generation used for the failing test. Here's the Docker command: I am now focusing on fixing the test error since that's the only issue so far while using the 19.2.1 SDK with JDK 8 (JDK 11 still needs to be tested). |
From what I can see locally, This looks like an easy issue to fix, but I don't know why this module is the only one affected. Is the environment variable set in the |
Hm I wonder why it relies on |
Well, I had a look at what's going on during NativeImageConfigBuildStep.java#L83-L122 execution, but maybe there's another way to load the |
By the way, I think NativeImageConfigBuildStep.java#L83-L122 will be partly or maybe completely useless (I'm not sure about the |
In that case maybe the right fix is to have a version check? |
Jsch was extracted from the jgit extension and everything was working before the split. So I would have a look at how the jgit extension was doing this in 1.1. |
|
||
@BuildStep | ||
FeatureBuildItem feature() { | ||
return new FeatureBuildItem(FeatureBuildItem.JSCH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extension was purposely designed to not produce a FeatureBuildItem since we didn't want to appear in the initialization logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good to know, thanks @gastaldi. This was part of a test that didn't produce the expected effect, so it would probably have been removed anyway.
@gwenneg I assume that this need a rebase now? |
@geoand I'll rebase tonight, but it's not really important. A small part of this PR has been pushed on master, but the code is the same in the end. |
OK, it's just that github shows conflicts :) |
Superseded by #6574. |
This PR tests one of the Graal SDK and image combinations decribed here.
svm version (SDK): 19.2.1
GraalVM image version : 19.2.1