Skip to content
New issue

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

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

Already on GitHub? # to your account

Gradle configuration cache support #307

Closed
ALikhachev opened this issue Jul 27, 2021 · 13 comments · Fixed by #351
Closed

Gradle configuration cache support #307

ALikhachev opened this issue Jul 27, 2021 · 13 comments · Fixed by #351

Comments

@ALikhachev
Copy link
Contributor

ALikhachev commented Jul 27, 2021

The plugin currently doesn't support Gradle configuration cache feature that aims to improve build performance by caching configured task graph. There're a bunch of limitations of what tasks can use and/or capture: https://docs.gradle.org/current/userguide/configuration_cache.html

9 problems were found storing the configuration cache, 7 of which seem unique.
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposeBuild', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: 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/6.9/userguide/configuration_cache.html#config_cache:requirements:disallowed_types
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposeDownForced', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposeDown', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposeLogs', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposePull', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
- Task `:composeUp` of type `com.avast.gradle.dockercompose.tasks.ComposeUp`: cannot serialize object of type 'com.avast.gradle.dockercompose.tasks.ComposePush', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/6.9/userguide/configuration_cache.html#config_cache:requirements:task_access
@augi
Copy link
Member

augi commented Jul 27, 2021

Hello, thank you for reporting this! 👍

I spent a long time reading the documentation and trying to understand what exactly should be done.

I'm not sure we are able to satisfy all the requirements 😢 E.g. ComposeSettings class creates the Tasks and stores them to its properties, to be used by the user to specify dependencies between the tasks. But the ComposeSettings is not a task, it is used as an extension, so we cannot declare these newly created tasks as Input nor Output, as they suggest.

Do you please have any idea how to overcome this? 🙏

@augi
Copy link
Member

augi commented Jul 27, 2021

I've probably got it. The tasks shouldn't depend on the extension but a type that contains a subset of the properties (excluding tasks). The main point for me is that the cache root is a Task so we must ensure the task is serializable.

@ALikhachev
Copy link
Contributor Author

ALikhachev commented Jul 27, 2021

Yes, you got it right. Each task and all types that it holds should be serializable and it's state should be small (granular) as much as possible.

@augi
Copy link
Member

augi commented Aug 2, 2021

@ALikhachev My idea is that I will use Property<> (instead of raw Groovy properties) in the extension (actually did it right now), and then I will assign these properties instances to the tasks created by the extension. So the tasks will have dependency on the Property objects instead of the whole extension. Also, when user assigns a new value to a property on the extension, the change is automatically distributed to the tasks (because it is the same instance of Property<>), et vice versa. Is this correct? 🙏

@eskatos
Copy link

eskatos commented Sep 17, 2021

@augi, this is correct and the way to go!

@augi
Copy link
Member

augi commented Sep 17, 2021

@eskatos Thank you for the confirmation! 👍 I will try to perform this refactoring during the next few weeks, so if this is critical for someone, any PR is welcomed 🙏

@eskatos
Copy link

eskatos commented Sep 17, 2021

Given the configuration cache adds more strictness to how plugins must be implemented, it is a good measure to always enable it in your integration tests. That way you can be confident your plugin works both without and with CC and you don't break the support going forward.

But I only found ProjectBuilder based unit tests in this repository. You can't test the configuration cache with these.

@vmiliantsei
Copy link

I use the following as a workaround for now:

task someTest(type: Test) {
	...

	// remove when fixed: https://github.com/avast/gradle-docker-compose-plugin/issues/307
	doFirst {
		tasks.getByName('composeUp').up()
		dockerCompose.exposeAsSystemProperties it
		tasks.getByName('composeDown').enabled = true
	}
	// remove when fixed: https://github.com/avast/gradle-docker-compose-plugin/issues/307
	finalizedBy 'composeDown'
}

dockerCompose {
	// uncomment when fixed: https://github.com/avast/gradle-docker-compose-plugin/issues/307
//	isRequiredBy tasks.named('someTest')
}

// remove when fixed: https://github.com/avast/gradle-docker-compose-plugin/issues/307
tasks.named('composeDown') {
	enabled = false
}

@ALikhachev
Copy link
Contributor Author

ALikhachev commented Jan 14, 2022

@vmiliantsei Are you sure your workaround works fine? You are accessing Project#getTasks at execution time which will lead to NPE (project object isn't available) on configuration cache reuse. You also access dockerCompose extension.

The first build probably passes fine due to lack of Groovy scripts validations in terms of configuration cache.

@vmiliantsei
Copy link

@ALikhachev, sorry, seems like I confused cache with the incremental build. I needed services to not start if test is up-to-date.

@eskatos
Copy link

eskatos commented Jan 27, 2022

@augi, it's great you took steps in the right direction. Any plans on going further and to split the extension in order to remove cross-task references?

@augi
Copy link
Member

augi commented Jan 27, 2022

@eskatos There is still one thing that I don't know how to resolve 😢 The tasks use an instance of ComposeExecutor class. The instance is created just once, in the extension, and the tasks are using it via the extension.

I don't know how to handle the lifestyle of this class instance and ensure that tasks are serializable. E.g. how exactly the tasks is deserialized, if there is an extension point where we could create a new instance of the ComposeExecutor (the ctor would be perfect).

It would be OK to have a new instance of this class for each task, but it would lead to small performance degradation because this cache wouldn't be so efficient.

@eskatos
Copy link

eskatos commented Jan 27, 2022

This sounds like a good candidate for a shared build service https://docs.gradle.org/current/userguide/build_services.html.

ExecOperations and FileSystemOperations can be injected in build services.
AutoCloseable can be used to replace gradle.buildFinished {}.

ProjectLayout isn't available in shared build services as they are build scoped whereas ProjectLayout is project scoped. You would have to pass it to the service when using it from a task.

Another approach would be to move the computation of the compose version to a task, write it's output to a file and then consume that from the tasks that need it.

ALikhachev added a commit to ALikhachev/gradle-docker-compose-plugin that referenced this issue Apr 15, 2022
Reading all the environment variables at configuration time leads to marking each of them as configuration cache input, so Gradle will detect value changes and invalidate cache on any irrelevant variable value change
avast#307
ALikhachev added a commit to ALikhachev/gradle-docker-compose-plugin that referenced this issue Apr 15, 2022
ALikhachev added a commit to ALikhachev/gradle-docker-compose-plugin that referenced this issue Apr 15, 2022
ALikhachev added a commit to ALikhachev/gradle-docker-compose-plugin that referenced this issue Apr 15, 2022
@augi augi closed this as completed in #351 Apr 27, 2022
augi pushed a commit that referenced this issue Apr 27, 2022
Reading all the environment variables at configuration time leads to marking each of them as configuration cache input, so Gradle will detect value changes and invalidate cache on any irrelevant variable value change
#307
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants