Copilot commented on code in PR #6587:
URL:
https://github.com/apache/incubator-kie-drools/pull/6587#discussion_r3232453603
##########
kie-memory-compiler/src/main/java/org/kie/memorycompiler/jdknative/NativeJavaCompiler.java:
##########
@@ -87,6 +93,10 @@ public CompilationResult compile( String[] pResourcePaths,
DiagnosticCollector<JavaFileObject> diagnostics = new
DiagnosticCollector<>();
JavaCompiler compiler = getJavaCompiler();
+ if (pResourcePaths == null || pResourcePaths.length == 0) {
+ return new CompilationResult( new CompilationProblem[0] );
+ }
Review Comment:
This changes behavior for `pResourcePaths == null` from throwing (or failing
later) to silently succeeding. It also makes the existing test
`simulateJreWithException()` (which calls `compile(null, ...)`) ineffective
because `getJavaCompiler()` is never reached. Consider limiting the
early-return to the empty-array case only (length == 0), and keep `null`
behaving as an error/exception as before.
##########
kie-memory-compiler/src/test/java/org/kie/memorycompiler/jdknative/NativeJavaCompilerTest.java:
##########
@@ -40,15 +55,152 @@ public void simulateJreWithException() {
assertThatExceptionOfType(KieMemoryCompilerException.class).isThrownBy(() ->
compiler.compile(null, null, null, null, null));
}
-
+
+ @Test
+ public void emptySourcesShouldSucceed() {
+ NativeJavaCompiler compiler = new NativeJavaCompiler();
+ CompilationResult result = compiler.compile(new String[0],
null, null, null, compiler.createDefaultSettings());
+ assertThat(result.getErrors()).isEmpty();
+ }
+
+ /**
+ * Tests that NativeJavaCompiler can resolve classes from
directory-based
+ * classpath entries (not JARs). This exercises the processDirectory()
method
+ * which was added to fix kjar builds where kie-maven-plugin compiles
DRL
+ * referencing Java classes from target/classes.
+ *
+ * The test compiles source that references CompilationResult (a class
from
+ * this module's target/classes directory) using a URLClassLoader that
includes
+ * target/classes as a directory URL — without setting any classpath on
+ * JavaCompilerSettings (simulating the classic DRL compilation path).
+ */
+ @Test
+ public void compileWithDirectoryClasspath() throws Exception {
+ // target/classes contains compiled classes from this module as
a directory (not a JAR)
+ File targetClasses = new File("target/classes");
+ assertThat(targetClasses).isDirectory();
+
+ // Create a classloader with target/classes as a directory URL
Review Comment:
These tests hardcode `target/classes` via a relative path, which can be
brittle depending on the test runner working directory (IDE, Gradle, Maven
reactor, etc.). A more robust approach is to locate the output directory via a
known class’ code source (e.g., `CompilationResult.class`) or resource URL and
derive the directory from that.
##########
kie-memory-compiler/src/test/java/org/kie/memorycompiler/jdknative/NativeJavaCompilerTest.java:
##########
@@ -40,15 +55,152 @@ public void simulateJreWithException() {
assertThatExceptionOfType(KieMemoryCompilerException.class).isThrownBy(() ->
compiler.compile(null, null, null, null, null));
}
-
+
+ @Test
+ public void emptySourcesShouldSucceed() {
+ NativeJavaCompiler compiler = new NativeJavaCompiler();
+ CompilationResult result = compiler.compile(new String[0],
null, null, null, compiler.createDefaultSettings());
+ assertThat(result.getErrors()).isEmpty();
+ }
+
+ /**
+ * Tests that NativeJavaCompiler can resolve classes from
directory-based
+ * classpath entries (not JARs). This exercises the processDirectory()
method
+ * which was added to fix kjar builds where kie-maven-plugin compiles
DRL
+ * referencing Java classes from target/classes.
+ *
+ * The test compiles source that references CompilationResult (a class
from
+ * this module's target/classes directory) using a URLClassLoader that
includes
+ * target/classes as a directory URL — without setting any classpath on
+ * JavaCompilerSettings (simulating the classic DRL compilation path).
+ */
+ @Test
+ public void compileWithDirectoryClasspath() throws Exception {
+ // target/classes contains compiled classes from this module as
a directory (not a JAR)
+ File targetClasses = new File("target/classes");
+ assertThat(targetClasses).isDirectory();
+
+ // Create a classloader with target/classes as a directory URL
+ try (URLClassLoader classLoader = new URLClassLoader(
+ new URL[]{targetClasses.toURI().toURL()},
+ ClassLoader.getPlatformClassLoader()
+ )) {
+ // Source that references CompilationResult from the
directory-based classpath
+ String source =
+ "package
org.kie.memorycompiler.test;\n" +
+ "import
org.kie.memorycompiler.CompilationResult;\n" +
+ "public class DirectoryClasspathTest
{\n" +
+ " public boolean
hasErrors(CompilationResult result) {\n" +
+ " return
result.getErrors().length > 0;\n" +
+ " }\n" +
+ "}";
+
+ Map<String, byte[]> compiled =
KieMemoryCompiler.compileNoLoad(
+
Map.of("org.kie.memorycompiler.test.DirectoryClasspathTest", source),
+ classLoader
+ );
+
+
assertThat(compiled).containsKey("org.kie.memorycompiler.test.DirectoryClasspathTest");
+
assertThat(compiled.get("org.kie.memorycompiler.test.DirectoryClasspathTest")).isNotEmpty();
+ }
+ }
+
+ /**
+ * Tests that a directory classpath entry whose path contains
URL-encoded characters
+ * (a space here, encoded as %20) is correctly recognized as a
directory rather than
+ * falling through to JAR / VFS handling. The previous code used
URL#getFile() which
+ * leaves the path URL-encoded, so File.isDirectory() returned false on
disk paths
+ * with spaces or non-ASCII characters.
+ */
+ @Test
+ public void compileWithEncodedDirectoryClasspath() throws Exception {
+ Path sourceClass =
Paths.get("target/classes/org/kie/memorycompiler/CompilationResult.class");
+ assertThat(sourceClass).exists();
+
+ Path tempRoot = Paths.get("target", "encoded url classpath");
+ Path tempPackage = tempRoot.resolve("org/kie/memorycompiler");
+ Files.createDirectories(tempPackage);
+ Files.copy(sourceClass,
tempPackage.resolve("CompilationResult.class"),
StandardCopyOption.REPLACE_EXISTING);
Review Comment:
The test writes to a fixed location under `target/encoded url classpath`.
This can intermittently fail under parallel test execution (or repeated runs)
due to shared filesystem state. Prefer using a unique temporary directory
(e.g., JUnit `@TempDir` or `Files.createTempDirectory`) and clean up
automatically.
##########
kie-memory-compiler/src/main/java/org/kie/memorycompiler/jdknative/NativeJavaCompiler.java:
##########
@@ -357,14 +494,16 @@ private List<JavaFileObject>
findClassesInExternalJars(String packageName) {
List<JavaFileObject> result = new ArrayList<>();
while (urlEnumeration.hasMoreElements()) { // one URL for each
jar on the classpath that has the given package
Review Comment:
Previously this method’s logic strongly suggested it could return `null`
when no classes were found (it only initialized `result` when something was
discovered). Initializing `result` eagerly to an empty list can change
observable behavior for callers that distinguish `null` vs empty. Consider
restoring the prior contract (return `null` when nothing is found) or update
callers accordingly and make the return contract explicit.
##########
kie-maven-plugin/src/main/java/org/kie/maven/plugin/mojos/AbstractKieMojo.java:
##########
@@ -82,7 +82,7 @@ public abstract class AbstractKieMojo extends AbstractMojo {
@Parameter(required = true, defaultValue = "src/main/resources")
private File resourceFolder;
- @Parameter(property = "javaCompiler", defaultValue = "ecj")
+ @Parameter(property = "javaCompiler", defaultValue = "native")
Review Comment:
Changing the Maven plugin default from `ecj` to `native` is a behavior
change that can affect downstream builds (different compilation diagnostics,
supported language levels, and classpath handling). If this is intended, it
should be paired with clear user-facing documentation/release notes and
(ideally) plugin tests verifying both values (`ecj`/`native`) still behave as
expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
