Support for requires.nb.javac modules with javac.source/target > 8.

[lahodaj]

Currently, when a module sets requires.nb.javac, it cannot have javac.source/javac.target > 8, and the use of --release is disabled for it. This is because the requires.nb.javac is implemented using -Xbootclasspath/p:, which is incompatible with both javac.source > 8 and --release.

As NetBeans now practically only supports JDK 17 as the build/runtime JDK, this is becoming increasingly troublesome. JDK 17 includes things like Text Blocks, which would be immensely useful for writing tests, but we cannot use that because javac.source is stuck on 8 for the javac-based modules.

There are multiple ways out of this, but the one in this patch is that we run the compilation with --limit-modules setup in such a way that java.compiler and jdk.compiler are disabled. The correct javac is then pulled from the classpath as any ordinary library. This is a bit tricky, as it means we need to list modules that should be enabled. This is achieved using a custom Ant task, which may run a probe on the target JDK, determining modules that are neither java.compiler nor jdk.compiler, and do not depend either of these.

Some alternatives:

• using --patch-module, and replace the content of the module (which may be tricky, as this does not remove the all module content)
• having more precise dependencies on JDK modules inside project.xml, and specifically setup the system module paths to only include these modules. This would work, but is a lot of more work. So, while we may do that in the long run, the patch here still seems reasonable for now to me.

There is an obvious relation to #7188 - either this or that patch will need tweaks to accommodate changes from the other. I am completely fine with doing that here, assuming there's a reasonable timeline.

[mbien]

sounds like this would fix #6817? awesome! Thanks for looking into this.

[mbien]

I like this solution. It has a very small footprint and is only active for modules which request nb-javac.

As mentioned in the linked issue I investigated a bit the option of making a java module out of nb-javac and patch the jdk with it, which sounded like the closest thing to a boot classpath, but this approach is more elegant.

[mbien]

is the input stream intentionally left open?

[ebarboni]

make sense for NB22 ?

[mbien]

I suggest the following merge order:
NB 22:

• NB Build: Basic support for javac.release property. #7188

NB 23:

• Use javac release option where possible #7247 (or some similar PR)
• Support for requires.nb.javac modules with javac.source/target > 8. #7201 (this PR)

This gives devs a chance to update to NB 22 before starting to use the option (since the build would work but NB would break since it would load everything with 1.4 lang level).

cc @matthiasblaesing @neilcsmith-net

[lahodaj]

FWIW - I've merged with master (to get javac.release), and tried adjust this patch for that:
a7e81d3

Also tried to fix apisupport.ant to support the --limit-modules. The effective way is that is asks nb-javac about modules for the target source level, which also includes the data for --release, and hence this is to a degree independent on the runtime platform. This is:
3e88fb0

Please let me know what you think!

[mbien]

the diff below would make the tests pass. It moves the tests to the build-tools job which is JDK 17 and disables some. Feel free to apply to your changes if you want.

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 8c1fcf4..ac9f4ec 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -922,6 +922,10 @@
         if: ${{ matrix.java == '17' }}
         run: ant $OPTS -f extide/o.apache.tools.ant.module test
 
+      - name: apisupport.ant
+        if: ${{ matrix.java == '17' }}
+        run: ant $OPTS -f apisupport/apisupport.ant test
+
       - name: Create Test Summary
         uses: test-summary/action@v2
         if: failure()
@@ -1544,9 +1548,6 @@
       - name: Extract
         run: tar --zstd -xf build.tar.zst
 
-      - name: apisupport.ant
-        run: ant $OPTS -f apisupport/apisupport.ant test
-
       - name: apisupport.project
         run: ant $OPTS -f apisupport/apisupport.project test
 
diff --git a/apisupport/apisupport.ant/nbproject/project.properties b/apisupport/apisupport.ant/nbproject/project.properties
index 03ab15a..eb26886 100644
--- a/apisupport/apisupport.ant/nbproject/project.properties
+++ b/apisupport/apisupport.ant/nbproject/project.properties
@@ -21,39 +21,16 @@
 
 requires.nb.javac=true
 javac.compilerargs=-Xlint -Xlint:-serial
-javac.source=1.8
-javac.release=17
+javac.source=11
+javac.target=11
 javadoc.arch=${basedir}/arch.xml
 
 test-unit-sys-prop.test.nbroot=${nb_all}
 test-unit-sys-prop.test.netbeans.dest.dir=${netbeans.dest.dir}
 test-unit-sys-prop.java.awt.headless=true
 
