-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix #239 - add property javaVersion to support GraalVM 19.3+ #255
Conversation
Thanks for your interest in palantir/gradle-graal, @muhlba91! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
Some test on Windows still fail like before:
@cosminpolifronie maybe you can help here? |
I will look on it tonight. |
On my machine a couple more tests are failing:
This means that native-image cannot find the class that's defining main(). The definition of the class in the test seems to be all right, so I don't know what is causing it. I remember I actually read about this problem in an issue open in the graalvm repository, but I cannot find it and I have no idea how to solve it. I've tried replacing the separators to fix the tests in About the other errors: I've been trying to understand but I don't even know where to start solving them. Most of them fail this way:
All the tests that are failing on my machine:
This is way above my level of understanding of this platform. I don't even know how to properly debug these, as these tests automatically jump to the last instruction in them (most of them fail with an exception well before that), I cannot use breakpoints properly. |
@muhlba91 I made you a PR that fixes the tests on Windows for me. I am not sure about the failing tests from @cosminpolifronie, as all the tests pass on my machine. @cosminpolifronie are you able to use native-image manually? Do the tests from the palantir/gradle-graal develop pass for you? |
I've tried building @frozenice PR on another Windows machine, without success. This is the output of the build:
For the
For the
So my file has been compiled by Java 11 (version 55.0) but it is trying to run it using Java 8 (version 52.0). I have no idea why. Both my JAVA_HOME and GRAALVM_HOME point to the graalvm folder, java from Path variable points to the bin directory of the graalvm folder. I cannot actually build a native-image using a simple hello-world example:
In standard PowerShell Core it says this:
Running native-image.cmd in Developer Command Prompt for VS 2017 (version 15.9.18) it says this:
|
fix download / tests that use the new Java version cache subdir
@frozenice thank you. I have merged your change. @cosminpolifronie finally, I got some tests for Java 11 working and figured out some errors and problems. First, some tests will always fail (obviously) - if you use Java 8 for running the tests, all Java 11 tests will fail and vice-versa... How can we deal with this properly? Secondly, on Windows you need VS 2017+ for GraalVM 19.3+ (see oracle/graal#1852). However, this creates one problem: depending on the VS version and, to make it even worse, also the VS version to be used (Community, Enterpise, ...) change the path for the Additionally, I figured that all command line arguments must be enclosed within |
That's the thing, I only have Java 11 installed, why is it running under Java 8 mode? I have managed to get native-image working standalone using your suggestion to look on oracle/graal#1852 (I was missing the C++ package for ARM and ARM64, why is that needed though?). Thank you! Tests still fail for me, even with calling |
The tests which are testing the default version (19.2.0) and default Java version (8) do not set the property As for the tests, did you check out my latest commit where I set the vcvars64 path to the VS 2019 Community one? I am no specialist in Windows Shells but maybe it matters whether the Gradle‘s executor sets the environment or you do it beforehand? |
Oh, correct, you are right, the default version uses Java 8.
Yes. The call operation most probably fails as I am trying to build on a work machine that has VS 2017/2019 Professional installed. Should I do a pull request with the other possible variants of the path (Professional and Enterprise instead of Community) or will you add those variants? |
Yes, this would be really awesome if you could add it, permitting you have time for it. Also it would add a test on a different VS version than I have (VS 2019 Community). ;) |
This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days. |
I don't think this is stale, happy holidays everyone :-) |
It's not, I will get to it soon after the holidays. Happy holidays! :) |
This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days. |
I've applied the label to keep this open. |
@cosminpolifronie - any updates from your side? :) |
Sadly I am quite overworked these days, as I need to finish my BSc final project until February. Maybe I will be able to push this during the weekend. |
Thank you for the update. 👍 Please let me know how you proceed, otherwise I‘ll have some time next week - most probably. :) |
I didn't even think about other OS'es, good catch. Well, I guess this is done, right? |
Yes, if it looks good from your side now too, I guess we can proceed and merge. :) |
I think some variables and methods in |
Other than that, I think we are ready for merge :). |
…ain windows for clarity
I renamed all VS/Windows only parameters and method to contain the wording windows. |
All good. @iamdanfox could you review this, please? |
Any update on this? |
@cosminpolifronie any way to get this sorted and merged, finally? |
Hi friends, I'm really sorry for the delay and slow reviews on this project. I can try to make time for a full review this weekend. Thanks for the contributions! |
Any update? Thanks! 😄 |
Is there any movement on this? To avoid such a thing in the future, could maybe provide configuration for the full download URL rather than just the base? |
@cosminpolifronie @iamdanfox @carterkozak any updates on this? it would be really awesome to get this merged. otherwise, it renders the plugin slowly useless with more and more people updating. |
What timing, I'd just started digging into this :-) |
I've released https://github.com/palantir/gradle-graal/releases/tag/0.6.1 including this change |
@carterkozak I believe something is wrong with the CircleCI step, preventing the new release from being published. |
- persist_to_workspace: | ||
root: . | ||
paths: [ . ] |
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.
@carterkozak this is causing the duplicate workspace files, we should just delete this.
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.
I'd also be in favor of either not doing check-11 (we don't do that anywhere else!), or merging it into with compile-11, such that this branch of jobs doesn't persist extra stuff to the workspace, nor require compiling with java 11 twice (if we don't persist, both compile-11
and check-11
would have to compile from scratch)
- save_cache: | ||
key: 'graal-cache' |
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.
also we don't need to be saving this cache twice (we're already doing it in check
).
Not to mention I don't believe this never changes, so we should probably checksum some files into this cache key...
0.7.0 has published successfully. |
Before this PR
Issue #239: GraalVM 19.3+ supports Java 11 and has a different download URL and layout causing the plugin to fail when using Java 11 or GraalVM 19.3+.
After this PR
Support for GraalVM 19.3+ and Java 11 including backwards compatibility.
Possible downsides?
Still havent' got the chance to test it on a project.