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

Remove project from task extension #6038

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

Sineaggi
Copy link
Contributor

@Sineaggi Sineaggi commented May 31, 2024

In order to support Gradle's configuration cache, having the Project object in extensions/tasks is not allowed https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements:disallowed_types. We can avoid that by injecting the ProjectLayout object directly which is legal.

There are a number of types that task instances must not reference from their fields.
These types fall into some categories as follows:

  • Live JVM state types
  • Gradle model types
  • Dependency management types

In all cases the reason these types are disallowed is that their state cannot easily be stored or recreated by the configuration cache.

Specifically

Gradle model types (e.g. Gradle, Settings, Project, SourceSet, Configuration etc…​) are usually used to carry some task input that should be explicitly and precisely declared instead.

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about Gradle plugins, but looks good to me.

cc. @manusa

@rohanKanojia
Copy link
Member

@Sineaggi : Could you please fix style errors with mvn spotless:apply?

@Sineaggi Sineaggi force-pushed the remote-project-from-task branch from dd353f7 to cbade85 Compare May 31, 2024 16:48
@rohanKanojia
Copy link
Member

@Sineaggi : Do you know if this configuration cache requirement was added since Gradle 8.x ?

@Sineaggi
Copy link
Contributor Author

@Sineaggi : Do you know if this configuration cache requirement was added since Gradle 8.x ?

I think since a minor version of Gradle 6.x, not sure which exactly. This does not prevent the plugin from running on Gradle 8.x, but it does break projects that attempt to enable configuration cache.

@Sineaggi
Copy link
Contributor Author

Without these changes, we get the following error

2 problems were found storing the configuration cache.
- Task `:app:crd2java` of type `io.fabric8.java.generator.gradle.plugin.task.JavaGeneratorCrd2JavaTask`: cannot deserialize object of type 'org.gradle.api.Project' as these are not supported with the configuration cache.
  See https://docs.gradle.org/8.7/userguide/configuration_cache.html#config_cache:requirements:disallowed_types
- Task `:app:crd2java` of type `io.fabric8.java.generator.gradle.plugin.task.JavaGeneratorCrd2JavaTask`: cannot serialize object of type 'org.gradle.api.internal.project.DefaultProject', a subtype of 'org.gradle.api.Project', as these are not supported with the configuration cache.
  See https://docs.gradle.org/8.7/userguide/configuration_cache.html#config_cache:requirements:disallowed_types

No errors with this change.

@Sineaggi Sineaggi force-pushed the remote-project-from-task branch from cbade85 to a78c2cd Compare May 31, 2024 17:25
@Sineaggi
Copy link
Contributor Author

Sineaggi commented Jun 1, 2024

Normally I'd be able to create a test (via GradleRunner) that starts up Gradle with the plugin, but I wasn't having much luck getting that to work from maven.

Copy link

sonarqubecloud bot commented Jun 3, 2024

@Sineaggi
Copy link
Contributor Author

Good to merge?

@manusa
Copy link
Member

manusa commented Jun 12, 2024

Good to merge?

Yes it is.

However, I'd like to check a few things locally. I'll probably merge on Friday.

Sorry for the delay.

@manusa manusa added this to the 7.0.0 milestone Jun 14, 2024 — with automated-tasks
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx!

@manusa manusa merged commit ef55554 into fabric8io:main Jun 14, 2024
21 checks passed
manusa added a commit that referenced this pull request Jun 14, 2024
Signed-off-by: Marc Nuri <marc@marcnuri.com>
@Sineaggi Sineaggi deleted the remote-project-from-task branch June 14, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants