Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix class loading of classes that have property upgrades #25893

Closed
asodja opened this issue Jul 25, 2023 · 2 comments · Fixed by #26015
Closed

Fix class loading of classes that have property upgrades #25893

asodja opened this issue Jul 25, 2023 · 2 comments · Fixed by #26015
Assignees
Labels
a:chore Minor issue without significant impact in:api-evolution Bytecode upgrades, multi-gradle-version support in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty p:lazy-migration Issues covered by migration to an all-lazy API
Milestone

Comments

@asodja
Copy link
Member

asodja commented Jul 25, 2023

We should be able to add instrumented projects as compileOnly dependencies of :instrumentation-declarations project, but that results in NoClassDefFoundError at runtime for upgraded types.

Let's figure out why that happens.

@asodja asodja added @execution p:lazy-migration Issues covered by migration to an all-lazy API labels Jul 25, 2023
@asodja asodja changed the title Fix classpath loading of classes that has properties upgrades Fix classpath loading of classes that have property upgrades Jul 25, 2023
@asodja asodja changed the title Fix classpath loading of classes that have property upgrades Fix class loading of classes that have property upgrades Jul 25, 2023
@asodja
Copy link
Member Author

asodja commented Jul 26, 2023

Reproducer:
https://github.com/gradle/gradle/compare/asodja/reproduce-classpath-issue?expand=1

Run any test in PropertyUpgradesBinaryCompatibilityCrossVersionSpec, you will see error like:

Caused by: org.gradle.api.internal.tasks.DefaultTaskContainer$TaskCreationException: Could not create task ':myCheckstyle'.
	at org.gradle.api.internal.tasks.DefaultTaskContainer.taskCreationException(DefaultTaskContainer.java:721)
Caused by: java.lang.NoClassDefFoundError: org/gradle/api/plugins/quality/Checkstyle
	at org.gradle.internal.classpath.generated.Checkstyle_Adapter.access_set_maxErrors(Checkstyle_Adapter.java:11)

Changing this line:

compileOnly(project(":code-quality"))

to implementation(project(":code-quality")) should fix the problem. But that change should not be necessary.

@asodja
Copy link
Member Author

asodja commented Jul 26, 2023

Checkstyle_Adapter is loaded from VisitableURLClassLoader(ant-and-gradle-loader) and we then need also Checkstyle there.
While if we use compileOnly(project(":code-quality")) Checkstyle is loaded from MixInLegacyTypesClassLoader(legacy-mixin-loader).

So one solution, that might work for Java and Kotlin is, that we separate instrumentation code and adapters. So we would have one jar with classes like InterceptorDeclaration_PropertyUpgradesJvmBytecode and another one with actuall logic. Not sure if that works for Groovy.

@asodja asodja self-assigned this Aug 4, 2023
@ov7a ov7a added a:chore Minor issue without significant impact a:investigation Issues requiring decision or investigation and removed a:investigation Issues requiring decision or investigation labels Aug 10, 2023
@eskatos eskatos added in:api-evolution Bytecode upgrades, multi-gradle-version support in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty and removed in:classloading labels Aug 10, 2023
@asodja asodja added this to the 8.4 RC1 milestone Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:chore Minor issue without significant impact in:api-evolution Bytecode upgrades, multi-gradle-version support in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty p:lazy-migration Issues covered by migration to an all-lazy API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants