Fix for Incremental compilation with modules #23119

asodja · 2022-12-12T17:26:40Z

Fixes #23067 and also makes sure, that if incremental compilation after failure is disabled we don't change compile outputs. So in the case of some troubles, users can easily use old behavior.

This is a bit complicated fix. There are basically 2 problems we need to handle, because we change output location of javac:

External modules are not recognized when compiling incrementally
Packages inside the module we compile, but are not recompiled are not recognized by javac

Is solved by always compiling module-info.java
Is a bit harder. We modify GradleStandardJavaFileManager, so when javac tries to list files in CLASS_OUTPUT for module, we return also files in previous output location (normally build/classes/)

Tested also with the Elastic reproducer.

I also found a bug in snapshotting of manual module-path. If user sets --module-path=<modules> then we don't snapshot that path, but if user sets --module-path <modules> then we snapshot module path. The problem is, that we parse only the second option from the compile arguments. That is fixed now.

...gradle/java/compile/incremental/BaseIncrementalCompilationAfterFailureIntegrationTest.groovy

...rg/gradle/java/compile/incremental/CrossTaskIncrementalJavaCompilationIntegrationTest.groovy

lptr · 2022-12-15T12:06:38Z

...rg/gradle/java/compile/incremental/CrossTaskIncrementalJavaCompilationIntegrationTest.groovy

+        description               | modulePathParameter
+        "--module-path=<modules>" | '["--module-path=\${classpath.join(File.pathSeparator)}"]'
+        "--module-path <modules>" | '["--module-path", "\${classpath.join(File.pathSeparator)}"]'
+        "-p <modules>"            | '["-p", "\${classpath.join(File.pathSeparator)}"]'


🤔 Why do we need to test all these options? I think this test should use one of them.

Maybe add a separate test that checks if all these options do the same thing?

This is because of a bug we had, where we didn't parse --module-path=<modules> and -p correctly. And with that we snapshot module-path only when it is set in --module-path <modules> form. But this is actually a problem of DefaultJavaCompileSpec, so I moved test there and removed this where block.

...rg/gradle/java/compile/incremental/CrossTaskIncrementalJavaCompilationIntegrationTest.groovy

lptr · 2022-12-15T12:42:51Z

...rg/gradle/java/compile/incremental/CrossTaskIncrementalJavaCompilationIntegrationTest.groovy

+        "--module-path=<modules>" | '["--module-path=\${classpath.join(File.pathSeparator)}"]'
+        "--module-path <modules>" | '["--module-path", "\${classpath.join(File.pathSeparator)}"]'
+        "-p <modules>"            | '["-p", "\${classpath.join(File.pathSeparator)}"]'
+    }


🤔 Seems like we are duplicating the same test setup here thrice. Can we somehow reduce the redundancy?

Removed duplicated test and created one test that has two variants: "with inferred module-path" and "with manual module-path"

If you have in mind third test "incremental compilation works for multi-module project with manual module paths" -> this one is a bit different, here we actually have Java multi-module project: so multiple directories with module-info in one project.

lptr · 2022-12-15T12:46:23Z

.../gradle/api/internal/tasks/compile/incremental/recomp/AbstractRecompilationSpecProvider.java

@@ -42,6 +42,10 @@
 import java.util.Set;

 abstract class AbstractRecompilationSpecProvider implements RecompilationSpecProvider {
+
+    private static final String MODULE_INFO_CLASS = "module-info";
+    private static final String PACKAGE_INFO_CLASS = "package-info";


🤔 The _CLASS suffix is a bit misleading for me. How about MODULE_INFO_NAME etc. instead?

What do you think about _CLASS_NAME? I am thinking to use that, since we use these two for example when calling:

sourceFileClassNameConverter.getRelativeSourcePaths(MODULE_INFO_CLASS_NAME)

Where parameter of getRelativeSourcePaths is className:

Set<String> getRelativeSourcePaths(String className)

lptr · 2022-12-15T12:48:42Z

...rc/main/java/org/gradle/api/internal/tasks/compile/incremental/recomp/RecompilationSpec.java

@@ -29,6 +29,7 @@ public class RecompilationSpec {
    private final Collection<String> classesToProcess = new LinkedHashSet<>();
    private final Collection<GeneratedResource> resourcesToGenerate = new LinkedHashSet<>();
    private String fullRebuildCause;
+    private boolean isIncrementalCompilationOfJavaModule;


🤔 This complicated name is a code smell to me. It seems to combine two things: 1) whether recompilation was incremental, and 2) whether it includes a Java module file.

It seems to me that 1) is already captured by fillRebuildCause, no? If it's empty, we are doing an incremental compilation?

I wonder if we could capture this better. That said, I think it's fine to follow up in a separate PR if it makes merging this quickly easier.

There is some data duplication indeed. I removed this from recompileSpec and simplified it a bit. I left it in JavaCompileSpec for now, since it's needed later when compiling. If we separate these two informations, we would need to set isModule also in cases when it's not incremental. So it needs some more work as you said.

...n/java/org/gradle/api/internal/tasks/compile/incremental/transaction/CompileTransaction.java

...c/main/java/org/gradle/api/internal/tasks/compile/reflect/GradleStandardJavaFileManager.java

lptr · 2022-12-15T12:59:46Z

...n/java/org/gradle/api/internal/tasks/compile/incremental/transaction/CompileTransaction.java

+        if (isIncrementalCompilationAfterFailure()) {
+            stagedOutputs.forEach(StagedOutput::restoreSpecOutput);
+        }
+    }


🤔 The many ifs here are a code smell. I think this would be better if we extracted these methods to an interface, and used a no-op implementation for when incremental-after-failure is disabled.

I see what you mean. Yes, we could split that, that would makes sense. I would do that next time we visit that class maybe. Since we might need to do some other changes, like splitting tests. And I think it's still somehow manageable, since this is used just in these three methods.

What do you think?

This shows that there is an issue also for packages that are exported, but are not recompiled.

…but not recompiled We achieve that by returning also previous compiled classes when javac requests CLASS_OUTPUT files

…ure is disabled That way we keep previous behaviour and in case of problems, users can easily rollback.

Co-authored-by: Lóránt Pintér <lorant@gradle.com>

…t, also reduce tests duplication in CrossTaskIncrementalJavaCompilationIntegrationTest

…modules fix

asodja · 2022-12-19T07:26:05Z

@bot-gradle test and merge

bot-gradle · 2022-12-19T07:26:16Z

OK, I've already triggered a build for you.

Fix for modules in GradleStandardJavaFileManager is not needed anymore, since we don't change output directories

