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

For nb.org projects, use the correct nbjavac prepend for the internal (boot)classpath. #5174

Merged

Conversation

jlahoda
Copy link
Contributor

@jlahoda jlahoda commented Jan 1, 2023

When building the bootclasspath for APISupport (nb.org modules in particular), lookup the correct nbjavac API jar in the particular repository, and prepend it to the bootclasspath.


^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

@jlahoda jlahoda added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Jan 1, 2023
bootstrapAPIs.add(ext);
}
}
buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, bootstrapAPIs.stream().collect(Collectors.joining(File.pathSeparator)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more compact to say
String path = Stream.of(javacLibrary.getClassPathExtensions().split(Pattern.quote(File.pathSeparator))).
filter(ext -> ext.endsWith("-api.jar")).
collect(Collectors.joining(File.pathSeparator)));
buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, path);

or even extract path calculation into a private method with meaningful name

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.

If I understand the idea correctly, the indention is, that code completion and type resolution of the IDE will work correctly for projects using the bundled javac. This is very good @jlahoda. However I checked java.source.base module and noticed, that only a subset of problems is gone. The module uses classes from the com.sun.tools.javac.tree package, which is in nb-javac-jdk-19+33.jar. I tested with this change and all problems are gone:

--- a/apisupport/apisupport.ant/src/org/netbeans/modules/apisupport/project/Evaluator.java
+++ b/apisupport/apisupport.ant/src/org/netbeans/modules/apisupport/project/Evaluator.java
@@ -426,15 +426,9 @@
             if (type == NbModuleType.NETBEANS_ORG && "true".equals(projectProperties.getProperties().get("requires.nb.javac"))) {
                 ModuleEntry javacLibrary = ml.getEntry("org.netbeans.libs.javacapi");
                 if (javacLibrary != null) {
-                    List<String> bootstrapAPIs = new ArrayList<>();
-                    for (String ext : javacLibrary.getClassPathExtensions().split(Pattern.quote(File.pathSeparator))) {
-                        if (ext.endsWith("-api.jar")) {
-                            bootstrapAPIs.add(ext);
+                    buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, javacLibrary.getClassPathExtensions());
                         }
                     }
-                    buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, bootstrapAPIs.stream().collect(Collectors.joining(File.pathSeparator)));
-                }
-            }
             
             baseEval = PropertyUtils.sequentialPropertyEvaluator(predefs, providers.toArray(new PropertyProvider[providers.size()]));

@jlahoda
Copy link
Contributor Author

jlahoda commented Jan 1, 2023

If I understand the idea correctly, the indention is, that code completion and type resolution of the IDE will work correctly for projects using the bundled javac. This is very good @jlahoda. However I checked java.source.base module and noticed, that only a subset of problems is gone. The module uses classes from the com.sun.tools.javac.tree package, which is in nb-javac-jdk-19+33.jar. I tested with this change and all problems are gone:

--- a/apisupport/apisupport.ant/src/org/netbeans/modules/apisupport/project/Evaluator.java
+++ b/apisupport/apisupport.ant/src/org/netbeans/modules/apisupport/project/Evaluator.java
@@ -426,15 +426,9 @@
             if (type == NbModuleType.NETBEANS_ORG && "true".equals(projectProperties.getProperties().get("requires.nb.javac"))) {
                 ModuleEntry javacLibrary = ml.getEntry("org.netbeans.libs.javacapi");
                 if (javacLibrary != null) {
-                    List<String> bootstrapAPIs = new ArrayList<>();
-                    for (String ext : javacLibrary.getClassPathExtensions().split(Pattern.quote(File.pathSeparator))) {
-                        if (ext.endsWith("-api.jar")) {
-                            bootstrapAPIs.add(ext);
+                    buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, javacLibrary.getClassPathExtensions());
                         }
                     }
-                    buildDefaults.put(ClassPathProviderImpl.BOOTCLASSPATH_PREPEND, bootstrapAPIs.stream().collect(Collectors.joining(File.pathSeparator)));
-                }
-            }
             
             baseEval = PropertyUtils.sequentialPropertyEvaluator(predefs, providers.toArray(new PropertyProvider[providers.size()]));

That's what I don't want to do blindly - that would mean that any module could seemingly use javac internals, which does not sound right. (There are many limitations on this, so some modules have an impl. dependency even though they should not, the impl might be partially visible in the editor anyway, and the build also uses everything including the impl. OTOH, runtime is strict, and I don't think we should make the situation worse.) But, we may be able to parse the javac dependency, and if it is an impl. dependency, use even the implementation. (Of course, the very correct way would be to only use exported packages, but we don't do that in the IDE for anything.) Please see 441cd88.

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.

Approach looks good to me and seems to work. Thank you!

@mbien mbien added this to the NB17 milestone Jan 4, 2023
@jtulach jtulach self-requested a review January 8, 2023 09:53
Copy link
Contributor

@jtulach jtulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to get this fixed. Otherwise working with javac sources is often a bit off for me.

@matthiasblaesing
Copy link
Contributor

Lets get this in.

@matthiasblaesing matthiasblaesing merged commit 8e1e1dd into apache:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants