-
Notifications
You must be signed in to change notification settings - Fork 874
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 libs.graaljs, libs.graalsdk and libs.truffleapi to 24.0.0 #7268
Conversation
8e3c89d
to
3430b7a
Compare
3430b7a
to
fc77e2a
Compare
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.
From what I can tell, seems reasonable. Thanks!
I've added a comment on the license of the shaded ICU4J - that would be good to update.
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.
Good as it preserves GraalJS for proxy recognition.
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.
Thank you for working through this! This looks good to me, but there are two things I noticed:
- the
.sig
files show class removals. Is it intentional, that the release major version is not bumped? - the references to the GraalVM implementations at runtime in
libs.graalsdk.system
make no sense to me. My understanding was, that the implementations should be loaded from the VM, not from the module. The warnings I mentioned in the inline comment are also there when testing with the generated zip file.
<runtime-relative-path>ext/launcher-common-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/launcher-common-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path></runtime-relative-path> | ||
<binary-origin>external/launcher-common-20.3.0.jar</binary-origin> | ||
<runtime-relative-path>ext/jline-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/jline-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/collections-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/collections-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/jniutils-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/jniutils-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/launcher-common-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/launcher-common-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/nativeimage-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/nativeimage-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/polyglot-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/polyglot-24.0.0.jar</binary-origin> | ||
</class-path-extension> | ||
<class-path-extension> | ||
<runtime-relative-path>ext/word-24.0.0.jar</runtime-relative-path> | ||
<binary-origin>external/word-24.0.0.jar</binary-origin> |
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 think, that the <runtime-relative-path>
entries should not be here/be empty. When running ant tryme
, I see:
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/launcher-common-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/jline-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/collections-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/jniutils-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/nativeimage-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/polyglot-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.InstalledFileLocatorImpl]: module org.netbeans.libs.graalsdk.system in /home/matthias/src/netbeans/nbbuild/netbeans/ide does not own modules/ext/word-24.0.0.jar at org.netbeans.LocaleVariants.findLogicalPath(LocaleVariants.java:246)
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/launcher-common-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/jline-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/collections-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/jniutils-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/nativeimage-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/polyglot-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
WARNING [org.netbeans.core.startup.NbEvents]: The extension /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/ext/word-24.0.0.jar may be multiply loaded by modules: [/home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk-system.jar, /home/matthias/src/netbeans/nbbuild/netbeans/ide/modules/org-netbeans-libs-graalsdk.jar]; see: http://www.netbeans.org/download/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#class-path
I understood the original version in the way, that the binary-origin
is used at development time and the runtime-relative-path
is empty, so that at runtime the binaries are not visibile to the module system (at least not via graalsdk.system
.
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.
the references to the GraalVM implementations at runtime in libs.graalsdk.system make no sense to me.
Thanks for noticing ! Seem I was copying from libs.graal to libs.graal.system (for compilation purposes) and did not prune after. Addressed in b43e68d
In theory, the major version should be updated for incompatible changes. But, in practice, for 3rd party libraries, I don't think that's done always. E.g. I don't think we do that for javac. I guess the question is how many people we expect will run into problems - and I would suspect not that many. Am I wrong? |
Yes, we could do with a defined way of handling third-party incompatible changes. I also think we should add implementation versions to match upstream versions for all third-party libraries. Separate that from the automatically incremented spec versions. We have a few already. Possibly worth considering here? |
It is hard to enforce 100% backward compatibility rules on 3rd party libraries.
Once I analyzed range dependencies and claimed that by the size of the range one can see how much a library is compatible
However NetBeans module system doesn't support range dependencies itself. Yeah, "99% compatibility" (good will compatibility) might then be good enough for 3rd party libraries. |
@matthiasblaesing (or others): I am not well trained in using sigtests, so I'd probably use some advice. From the diff, I can see that The AbstractPolyglotImpl change seems farily"local" to graalsdk-truffle combo. Can you advise how to reliably list all removed/changed things ? I fear that I can loose something when manually searching through diff. |
I've done yet another experiment ... make I would prefer now to continue with non-breaking versioning, I'll add impl version of 24000 (24.00.0) that could be incremented / matched to upgraded graal packages ... |
@sdedic thanks. Trivial point - implementation version is a plain string isn't it. Default is IDE version and git hash. Any reason it cannot be |
Using spec.version.base AND non-numeric implementation-dependency
Not using spec.version.base:
|
Waiting for #7245 to complete tests and be merged; then I consolidate commits here & rebase to prepare this PR for final merge, if approved. |
Ah, yes, that one - 523f01d ?? It kind of makes sense to disable that warning in this particular use case, but happy with whatever works (and it's a minor aspect of this!) |
I am all for disabling (using nice version scheme) - but will the AU work correctly for such modules ? Not much time before branching :) and I would not like to cause release disaster as I usually do ... |
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.
Looks sane to me. The stability of the GraalVM API can only realistically determined by someone having deep knowledge. I know other projects, that from a pure fact perspective broke API, but from a reality-check perspective were fine, so this might match GaalVM.
f414f37
to
05febf1
Compare
Consolidated commits, rebased on latest master. |
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.
@sdedic @matthiasblaesing thanks for looking into this - changes make sense to me.
lets try to update the graal dependencies periodically from now on (if possible) since skipping multiple major versions is usually more stressful.
Once this is merged, I am going to refresh #7206 and bump the CV tests to JDK 22.
URL=https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-${{ matrix.graal }}/graalvm-ce-java11-linux-amd64-${{ matrix.graal }}.tar.gz | ||
URL=https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-${{ matrix.graal }}/graalvm-ce-java17-linux-amd64-${{ matrix.graal }}.tar.gz | ||
curl -L $URL | tar -xz | ||
GRAALVM=`pwd`/graalvm-ce-java11-${{ matrix.graal }} | ||
GRAALVM=`pwd`/graalvm-ce-java17-${{ matrix.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.
for future reference (since I would forget this within a month): the test setup is still on an EOL release due to the fact that current releases do no longer offer all required language extension test dependencies.
CI is using 22.3.1
which is the last known release which supports the full set of extensions (NB tests need R, python, ruby and js):
graalvm-ce-java17-22.3.1]$ bin/gu available
Downloading: Component catalog from www.graalvm.org
ComponentId Version Component name Stability Origin
---------------------------------------------------------------------------------------------------------------------------------
espresso 22.3.1 Java on Truffle Supported github.com
espresso-llvm 22.3.1 Java on Truffle LLVM Java librSupported github.com
js 22.3.1 Graal.js Supported github.com
llvm 22.3.1 LLVM Runtime Core Supported github.com
llvm-toolchain 22.3.1 LLVM.org toolchain Supported github.com
native-image 22.3.1 Native Image Early adopter github.com
native-image-llvm-backend22.3.1 Native Image LLVM Backend Early adopter (experimental) github.com
nodejs 22.3.1 Graal.nodejs Supported github.com
python 22.3.1 GraalVM Python Experimental github.com
R 22.3.1 FastR Experimental github.com
ruby 22.3.1 TruffleRuby Experimental github.com
visualvm 22.3.1 VisualVM Experimental github.com
wasm 22.3.1 GraalWasm Experimental github.com
I think this would be the current GraalVM 17 release:
graalvm-community-openjdk-17.0.9+9.1]$ bin/gu available
Downloading: Component catalog from www.graalvm.org
ComponentId Version Component name Stability Origin
---------------------------------------------------------------------------------------------------------------------------------
espresso 23.0.2 Java on Truffle Supported github.com
espresso-llvm 23.0.2 Java on Truffle LLVM Java librSupported github.com
icu4j 23.0.2 ICU4J Supported github.com
js 23.0.2 Graal.js Supported github.com
llvm 23.0.2 LLVM Runtime Core Supported github.com
llvm-toolchain 23.0.2 LLVM.org toolchain Supported github.com
nodejs 23.0.2.1 Graal.nodejs Supported github.com
regex 23.0.2 TRegex Supported github.com
visualvm 23.0.2 VisualVM Supported github.com
GraalVM releases based on JDK 21 or 22 don't even have gu
anymore (#7248 (comment)).
Hm, so TODO is - how to retain language richness in absence of installable JDK components.... maybe time for some more modules; if we want to retain the functionality. |
Please try and catch if a library JAR update includes new native macOS binaries, and add a comment to the PR. These need to be special cased in our installer building (extracted, signed and repackaged) or we fail Apple notarization. |
This is an alternative approach to #7248, trying just to upgrade the libraries + necessary fixes for tests to pass. I started from a clean
master
, upgraded libs and applied relevant fixes found during development of #7248, but without droppinglibs.graalsdk.system
and restructuring the libraries.So far I tested in locally on
tests seem to pass. I would appreciate a review against #7248 so that all issues addressed there were correctly picked.