-
Notifications
You must be signed in to change notification settings - Fork 844
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
Update GraalJS to 23.0.3 #7248
Update GraalJS to 23.0.3 #7248
Conversation
Commit validation fails. All javac.source=17
javac.target=17 |
d35c809
to
7c265c6
Compare
da32e87
to
32a10a6
Compare
The GraalVM tests have tested outdated GraalVM execution modes. In the beginning GraalVM had the mode where the base VM was installed and additional languages were installed in to that distribution. That mode was replaced by either distinctive distributions for the supported languages or by loading the JARs into the classpath. The latter approach is in line with the NetBeans approach to loading JS support through GraalVM/Truffle.
32a10a6
to
3ab04b1
Compare
23.0.3 is still broken arg
What a clusterf****. Its so great, that rhino was replaced with something "better"...... |
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.
LGTM
@entlicher what is the graal-sdk JDK compatibility matrix? Version 24.0.0 sounds like it is using the old, non-jdk versioning scheme? |
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.
Upgrading the GraalJS library seems like a reasonable direction to me (and when I checked, it seems to resolve the problem on JDK 22). I didn't notice the 24.0.0 release already happened, sorry. There are some tests failing, which may require a bit of investigation. Also, a few mostly small/nit comments inline.
@@ -14,5 +14,14 @@ | |||
# KIND, either express or implied. See the License for the | |||
# specific language governing permissions and limitations | |||
# under the License. | |||
8E62643FD621053E00EE52D797C7FBD08D7FDEDC org.graalvm.sdk:graal-sdk:20.3.0 | |||
8968BDBE4058F4E6610AF0DC184BD885B236A2B1 org.graalvm.sdk:launcher-common:20.3.0 | |||
8AA4ECE060629AAAED57215A5FD8C377EFF1A191 org.graalvm.sdk:graal-sdk:24.0.0 |
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.
Technically, graal-sdk
may not be needed - it seems to be just a placeholder package, and the original packages probably have been split up into some of the other bundles here.
As a side note, maybe there's a better name for the module? maybe libs.graal
?
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 would prefer not to rename modules, unless necessary. We can have module_auto_deps entry, I'd stick with the current name.
8968BDBE4058F4E6610AF0DC184BD885B236A2B1 org.graalvm.sdk:launcher-common:20.3.0 | ||
8AA4ECE060629AAAED57215A5FD8C377EFF1A191 org.graalvm.sdk:graal-sdk:24.0.0 | ||
06055903437576484DCC6545AF995FF67E0A3B55 org.graalvm.sdk:launcher-common:24.0.0 | ||
335646EC205A3BCE0D518748D08A5C45900A9869 org.graalvm.shadowed:jline:24.0.0 |
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 guess the license file for JLine is not present (which may be reason why the paperwork job failed). (And I don't think pure UPL would be the correct license for that.)
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.
Fixed in #7253
@@ -2,8 +2,8 @@ Name: Graal SDK and Truffle API | |||
Description: Graal SDK and Truffle API |
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.
Nit: please rename the file to ...-24.0.0-...
.
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.
Fixed in #7253
@matthiasblaesing I'd like to participate on fixing the tests -- can I push changes to your branch, or would you prefer a separate branch over your PR ? |
@sdedic thanks for taking a look! Feel free to fork. I'd also like to continue experimenting and so it would be good if you could create a separate PR. What I'm not happy with in my current approach is:
For one problem I saw the last few days, I think I found a solution yesterday: The services of the modules need to be exported, so that they can be found by the ServiceLoader of the NetBeans module system. |
I think a big wrapper module makes sense here and would make maintenance easier I think. Updates would update the whole dependency list of the wrapper, instead of having to track them down individually. Splitting it up would be still possible later if needed. |
I am also slightly concerned with removal of the GraalVM testsuites. The recent GraalVM for JDK21 has dropped builtin JS support, so that's fine. But GraalVM for JDK17 still has |
@matthiasblaesing I probably do not have enough context, sorry .... what issue was addressed by setting
? |
I am pretty sure the Graal JDK dropped everything. Languages are now regular dependencies for the application and not part of the env.
21, 22 does not. To plan ahead we should have test setups which work on future proof deployments. The graal debugger/polyglot tests do also use far more languages than just js. The same tests use ruby, python java and js. The last bump to EOL GraalVM 11 bumped as far as possible, I literally had all releases between 11 and 21 downloaded and checked what languages they supported - since it changed from update release to update release(!) (#6369 (comment), #6369 (comment)). |
Our current roadmap has JDK 17 supported until 3/2026, and I'm not entirely sure where any decision on running on GraalVM fits into that either. @mbien am I right in thinking that running the scripting tests on JDK 22 would have picked up the reported issue earlier? |
FYI - tests enabled in this PR are green in #7253, ready to merge into this one, after review. |
Truffle has excessive logging, when it is run on a JVM it does not support (without JVMCI enabled). As we are aware of that fact ant that it will take years for a JVMCI supported JVM to reach all users, it is useless to show users errors, they can do nothing about.
Without it I'm seeing this:
|
03bd981
to
d6ed8e4
Compare
I pushed an update, but I'm still not happy. With that I think this works on OpenJDK 17, 21, 22 GraalVM 21, 22 and 17 (if JS is installed). I tested the modules:
on the mentioned JDKs. I trust the core.network tests, I don't trust the rest of them, especially not on GraalVM 17. On GraalVM 17:
|
what does "the GraalVM.js tests work only when invoked manually" mean ? |
Well. There are 3 possible cases with different versions / states of GraalVM. Easiest: GraalVM for JDK 21 or generally GraalVM JDKs of internal version greater (I hope) or equal to 23.1.0 -- these do not contain org.graalvm.sdk module at all, so Polyglot class API and truffle API is not available on the bootclassspath. No Now GraalVM for JDK17, without JS installed. I did a bad job validating my changes. Because this combination fails:
The GraalVM does not contain Third option, GraalVM for JDK17 (better: JDK with polyglot API) and js installed in it: things are fine - Jbecause JDK-provided JS implementation is loaded. Although that wasn't the exactly for why Why the tests for So one way is to fix the test. It can run without the classloading wrapper on non-polyglot GraalVM and on Polyglot + JS enabled GraalVM. It will not succeed on Polyglot JDK without JS as JS won't be enumerated by JDK. This also means that NB will not be able to bring its JS implementation working on Polyglot JDK without JS installed in it (= GraalVM for JDK17 and older GraalVMs, like 22.3 /JDK17). |
Sorry, bad wording. "the GraalVM.js tests work only when invoked manually" might be better phrased "the GraalVM.js tests work only when invoked individually". I mean, that when all tests of the GraalVM.js module are run I see 3 failing and 2 successful tests. The 3 failing tests work, when run isolated. I think it is ok, to support GaalJS 17 only on an untested basis as the platform itself is unstable (GraalVM as a whole) or a niche (GraalVM 17). I also think it is fair, that people running such an experimental platform must be prepared to take the pain (in the case of GraalVM 17, install JS). For a future perspective, this might become a problem: netbeans/ide/libs.graalsdk/src/org/netbeans/libs/graalsdk/impl/Installer.java Lines 27 to 41 in 14fb065
But I have some hope, that in the future Truffle/GraalVM will not stay with native libraries per platform or have a sane strategy to work then without failing hard. Seeing a Java runtime loading a native library by default and calling that the future of the JVM can't be the future. I reached the end of what I can do. I would only squash this. |
I do not entirely think that GraalVM is an 'exerimental' technology ... but anyway, I think there are still some issues:
|
If you mean that you did "Run test" from the IDE on an individual test* method... then it's working because the module environment for the test was not properly set up in |
I consider GraalVM stable once project Galahad (integration into the JDK) has happend and the first LTS JDK with that technology is out. I'm still irritated, that Nashorn was dropped before the replacement was ready.
I ran sigtest - and the signature of the module changed in an incompatible way. So the release major version of the module would go up and thus dependencies will break anyway. I would also assume, that it will break again in the next few years, until the technology is stable.
My reasoning: I have limited time and release of NB 22 is planed in the not too distant future. To me the question was: Support OpenJDK 22 or GraalVM 17. Supporting the current JVM is IMHO more important, that a VM with experimental extensions. This was the state I could reach with my resources. I never claimed, that it was the best/right approach.
I would prefer a better solution, so it would be great if you could come up with one. 😆 |
@matthiasblaesing understood... I planned to work on this today, but other schedule creeped in (sorry), so I hope I'll do something over the weekend to have a stable thing (or a failure ;)) before branching. |
Abandoned in favor of #7268 |
Closes: #7245