Skip to content
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

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Apr 14, 2024

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 dropping libs.graalsdk.system and restructuring the libraries.

So far I tested in locally on

  • Oracle JDK 17.0.8
  • Oracle JDK 22+36-2370
  • Oracle JDK 21+35-2513
  • Oracle GraalVM 17.0.10+11.1 - with JS installed and without
  • Oracle GraalVM 21.0.2+13.1
  • GraalVM CE 21+35.1
  • GraalVM CE JDK17, release 22.3.3 - with JS installed and without

tests seem to pass. I would appreciate a review against #7248 so that all issues addressed there were correctly picked.

@sdedic sdedic self-assigned this Apr 14, 2024
@sdedic sdedic added do not merge Don't merge this PR, it is not ready or just demonstration purposes. ci:all-tests [ci] enable all tests labels Apr 14, 2024
@apache apache locked and limited conversation to collaborators Apr 14, 2024
@apache apache unlocked this conversation Apr 14, 2024
@sdedic sdedic added Platform [ci] enable platform tests (platform/*) GraalVM [ci] enable GraalVM tests and removed ci:all-tests [ci] enable all tests labels Apr 15, 2024
@sdedic sdedic added this to the NB22 milestone Apr 15, 2024
@sdedic sdedic changed the title Update libs.graaljs, libs.graalsdk and libs.truffleap to 24.0.0 Update libs.graaljs, libs.graalsdk and libs.truffleapi to 24.0.0 Apr 15, 2024
@sdedic sdedic changed the title Update libs.graaljs, libs.graalsdk and libs.truffleapi to 24.0.0 Update libs.graaljs, libs.graalsdk, libs.truffleapi and libs.graaljs to 24.0.0 Apr 15, 2024
@sdedic sdedic changed the title Update libs.graaljs, libs.graalsdk, libs.truffleapi and libs.graaljs to 24.0.0 Update libs.graaljs, libs.graalsdk and libs.truffleapi to 24.0.0 Apr 15, 2024
@sdedic sdedic added JavaScript [ci] enable web job and extra JavaScript tests (webcommon/javascript2.editor) LSP [ci] enable Language Server Protocol tests Upgrade Library Library (Dependency) Upgrade ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Apr 15, 2024
@sdedic sdedic marked this pull request as ready for review April 15, 2024 10:32
Copy link
Contributor

@lahodaj lahodaj left a 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.

Copy link
Contributor

@MartinBalin MartinBalin left a 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.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a 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.

Comment on lines 85 to 114
<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>
Copy link
Contributor

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.

Copy link
Member Author

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

@lahodaj
Copy link
Contributor

lahodaj commented Apr 16, 2024

* the `.sig` files show class removals. Is it intentional, that the release major version is not bumped?

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?

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Apr 16, 2024

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?

@jtulach
Copy link
Contributor

jtulach commented Apr 16, 2024

... for 3rd party libraries, I don't think that's done always...

It is hard to enforce 100% backward compatibility rules on 3rd party libraries.

the question is how many people we expect will run into problems...

Once I analyzed range dependencies and claimed that by the size of the range one can see how much a library is compatible

The stability of an API in complete range repository is ultimately decided by users of the API. In case the API remains compatible for many versions, the dependency ranges on such API will be wider.

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.

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

@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 org.graalvm.nativeimage.RuntimeOptions$OptionClass has been removed; and then AbstractPolyglotImpl substantially changed. This second one is actually used in libs.truffle, that delivers its TrufflePolyglotImpl, which was and is a separate library in this case. Sadly we do not have a way how to separate APIs from SPIs -- especially if as in here, the 3rd party's impl package mixes SPIs and real implementations.

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.

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

I've done yet another experiment ... make libs.truffleapi implementation-dependent on libs.graalsdk and remove polyglot.impl from its public packages -- as this should be a 'semi-private' SPI between truffle and polyglot. Seems to work, but does not solve the issue - the harm of exposing half-implementation package has been already done.

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 ...

@neilcsmith-net
Copy link
Member

@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 24.0.0 here?

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

@neilcsmith-net:

Using spec.version.base AND non-numeric implementation-dependency

BUILD FAILED
/space/src/nb/apache/github/netbeans/nbbuild/templates/common.xml:262: /space/src/nb/apache/github/netbeans/ide/libs.graalsdk/manifest.mf: use of spec.version.base with non-integer OpenIDE-Module-Implementation-Version
(see http://wiki.netbeans.org/DevFaqImplementationDependency)

Not using spec.version.base:

BUILD FAILED
/space/src/nb/apache/github/netbeans/nbbuild/templates/common.xml:262: /space/src/nb/apache/github/netbeans/ide/libs.graalsdk/manifest.mf: not using spec.version.base, yet declaring implementation version; may lead to problems with Auto Update
(see http://wiki.netbeans.org/DevFaqImplementationDependency)

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

Waiting for #7245 to complete tests and be merged; then I consolidate commits here & rebase to prepare this PR for final merge, if approved.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Apr 16, 2024

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!)

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

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 ...

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a 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.

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2024

Consolidated commits, rebased on latest master.

Copy link
Member

@mbien mbien left a 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.

Comment on lines -2635 to +2628
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 }}
Copy link
Member

@mbien mbien Apr 16, 2024

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)).

@sdedic
Copy link
Member Author

sdedic commented Apr 17, 2024

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.

@sdedic sdedic merged commit 88d9395 into apache:master Apr 17, 2024
43 checks passed
@mbien mbien removed do not merge Don't merge this PR, it is not ready or just demonstration purposes. Need Squashing labels Apr 17, 2024
@neilcsmith-net
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) GraalVM [ci] enable GraalVM tests JavaScript [ci] enable web job and extra JavaScript tests (webcommon/javascript2.editor) LSP [ci] enable Language Server Protocol tests Platform [ci] enable platform tests (platform/*) Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetBeans bundled Truffle Version incompatbile with JDK 22
8 participants