netbeans icon indicating copy to clipboard operation
netbeans copied to clipboard

Upgrade nb-javac to JDK 25-ea

Open mbien opened this issue 6 months ago • 3 comments

preparations to upgrade to nb-javac based on JDK 25

this is not meant to be integrated yet, the wrapped nb-javac is an experimental build which was backported to JDK 17 instead of 8 to reduce the delta to upstream. It was built from this old branch I created a while ago. The actual nb-javac 25 is also not available yet.

mbien avatar Jun 09 '25 01:06 mbien

javadoc doesn't build on JDK 25, i saw that already in https://github.com/apache/netbeans/pull/8482 but hoped that it would work as soon we bump nb-javac (like it was the case in the past)

 /home/runner/work/netbeans/netbeans/nbbuild/javadoctools/template.xml:284: Warning: Could not find file /home/runner/work/netbeans/netbeans/nbbuild/build/javadoc/org-netbeans-api-annotations-common/resource-files/glass.png to copy.

see https://github.com/apache/netbeans/pull/8633 for javadoc fix

for hints LambdaTest and ConvertVarToExplicitTypeTest need looking at. java.completion test data needs updating most likely.

mbien avatar Jun 09 '25 02:06 mbien

FWIW, I've opened: https://github.com/JaroslavTulach/nb-javac/pull/30

lahodaj avatar Jun 16 '25 14:06 lahodaj

FWIW, I've opened: JaroslavTulach/nb-javac#30

@lahodaj thanks! I can refresh this PR once a staged build is available and drop the experimental nb-javac17 bits.

mbien avatar Jun 16 '25 21:06 mbien

switched to a temporary hosted build of nb-javac 25b29 until its available, the same tests are failing as before (when the experimental build was used)

mbien avatar Jul 03 '25 23:07 mbien

ComputeImportsTest failed due to CCE in javac Lint class. Likely due to https://github.com/openjdk/jdk/pull/22056, disabled test case since it influenced code scanning of other tests.

mbien avatar Jul 06 '25 03:07 mbien

ComputeImportsTest failed due to CCE in javac Lint class. Likely due to openjdk/jdk#22056, disabled test case since it influenced code scanning of other tests.

Thanks. FWIW, I've opened: https://github.com/openjdk/jdk/pull/26142

(The failure in LSP tests/ServerTest also seems to be caused by this.)

lahodaj avatar Jul 06 '25 07:07 lahodaj

will take a look at the (hopefully) last remaining failing test later (java.source.base).

update:

MRJARCachingFileManagerTest#testJavac is failing with:

java.lang.IllegalArgumentException: Path component should be '/'
Working directory: /home/mbien/NetBeansProjects/netbeans/java/java.source.base/build/test/unit/work/o.n.m.j.s.p.M/j
	at java.base/sun.nio.fs.UnixFileSystemProvider.checkUri(UnixFileSystemProvider.java:81)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newFileSystem(UnixFileSystemProvider.java:90)
	at java.base/java.nio.file.FileSystems.newFileSystem(FileSystems.java:339)
	at com.sun.tools.javac.file.JavacFileManager$ArchiveContainer.<init>(JavacFileManager.java:586)
	at com.sun.tools.javac.file.JavacFileManager.getContainer(JavacFileManager.java:337)
	at com.sun.tools.javac.file.JavacFileManager.pathsAndContainers(JavacFileManager.java:1101)
	at com.sun.tools.javac.file.JavacFileManager.indexPathsAndContainersByRelativeDirectory(JavacFileManager.java:1056)
	at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1220)
	at com.sun.tools.javac.file.JavacFileManager.pathsAndContainers(JavacFileManager.java:1044)
	at com.sun.tools.javac.file.JavacFileManager.list(JavacFileManager.java:792)
	at org.netbeans.modules.java.source.parsing.MRJARCachingFileManagerTest.testJavac(MRJARCachingFileManagerTest.java:176)

which is due to this new transformation within nb-javac:

java.nio.file.FileSystems.newFileSystem($path, $env, $classloader) :: $path instanceof java.nio.file.Path
=>
java.nio.file.FileSystems.newFileSystem($path.toUri(), $env, $classloader)
;;

the fascinating detail here is that the second factory method fails, since the uri checks are different from the path checks:

        // JDK 13+ API
        FileSystem fs1 = FileSystems.newFileSystem(
                Path.of("/mnt/ram/netbeans/java/java.source.base/build/test/unit/work/o.n.m.j.s.p.M/testJavac/mr.jar"),
                Map.of(),
                (ClassLoader)null
        );
        // JDK 8 API
        FileSystem fs2 = FileSystems.newFileSystem(
                Path.of("/mnt/ram/netbeans/java/java.source.base/build/test/unit/work/o.n.m.j.s.p.M/testJavac/mr.jar").toUri(),
                Map.of(),
                (ClassLoader)null
        );