-test.config.stableBTD.includes=**/*Test.class
-test.config.stableBTD.excludes=\
-    **/AccessibilityQueryImplTest.class,\
-    **/AntArtifactProviderImplTest.class,\
-    **/ApisupportAntUtilsTest.class,\
-    **/AvoidModuleListInProjectConstructorTest.class,\
-    **/BrandingSupportTest.class,\
-    **/BuildZipDistributionTest.class,\
-    **/ClassPathProviderImplTest.class,\
-    **/CustomizerLibrariesTest.class,\
-    **/EvaluatorTest.class,\
-    **/GlobalJavadocForBinaryImplTest.class,\
-    **/GlobalSourceForBinaryImplTest.class,\
-    **/Issue167725DeadlockTest.class,\
-    **/JavadocForBinaryImplTest.class,\
-    **/ModuleActionsTest.class,\
-    **/ModuleListTest.class,\
-    **/ModuleTypePanelTest.class,\
-    **/NbModuleProjectTest.class,\
-    **/ProjectXMLManagerTest.class,\
-    **/SingleModulePropertiesTest.class,\
-    **/SourceForBinaryImplTest.class,\
-    **/SourceLevelQueryImplTest.class,\
-    **/SubprojectProviderImplTest.class,\
-    **/SuiteOperationsTest.class,\
-    **/TestEntryTest.class,\
-    **/UnitTestForSourceQueryImplTest.class,\
-    **/UpdateTrackingFileOwnerQueryTest.class
+test.config.default.includes=**/*Test.class
+test.config.default.excludes=\
+    **/ExternalBuildDirTest.class,\
+    **/UseHtml4JavaTest.class
+
diff --git a/apisupport/apisupport.ant/test/unit/src/org/netbeans/modules/apisupport/project/CompilationDependencyTest.java b/apisupport/apisupport.ant/test/unit/src/org/netbeans/modules/apisupport/project/CompilationDependencyTest.java
index 597b38d..ef778b0 100644
--- a/apisupport/apisupport.ant/test/unit/src/org/netbeans/modules/apisupport/project/CompilationDependencyTest.java
+++ b/apisupport/apisupport.ant/test/unit/src/org/netbeans/modules/apisupport/project/CompilationDependencyTest.java
@@ -88,8 +88,9 @@
         assertFalse("Successfully compiled when is invalid specification version",
                 testingProject.getModuleJarLocation().exists());
     }
-    
-    public void testCompileAgainstPublicPackage() throws Exception {
+
+    // TODO fixme
+    public void fails_on_11_testCompileAgainstPublicPackage() throws Exception {
         NbModuleProject testingProject = TestBase.generateStandaloneModule(getWorkDir(), "testing");
         testingProject.open();
         FileObject buildScript = findBuildXml(testingProject);

[lahodaj]

the diff below would make the tests pass. It moves the tests to the build-tools job which is JDK 17 and disables some. Feel free to apply to your changes if you want.

Thanks! I've applied your patch, except for the change in source/target for apisupport.ant:
ea4246c
Lets see is tests will pass.

[mbien]

@lahodaj although I do like the changes here, I am a bit worried that this could cause unexpected issues. Would it make more sense to merge this early in the NB 23 dev cycle rather than at the end of NB 22?

One nitpick I have is that I am no fan of duplicated code in the repo, we might find a way to copy SetupLimitModulesProbe.java around before merging this.

[mbien]

Hi @lahodaj and reviewers,

I tested this by rebasing this PR on latest master which contains the other javac.release changes (#7247), and then adding another commit which sets javac.release to all modules which set OpenIDE-Module-Java-Dependencies in their manifest. I did also open all involved modules with NB 22 and checked if the editor is picking everything up properly (verifies that #7188 is working).

tests are currently running at https://github.com/mbien/netbeans/actions/runs/9233121350?pr=1

since my branch is already rebased and squashed, I could force push this into this PR if you want and we take another quick look and merge as two commits?

https://github.com/mbien/netbeans/commits/java-modules-using-jdk9-plus/

• mbien@646f89d this PR, rebased and squashed
• mbien@af7798d additional commit which sets javac.release

let me know what to do.

[mbien]

I have another change to offer. We could now remove the JDK manifest entries entirely, since the build adds them by looking at the javac.release property if they aren't there yet.

• mbien@8db6aab

This would simplify the module configuration to a single value in project.properties.

to validate this I diffed the manifest dump of all jars before/after a rebuild using this script:

find nbbuild/netbeans/ -type f -iname '*.jar' | sort | while read line || [[ -n $line ]];
do
    echo "$line/META-INF/MANIFEST.MF"
    unzip -p $line META-INF/MANIFEST.MF
done

checking all jars in nbbuild/netbeans/ should be sufficient, right? @matthiasblaesing

i could squash this into my other commit

[neilcsmith-net]

@mbien +1 to removing those that match. There can be times where they deliberately don't match and those should probably be reviewed separately.

[mbien]

+1 to removing those that match. There can be times where they deliberately don't match and those should probably be reviewed separately.

well, as you can see in mbien@af7798d and mbien@8db6aab, I bumped javac.release to the value of OpenIDE-Module-Java-Dependencies. There was luckily no instance where lang level was > manifest attribute

the reason it wasn't synced in the first place was likely due to #6817 which this PR would fix.

[neilcsmith-net]

There was luckily no instance where lang level was > manifest attribute

That scenario would fail CI tests. I saw your release changes. But there are a few that specify a higher JDK requirement and use reflection to compile with a lower language level. eg. External processes jdk9 shouldn't just have this done, it should be rewritten to use the APIs directly, and folded back into the main module. That's why I asked if we could split out and review those few separately?

[mbien]

...That's why I asked if we could split out and review those few separately?

sure. I also just realized while jogging that I broke gradle again since i reverted my own revert (#7367) with this by setting release to 17 in the first commit 🤦

• mbien@b31e776 reverts
• branch diff master...mbien:netbeans:java-modules-using-jdk9-plus

[lahodaj]

I was getting ready to ask whether we can integrate this, so thanks for being quicker. I am +1 on integrating this, and +1 on removing obvious unnecessary manifest entries. Keeping those that are not obvious separate would surely be good.

Thanks for working on this!

[mbien]

awesome! Squashed and pushed. Your changes are the first commit (unmodified, just squashed), the javac.release/manifest updates are in the second commit.

Planning to merge today, if anyone still wants to look through it - there is still time to make changes.

[mbien]

all green -> merging