Repository navigation
[WIP] Include existing output directories on the compiler classpath - #1127
goutamadwant wants to merge 3 commits into
Conversation
Expose earlier compiler output during main and test compilation and add mixed-language and forked regression coverage. Related to apache#1036. This is an incomplete comparison for review. Broader validation demonstrates stale Java bytecode can satisfy references after sources or declarations are removed. Output ownership and cleanup must be addressed before this change can be merged. The full integration suite is not passing.
Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
The direction is right — putting outputDirectory on the classpath unconditionally for non-modular builds is the correct fix for the Kotlin→Java cross-compilation problem. Two observations beyond what @desruisseaux already noted.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| Path.of(System.getProperty("java.home"), "bin", "javac").toString(); | ||
| } | ||
| Path output = Files.createDirectories(mojo.getOutputDirectory()); | ||
| String helperName = mojo instanceof TestCompilerMojo ? "TestHelper" : "MainHelper"; | ||
| Path helper = mojo.basedir.resolve(helperName + ".java"); | ||
| Files.writeString( | ||
| helper, "public class " + helperName + " { public static String value() { return \"existing\"; } }"); | ||
| assertEquals( | ||
| 0, | ||
| ToolProvider.getSystemJavaCompiler().run(null, null, null, "-d", output.toString(), helper.toString())); | ||
| Files.delete(helper); | ||
| Files.deleteIfExists(output.resolve("Consumer.class")); | ||
| mojo.execute(); | ||
| assertTrue(Files.isRegularFile(output.resolve(helperName + ".class"))); | ||
| assertTrue(Files.isRegularFile(output.resolve("Consumer.class"))); | ||
| } | ||
|
|
||
| @Provides | ||
| @Singleton | ||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
outputDirectory is on the classpath, but it does not cover the correctness regression the PR description warns about.
The regression case is: MainHelper.java is compiled in one build, then MainHelper.java is deleted, then the mojo runs a full clean build — MainHelper.class should be absent from the output (or the build should fail because Consumer.java can no longer resolve it). Without this test, the known stale-class regression ships untested.
Suggested additional test (schematic):
private static void compileWithDeletedSource(AbstractCompilerMojo mojo) throws Exception {
Path output = Files.createDirectories(mojo.getOutputDirectory());
// Step 1: pre-populate a stale class file (simulating a previous build artifact)
Path helper = mojo.basedir.resolve("StaleHelper.java");
Files.writeString(helper, "public class StaleHelper {}");
ToolProvider.getSystemJavaCompiler().run(null, null, null, "-d", output.toString(), helper.toString());
Files.delete(helper); // source is gone — class file is stale
// Step 2: run the mojo — Consumer.java does NOT reference StaleHelper
Files.deleteIfExists(output.resolve("Consumer.class"));
mojo.execute();
// Stale class MUST be removed or the build must fail
assertFalse("Stale StaleHelper.class should not survive a full rebuild",
Files.isRegularFile(output.resolve("StaleHelper.class")));
}This is exactly the scenario described in the PR body ("after removing a source or a secondary class declaration, a full rebuild can incorrectly succeed by resolving the old class file from the output directory"). If the regression is a known blocker that prevents merging, capturing it as a failing test would make the blocker explicit and trackable.
There was a problem hiding this comment.
Agreed that this needs a regression. I reproduced the deleted-source case with the existing mcompiler-21_class-remove integration test and a removed secondary top-level class case. The cleanup design still has ownership gaps, so I marked the PR Draft and described the cases needing maintainer direction here: #1127 (comment).
|
I reproduced the stale-class failure in A cleanup based on inferred source paths misses classes when the declared package differs from the source path. Scanning class files by @desruisseaux, would you favor persisting exact Java output ownership across embedded and forked compilation, with an explicit policy for builds whose prior ownership is unknown? |
Correct me if I'm wrong, but it seems to me that this is a request for better incremental compilation, isn't it? The current incremental compilation algorithms are known to have holes. This is not a regression compared to 3.x, which had the same holes. The above AI analysis give the impression that incremental compilation was the whole point of this pull request, while I think that the point was to allow a Java project to build when some classes have been compiled by Kotlin. I suggest to stick to that goal, in which case this pull request seems about complete to me, and defer better incremental compilation to later. We would need to do that for Java first, and then in a second step see how it could be applied to Kotlin too. |
+1 to your analysis. The goal of this PR is to fix the Kotlin+Java interop case — putting The stale-class scenario is pre-existing incremental compilation debt, not a regression introduced here. It only manifests on incremental builds without a prior |
|
We have one integration test failure:
|
The class-removal fixture declared package foo while keeping its Java files at the source root. Incremental cleanup maps source paths to output paths, so the fixture left foo/BeanA.class behind and the output-classpath change exposed that stale class. Place both sources under foo and assert that the removed class file is deleted before checking the expected compilation failure.
|
I aligned the class-removal IT with its declared |
Related to #1036.
Draft for design review. A known correctness regression blocks merging this change.
The current compiler plugin omits the output directory from the classpath during full compilation. This prevents Java sources from resolving classes written earlier by Kotlin, in both main and test compilation.
This draft adds the output directory to the nonmodular classpath and covers main/test compilation with embedded and forked javac. The four native regressions fail on unchanged production code, and the reporter's mixed Kotlin/Java project passes with the initial implementation.
Broader validation exposed stale Java bytecode reuse: after removing a source or a secondary class declaration, a full rebuild can incorrectly succeed by resolving the old class file from the output directory. The existing source-to-output cache cannot identify every javac-produced class. Reliable output ownership and cleanup are needed before enabling this behavior generally.
The annotation-processor fixture also needed to disable processing while compiling its own provider, whose service descriptor is copied before its class exists. This is restricted to the provider module; the consuming module still exercises annotation processing. Compiler 3.15 reproduces the same provider self-discovery failure.
Validation evidence:
clean verifypassed 20 tests on Java 17 and Java 21.Proposed next step: agree on compiler-output ownership tracking, including partial/full builds, failed compilations, cache migration, multiple executions and multi-release outputs, while preserving classes generated by earlier compilers.
mvn verifyvalidation after resolving the blocker.mvn -Prun-its verifypasses.