first call creates a ZipFileSystem, I believe the second attempts to create a UnixFileSystem, given the call into UnixFileSystemProvider.

Javadoc makes also slightly different promises path variant, uri variant.

javac 24 worked since it didn't use the env param and used the path variant, essentially ending up doing something similar to the fs1 call.

Easiest fix would be to bump nb-javac to 17 to drop some of the transformations which aren't needed for NB anyway. But we would have to find a new maven namespace for that most likely.

Second option is to use the path+env variant on JDK 13+ via reflection, and the path (without env) variant on <13.

mbien avatar Jul 06 '25 15:07 mbien

Easiest fix would be to bump nb-javac to 17 to drop some of the transformations which aren't needed for NB anyway. But we would have to find a new maven namespace for that most likely.

This library exists to support NetBeans, so why is upgrading to JDK 17 an issue?! If it needs to move to new hosting and to a new Maven namespace to ensure it's properly responsive to our requirements, then let's work out how that happens. Perhaps a dev@ thread? I had previously mentioned to @geertjanw that FoAN would be a better home for this.

Of course, doing all that within the week left to get this required change in before freeze might be tricky! Still, this upgrade needs to be in NB27.

neilcsmith-net avatar Jul 07 '25 09:07 neilcsmith-net

taking a look at JavaRefactoringActionsProviderTest. Its the last test of refactoring.java which is failing

update: reason seems to be similar to https://github.com/apache/netbeans/pull/8572#discussion_r2187879041, the test case intentionally uses code with error (class declaration commented out) and javac 25 thinks it is a "compact java file" which leads to different results

testcase is named after 190101, unfortunately I can't find the issue outside of https://github.com/emilianbold/netbeans-releases/commit/dc3c8858a8b5a507227370ebd7968df9b6f1cc78

will likely turn the test off

mbien avatar Jul 07 '25 13:07 mbien

yey, all java tests green!

will rebase/squash to a reasonable amount of commits, remove merge commits and give co-author credits.

PR ready to review, all what is missing is a nb-javac release + updating artifact locations. (keeping it as draft so that nobody merges it by accident with the dependency pointing to my raspi)

mbien avatar Jul 07 '25 17:07 mbien

noticed during code scanner smoke test:

    public static void main(String[] args) {
        new WeakReference<>(null) {
            private Object hardRef = null;
        };
    }

will now cause a

modifier sealed not allowed here

javac error. WeakReference is non-sealed. Not sure if this is intended, but its not nb-specific.

mbien avatar Jul 07 '25 20:07 mbien

noticed during code scanner smoke test:

    public static void main(String[] args) {
        new WeakReference<>(null) {
            private Object hardRef = null;
        };
    }

will now cause a

modifier sealed not allowed here

javac error. WeakReference is non-sealed. Not sure if this is intended, but its not nb-specific.

Uhhhh. Thanks!!! Filled as: https://bugs.openjdk.org/browse/JDK-8361570 fix is coming.

lahodaj avatar Jul 08 '25 07:07 lahodaj

squashed into logical units, the idea is to revert or drop the last commit with the next nb-javac / JDK build.

mbien avatar Jul 08 '25 15:07 mbien

last showstopper is having no line numbers in stack traces

the emitted classes in nb-javac do seem to have a line number table (javap -v), so on first glance everything is ok (but I haven't diffed the javap outputs).

stack trace looks like:

Caused: java.lang.AssertionError: Unexpected tree: (ERROR) ? (ERROR) > entry : (ERROR) with kind: CONDITIONAL_EXPRESSION within: (ERROR) ? (ERROR) > entry : (ERROR) with kind: CONDITIONAL_EXPRESSION
	at com.sun.tools.javac.util.Assert.error(Unknown Source)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.validateAnnotatedType(Unknown Source)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitVarDef(Unknown Source)
	at com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept(Unknown Source)
	at com.sun.tools.javac.tree.TreeScanner.scan(Unknown Source)
	at com.sun.tools.javac.tree.TreeScanner.scan(Unknown Source)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitMethodDef(Unknown Source)
	at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(Unknown Source)
	at com.sun.tools.javac.tree.TreeScanner.scan(Unknown Source)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitClassDef(Unknown Source)
	at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(Unknown Source)
	at com.sun.tools.javac.comp.Attr.validateTypeAnnotations(Unknown Source)

building nb-javac without passing --enable-preview to frgaal does seem to work:

Caused: java.lang.AssertionError: Unexpected tree: (ERROR) ? (ERROR) > entry : (ERROR) with kind: CONDITIONAL_EXPRESSION within: (ERROR) ? (ERROR) > entry : (ERROR) with kind: CONDITIONAL_EXPRESSION
	at com.sun.tools.javac.util.Assert.error(Assert.java:162)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.validateAnnotatedType(Attr.java:5937)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitVarDef(Attr.java:5776)
	at com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept(JCTree.java:1066)
	at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:50)
	at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:58)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitMethodDef(Attr.java:5766)
	at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:960)
	at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:50)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitClassDef(Attr.java:5829)
	at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:859)
	at com.sun.tools.javac.comp.Attr.validateTypeAnnotations(Attr.java:5726)

