Add base property auto upgrades instrumentation support #24632

asodja · 2023-04-04T12:41:51Z

Cherry picks property upgrades instrumentation from #24005.

It adds an option to use:

@UpgradedProperty

in our code, that can be upgraded to a Property<T>.

Currently it supports creating automatic interceptors, that means, that you can use:

@UpgradedProperty
abstract Property<String> getProperty()

and it will automatically generate interceptors for

String getProperty();
void setProperty(String value);

Optionally, you can also set originalType, that can be useful especially for primitive types:

@UpgradedProperty(originalType = int.class)
abstract Property<Integer> getProperty()

will generate interceptors for:

int getProperty();
void setProperty(int value);

Is base for: #24710.

…bilityCrossVersionSpec

…pter

…nd also do some other renaming

…rty and ConfigurableFileCollection

…ionSpec and remove Checkstyle from PropertyUpgradeInstrumentationRegistry

h0tk3y · 2023-04-12T13:35:07Z

I have reviewed the current set of changes. My only concern is non-deterministic ordering in the processor. FYI, some tests for ordering have been added in e716082, but those might not be enough. Other than that, LGTM!

subprojects/architecture-test/src/changes/archunit_store/internal-api-nullability.txt

subprojects/core/src/main/java/org/gradle/internal/classpath/InstrumentingTransformer.java

...ts/core/src/main/java/org/gradle/internal/classpath/declarations/InterceptorDeclaration.java

...ava/org/gradle/internal/classpath/declarations/stubs/ConfigurationCacheInterceptorsStub.java

.../java/org/gradle/internal/classpath/declarations/stubs/PropertyUpgradesInterceptorsStub.java

...ovy/org/gradle/integtests/AbstractPropertyUpgradesBinaryCompatibilityCrossVersionSpec.groovy

...Test/groovy/org/gradle/integtests/PropertyUpgradesBinaryCompatibilityCrossVersionSpec.groovy

...-api/src/main/java/org/gradle/internal/instrumentation/api/annotations/UpgradedProperty.java

...c/main/java/org/gradle/internal/instrumentation/api/annotations/VisitForInstrumentation.java

…stions

…h.generated

…view suggestions

…trumentation

h0tk3y · 2023-04-19T13:05:26Z

subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java

            Class<?> generatedClass = Class.forName(className);
            Method callInterceptorsGetter = generatedClass.getDeclaredMethod("getCallInterceptors");
+            // Generated classes are not public and in the org.gradle.internal.classpath.generated package
+            callInterceptorsGetter.setAccessible(true);


Would making the generated classes public turn out cleaner? Also, I would like to reference them from tests at some point.

Yes, that will make it cleaner. So I can just make them public. And if we need to forbid package access in the future, we can move that method to some org.gradle.internal.classpath.generated.InterceptorFactory class

Classes are now public

h0tk3y

👍 I don't think any changes are strictly necessary, but please take a look at the suggestions.

lptr · 2023-04-19T14:29:51Z

subprojects/instrumentation-declarations/build.gradle.kts

+
+tasks.named<JavaCompile>("compileJava") {
+    // Without this, javac will complain about unclaimed annotations
+    options.compilerArgs.add("-Xlint:-processing")


❔ Which annotations does it complain about and why?

Looks like the org.gradle.api.NonNullApi. You get this kind of warning, that breaks the build since we have -Werror:

warning: No processor claimed any of these annotations: org.gradle.api.NonNullApi error: warnings found and -Werror specified 1 error 1 warning

I guess we would need to try fix that at one point

Added annotation info to the comment

If @alllex changed the default for non-nullable APIs, we could lose the annotation from our packages, and then this problem would go away.

So they can be accessed outside of the org.gradle.internal.classpath.generated package

lptr

LGTM.

asodja · 2023-04-25T08:46:02Z

@bot-gradle test this

bot-gradle · 2023-04-25T08:46:14Z

I've triggered the following builds for you:

PullRequestFeedback build

asodja · 2023-04-25T09:43:16Z

@bot-gradle test and merge

bot-gradle · 2023-04-25T09:43:23Z

Your PR is queued. See the queue page for details.

asodja requested a review from a team April 4, 2023 12:42

asodja self-assigned this Apr 4, 2023

asodja added the @execution label Apr 4, 2023

asodja force-pushed the asodja/property-upgrades-cherry branch 5 times, most recently from bf8f6cb to 94a9df3 Compare April 6, 2023 09:41

asodja added 14 commits April 6, 2023 14:47

Add initial cross version tests for Property upgrades

95d0890

Initial generation of implementation class for upgraded properties

63ccfdb

Use Gradle 8.0.2 as a target version in PropertyUpgradesBinaryCompati…

b90d0b8

…bilityCrossVersionSpec

Fix generating setter and add static modifier to generated methods

af7f735

Rename generated class for upgraded properties to <original name>_Ada…

52e94a4

…pter

Small code style polishing of UpgradePropertyInstrumentationExtension

f130070

Split logic for code generation for property upgrade to multiple classes

e644353

Rename code-quality-instrumentation to instrumentation-declarations a…

67fb0b5

…nd also do some other renaming

Some more polishing of Property upgrades code generation logic

2f0ddbf

Generate also Groovy interceptor for UpgradedProperty annotation

1352220

Extract original type from generics for Property upgrades

1146fe1

Add com.google.testing.compile:compile-testing dependency

4508a60

Add some tests for Property upgrade generated code logic

5fcf370

Fix auto upgrading properties for RegularFileProperty, DirectoryPrope…

fdcb2dc

…rty and ConfigurableFileCollection

asodja force-pushed the asodja/property-upgrades-cherry branch 6 times, most recently from 5bbb2a6 to fc297cb Compare April 11, 2023 09:59

asodja added 2 commits April 11, 2023 13:56

Add a test for a Kotlin plugin using Checkstyle with property upgrades

0909a07

Add NotYetImplemented to PropertyUpgradesBinaryCompatibilityCrossVers…

75b6685

…ionSpec and remove Checkstyle from PropertyUpgradeInstrumentationRegistry

lptr reviewed Apr 14, 2023

View reviewed changes

asodja added 5 commits April 17, 2023 17:15

Polish code for UpgradeProperties instrumentation: apply review sugge…

915514c

…stions

Move instumentation generated classes to org.gradle.internal.classpat…

2fdcf31

…h.generated

Polish code for UpgradeProperties instrumentation: apply additiona re…

7616830

…view suggestions

Rename generated classes for instrumentation and remove stubs

4b9eb52

Polish comments and reduce code duplication for property upgrades ins…

21dab8b

…trumentation

asodja force-pushed the asodja/property-upgrades-cherry branch from f442a95 to 21dab8b Compare April 18, 2023 11:28

asodja requested review from h0tk3y and lptr April 18, 2023 12:45

h0tk3y reviewed Apr 19, 2023

View reviewed changes

lptr reviewed Apr 19, 2023

View reviewed changes

asodja added 3 commits April 20, 2023 14:34

Additional polishing to property upgrades instrumentation

0ce655b

Generate interceptor declarations with public access

7ff8e6e

So they can be accessed outside of the org.gradle.internal.classpath.generated package

Use Stream in AbstractInstrumentationProcessor in more cases

0e45e93

gradle deleted a comment from asodja Apr 20, 2023

lptr approved these changes Apr 21, 2023

View reviewed changes

Merge branch 'master' into asodja/property-upgrades-cherry

16ee011

gradle deleted a comment from asodja Apr 21, 2023

Merge branch 'master' into asodja/property-upgrades-cherry

336cc95

gradle deleted a comment from asodja Apr 25, 2023

Add base property auto upgrades instrumentation support #24632

Add base property auto upgrades instrumentation support #24632

Uh oh!

Conversation

asodja commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h0tk3y commented Apr 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h0tk3y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lptr left a comment

Choose a reason for hiding this comment

Uh oh!

asodja commented Apr 25, 2023

Uh oh!

bot-gradle commented Apr 25, 2023

Uh oh!

asodja commented Apr 25, 2023

Uh oh!

bot-gradle commented Apr 25, 2023

Uh oh!

asodja commented Apr 4, 2023 •

edited

Loading