Copilot commented on code in PR #6587:
URL:
https://github.com/apache/incubator-kie-drools/pull/6587#discussion_r3231327629
##########
kie-memory-compiler/src/main/java/org/kie/memorycompiler/JavaCompiler.java:
##########
@@ -44,7 +44,7 @@ static JavaCompiler getCompiler() {
class CompilerHolder {
private static final JavaCompiler JAVA_COMPILER =
- System.getProperty("jboss.server.name") != null ?
+
"ECLIPSE".equalsIgnoreCase(System.getProperty(JavaConfiguration.JAVA_COMPILER_PROPERTY))
?
JavaCompiler.createEclipseCompiler() :
JavaCompiler.createNativeCompiler();
Review Comment:
The default compiler selection logic now checks only
`drools.dialect.java.compiler` for the literal value `ECLIPSE`, otherwise it
unconditionally uses the native compiler. This removes the previous automatic
ECJ selection on some containers and can cause runtime failures on JRE-only
environments unless the system property is set. If the intent is to change the
default, consider still auto-falling back to ECJ when the system compiler isn’t
available but the ECJ implementation is present.
##########
kie-memory-compiler/src/main/java/org/kie/memorycompiler/jdknative/NativeJavaCompiler.java:
##########
@@ -51,6 +52,7 @@
import java.util.Map;
import java.util.Set;
import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
Review Comment:
`java.util.jar.JarFile` is imported but never used. In Java this is a
compilation error (unused import), so the module won’t compile until it’s
removed.
This issue also appears in the following locations of the same file:
- line 89
- line 365
- line 475
##########
drools-compiler/src/main/java/org/drools/compiler/compiler/JavaDialectConfiguration.java:
##########
@@ -108,11 +108,11 @@ private CompilerType getDefaultCompiler() {
return CompilerType.ECLIPSE;
} else {
logger.error( "Drools config: unable to use the
drools.compiler property. Using default. It was set to:" + prop );
- return CompilerType.ECLIPSE;
+ return CompilerType.NATIVE;
}
Review Comment:
Changing the default from `hasEclipseCompiler() ? "ECLIPSE" : "NATIVE"` to
always defaulting to `"NATIVE"` is a breaking behavioral change: on JRE-only
runtimes (no `ToolProvider` compiler) but with `drools-ecj` on the classpath,
Drools will now pick NATIVE and fail unless the user explicitly sets
`drools.dialect.java.compiler=ECLIPSE`. Consider keeping the previous fallback
logic (or falling back to ECLIPSE when the system compiler is unavailable but
ECJ is present) to preserve existing deployments.
##########
kie-memory-compiler/src/test/java/org/kie/memorycompiler/jdknative/NativeJavaCompilerTest.java:
##########
@@ -40,15 +48,64 @@ 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
+ 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();
Review Comment:
`URLClassLoader` should be closed after use to avoid leaking file handles
(notably on Windows, where it can keep `target/classes` entries locked). Wrap
it in try-with-resources or explicitly close it at the end of the test.
--
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]