other curiosity:

  • setting -g --enable-preview breaks the bytecode?
    [junit] java.lang.ClassFormatError: Illegal field name "" in class com/sun/tools/javac/comp/Check
    [junit] 	at com.sun.tools.javac.comp.Modules.<init>(Modules.java:195)
    [junit] 	at com.sun.tools.javac.comp.Modules.instance(Modules.java:183)
  • setting -g:lines --enable-preview produces traces without lines as shown above
  • setting -g does appear to work, and was the config we ran with so far

opened https://github.com/JaroslavTulach/nb-javac/pull/32

mbien avatar Jul 08 '25 18:07 mbien

there is now a nb-javac build based on 25b31 available

  • going to switch the coordinates to maven central
  • and drop https://github.com/mbien/netbeans/commit/60166d3fbd71594590c97191c3c7855a23833577

everything should be still green after that

mbien avatar Jul 12 '25 15:07 mbien

something is wrong, JDK-8361445 doesn't seem to be in the nb-javac build

2025-07-12T17:48:06.4357931Z     [junit] java.lang.ClassCastException: class com.sun.tools.javac.code.Attribute$Error cannot be cast to class com.sun.tools.javac.code.Attribute$Constant (com.sun.tools.javac.code.Attribute$Error and com.sun.tools.javac.code.Attribute$Constant are in unnamed module of loader 'app')
2025-07-12T17:48:06.4358120Z     [junit] 	at com.sun.tools.javac.code.Lint.suppressionsFrom(Lint.java:533)
2025-07-12T17:48:06.4358377Z     [junit] 	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
2025-07-12T17:48:06.4358628Z     [junit] 	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
2025-07-12T17:48:06.4358815Z     [junit] 	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
2025-07-12T17:48:06.4359109Z     [junit] 	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1845)
2025-07-12T17:48:06.4359359Z     [junit] 	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
2025-07-12T17:48:06.4359630Z     [junit] 	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
2025-07-12T17:48:06.4359897Z     [junit] 	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
2025-07-12T17:48:06.4360173Z     [junit] 	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
2025-07-12T17:48:06.4360416Z     [junit] 	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
2025-07-12T17:48:06.4360670Z     [junit] 	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
2025-07-12T17:48:06.4360852Z     [junit] 	at com.sun.tools.javac.code.Lint.suppressionsFrom(Lint.java:524)
2025-07-12T17:48:06.4361095Z     [junit] 	at com.sun.tools.javac.code.Lint.suppressionsFrom(Lint.java:511)
2025-07-12T17:48:06.4361246Z     [junit] 	at com.sun.tools.javac.code.Lint.augment(Lint.java:82)
2025-07-12T17:48:06.4361462Z     [junit] 	at com.sun.tools.javac.comp.Attr.attribClass(Attr.java:5414)

sources of the Lint class do also not seem to include the fix:

    // Given a @SuppressWarnings annotation, extract the recognized suppressions
    private EnumSet<LintCategory> suppressionsFrom(Attribute.Compound suppressWarnings) {
        EnumSet<LintCategory> result = LintCategory.newEmptySet();
        Attribute.Array values = (Attribute.Array)suppressWarnings.member(names.value);
        for (Attribute value : values.values) {
            Optional.of((String)((Attribute.Constant)value).value)

^^L 533

JDK diff shows that it should be in it https://github.com/openjdk/jdk/compare/jdk-25%2B29...jdk-25%2B31

edit: decompiled the class to double check and it seems to confirm it that the release isn't based on build 31:

$ java -jar cfr-0.152.jar /tmp/nb-javac-jdk-25+31/com/sun/tools/javac/code/Lint.class --methodname suppressionsFrom
...
private EnumSet<LintCategory> suppressionsFrom(Attribute.Compound suppressWarnings) {
    EnumSet<LintCategory> result = LintCategory.newEmptySet();
    Attribute.Array values = (Attribute.Array)suppressWarnings.member(this.names.value);
    for (Attribute value : values.values) {
        Optional.of((String)((Attribute.Constant)value).value).flatMap(LintCategory::get).filter(lc -> lc.annotationSuppression).ifPresent(result::add);
    }
    return result;
}

here the JDK commit for comparison (compare missing filter stage): https://github.com/openjdk/jdk/commit/21cb2acda0d70cc838dfa097d235e86338609e7e

mbien avatar Jul 12 '25 18:07 mbien

second attempt to switch to release artifacts (25+31.1)

mbien avatar Jul 14 '25 13:07 mbien

thanks a lot for the help @lahodaj and review! merging.

mbien avatar Jul 14 '25 15:07 mbien