…lation after a failure Changing compile output folder to a temporary folder for incremental compilation after failure causes some issues with modules (#23067) and with annotation processors (#23066). But to restore outputs we can just do (not necessarly in the given order): 1. restore stale classes and resources that were deleted 2. remove all newly generated classes and resources 3. restore any overwritten classes (this happens in practice rarely, and only on the incremental compilation, e.g. if we have `B.java` with `class B` and we don't change that file and we create `B1.java` with `class B`). We already do 1) in the current implementation, for 2) we can use Compiler API data we generate for incremental annotation processors and for "source to class name" mapping. For step 3) I implemented a [CompilationClassBackupService.java](https://github.com/gradle/gradle/pull/23226/files#diff-21d9cec673f39ed2a74d60de1b8862dd6fa18e6eea1769209e8ec9fb47b53919) that is injected in the Compiler API and backups any `.class` file during compilation. Backup is done just for "normal compilation". Annotation processors are very limited so this is not needed. They are limited in a way that If two types generate one class/resource, the annotation processor can only be aggregating, for which we always regenerate class/resources and reprocess all types with aggregating annotation. Since we track types also for Groovy compilation, the same method for restoring outputs works well also with Groovy and Groovy/Java joint compilation. --- Todo: - [x] When #23119 gets merged to master, revert changes done in [GradleStandardJavaFileManager](https://github.com/gradle/gradle/blob/5d8adb4002742924f7327db670eb8f2fffff9414/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/reflect/GradleStandardJavaFileManager.java#L100-L111). Co-authored-by: Anže Sodja <asodja@gradle.com>

Backport from #23119 to 7.6.1 Fixes #23224

…modules" to 7.6.1 Backport from #23119 to 7.6.1. Also backport from #23251 to 7.6.1 Fixes #23224 Co-authored-by: Anže Sodja <asodja@gradle.com>

asodja self-assigned this Dec 12, 2022

asodja added the @execution label Dec 12, 2022

asodja force-pushed the asodja/fix-incremental-modules branch from 56106bf to e046445 Compare December 13, 2022 13:36

asodja changed the base branch from master to release7x December 13, 2022 13:37

asodja force-pushed the asodja/fix-incremental-modules branch from e046445 to f7398e9 Compare December 13, 2022 13:42

asodja added this to the 7.6.1 milestone Dec 13, 2022

asodja linked an issue Dec 13, 2022 that may be closed by this pull request

Incremental compilation of java modules is broken with Gradle 7.6 #23067

Closed

asodja modified the milestones: 7.6.1, 8.0 RC1 Dec 14, 2022

asodja force-pushed the asodja/fix-incremental-modules branch 5 times, most recently from 8b272e9 to 2197739 Compare December 14, 2022 14:57

gradle deleted a comment from bot-gradle Dec 14, 2022

asodja force-pushed the asodja/fix-incremental-modules branch from ba66088 to d9257bb Compare December 14, 2022 17:18

gradle deleted a comment from asodja Dec 14, 2022

asodja changed the base branch from release7x to master December 15, 2022 09:40

asodja force-pushed the asodja/fix-incremental-modules branch from d9257bb to c284e9b Compare December 15, 2022 09:40

asodja changed the base branch from master to release7x December 15, 2022 09:41

asodja changed the base branch from release7x to master December 15, 2022 09:42

gradle deleted a comment from bot-gradle Dec 15, 2022

asodja force-pushed the asodja/fix-incremental-modules branch from c284e9b to c30c658 Compare December 15, 2022 09:43

asodja requested a review from a team December 15, 2022 10:51

asodja marked this pull request as ready for review December 15, 2022 10:51

asodja requested a review from a team as a code owner December 15, 2022 10:51

asodja requested a review from ghale December 15, 2022 10:51

lptr reviewed Dec 15, 2022

View reviewed changes

asodja mentioned this pull request Dec 16, 2022

Easier debugging of CompileTransaction path selection in incremental task execution #23066

Closed

asodja and others added 10 commits December 19, 2022 08:25

Improve reproducer for incremental compilation with manual module path

a228bc2

This shows that there is an issue also for packages that are exported, but are not recompiled.

Fix incremental compilation for modules where packages are exported, …

36bf8d7

…but not recompiled We achieve that by returning also previous compiled classes when javac requests CLASS_OUTPUT files

Add incremental compilation test also for inferred module path

eea6129

Small polish of GradleStandardJavaFileManage

ee102ee

Handle GStrings for manual module-path in DefaultJavaCompileSpec

46d80fa

Don't modify compiler outputs when incremental compilation after fail…

d3eff98

…ure is disabled That way we keep previous behaviour and in case of problems, users can easily rollback.

Add tests for incremental compilation after failure with modules

e564ab5

Fix copy/paste typos in test names for incremental compilation

ad7978b

Co-authored-by: Lóránt Pintér <lorant@gradle.com>

Move testing of parsing of --module-path to DefaultJavaCompileSpecTes…

702c6a6

…t, also reduce tests duplication in CrossTaskIncrementalJavaCompilationIntegrationTest

Fix production code review comments for Incremental compilation with …

5d8adb4

…modules fix

asodja force-pushed the asodja/fix-incremental-modules branch from 94564d2 to 5d8adb4 Compare December 19, 2022 07:25

asodja changed the base branch from master to release December 19, 2022 07:25

gradle deleted a comment from asodja Dec 19, 2022

bot-gradle merged commit 4517d39 into release Dec 19, 2022

This was referenced Dec 19, 2022

Backport to 7.6.1 "Fix for Incremental compilation with modules" #23224

Closed

Use Compiler API data for incremental compilation after a failure #23226

Merged

asodja added a commit that referenced this pull request Dec 20, 2022

Revert part of modules fix from #23119

21b39a8

Fix for modules in GradleStandardJavaFileManager is not needed anymore, since we don't change output directories

blindpirate deleted the asodja/fix-incremental-modules branch December 21, 2022 00:25

asodja added a commit that referenced this pull request Jan 9, 2023

Revert part of modules fix from #23119

479c3d1

Fix for modules in GradleStandardJavaFileManager is not needed anymore, since we don't change output directories

asodja added a commit that referenced this pull request Jan 10, 2023

Revert part of modules fix from #23119

37e0d70

Fix for modules in GradleStandardJavaFileManager is not needed anymore, since we don't change output directories

asodja mentioned this pull request Feb 15, 2023

Backport "Fix incremental compilation with modules" to 7.6.1 #23913

Merged

asodja added a commit that referenced this pull request Feb 16, 2023

Fix incremental compilation with modules

a650e73

Backport from #23119 to 7.6.1 Fixes #23224

lptr mentioned this pull request Feb 20, 2023

Backport "Use Compiler API data for incremental compilation after a failure" to 7.x #23922

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Incremental compilation with modules #23119

Fix for Incremental compilation with modules #23119

asodja commented Dec 12, 2022 •

edited

lptr Dec 15, 2022

asodja Dec 16, 2022 •

edited

lptr Dec 15, 2022

asodja Dec 16, 2022 •

edited

asodja Dec 16, 2022

lptr Dec 15, 2022

asodja Dec 16, 2022

lptr Dec 15, 2022

asodja Dec 16, 2022

lptr Dec 15, 2022

asodja Dec 16, 2022

asodja commented Dec 19, 2022

bot-gradle commented Dec 19, 2022

Fix for Incremental compilation with modules #23119

Fix for Incremental compilation with modules #23119

Conversation

asodja commented Dec 12, 2022 • edited

Choose a reason for hiding this comment

asodja Dec 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asodja Dec 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asodja commented Dec 19, 2022

bot-gradle commented Dec 19, 2022

asodja commented Dec 12, 2022 •

edited

asodja Dec 16, 2022 •

edited

asodja Dec 16, 2022 •

edited