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

Change WireTask inputs to a serializable type #1560

Merged

Conversation

stephanenicolas
Copy link
Contributor

Solves issue #1515.

@stephanenicolas stephanenicolas marked this pull request as draft May 22, 2020 17:23
@@ -34,10 +35,10 @@ open class WireTask : SourceTask() {
var pluginVersion: String = VERSION

@get:Internal
internal lateinit var sourceInput: WireInput
internal lateinit var sourceInput: List<Location>
Copy link
Contributor Author

@stephanenicolas stephanenicolas May 22, 2020

Choose a reason for hiding this comment

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

This is the main change: WireInput cannot be serialized during instant execution (a.k.a config caching) because it has a configuration which is not serializable.
The workaround is to use the Locations data class which seem to be handled well by Gradle. I could test and this seem to work well.
https://issuetracker.google.com/issues/153711618

it.outputDirectories = outputs.map { output -> File(output.out!!) }
it.source(sourceInput.configuration)
it.sourceInput = sourceInput
it.protoInput = protoInput
it.sourceInput = sourceInput.toLocations()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means the toLocations method is now called during configuration. Before, it was run during an afterEvaluate block which is pretty much the same. There might be a better way to organize things so that the dependency resolution would be performed at execution time, as it should but I don't know the plugin enough to refactor it this way.

Copy link

Choose a reason for hiding this comment

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

You could try make WireTask.sourceInput a ListProperty<Location> and wire a Provider<Location> to it here. Maybe changing WireInput.toLocation() to directly return the provider. This would defer dependency resolution to the task execution on a vintage build, and to the task graph calculation on a configuration-cache enabled build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be better, and I thought about using properties but all fields indeed should use lazy properties when it applies and it does for most of them.
That would be a nice refresh for the wire plugin. Can it be done in a different PR though @eskatos ? My goal is not really to modernize the plugin but to have instant execution working.

@stephanenicolas stephanenicolas marked this pull request as ready for review May 22, 2020 18:15
@stephanenicolas stephanenicolas changed the title [Draft] Change WireTask inputs to a serializable type Change WireTask inputs to a serializable type May 22, 2020
@oldergod
Copy link
Member

I think we don't wanna make toLocations on configuration. I wonder if we could just pass a list of string, then within the Plugin, do something like File(string).toLocation

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

This makes our cache key a function of our configurations’ dependencies. If these configurations dependencies change, will our cache be invalidated? If so this is good!

Alternately – can we change our cache key to be the configuration object or the dependencies object? This needs to be recomputed when that changes.

@stephanenicolas
Copy link
Contributor Author

@swankjesse @eskatos , I adopted the solution of @eskatos after talking with Rodrigo from Gradle. I didn't know that providers are evaluated after the configuration of the task, when configurations are resolved and the tasks are serialized.

So,in this case, @oldergod , we don't resolve configurations (via the call to toLocations()) during configuration, but right after. When the task is serialized, that's where gradle wants to resolve configuration and we use this information in the plugin exactly at this moment to provide the locations.

@swankjesse I am also wondering if we shouldn't change the @Internal annotation for @Input, and maybe apply some kind of normalizer to help the up-to-date check / caching.

@stephanenicolas
Copy link
Contributor Author

I could test and make sure it works:

  • gradle nightly 6.6-20200603220033+0000
  • AGP 4.1.0-alpha09
  • KGP 1.3.72
  • plus this PR for 3.3.0-SNAPSHOT

And everything works perfectly :D

@oldergod
Copy link
Member

oldergod commented Jun 5, 2020

Looks good to me. @jrodbx could you give a look?

@jrodbx
Copy link
Collaborator

jrodbx commented Jun 5, 2020

LGTM

@jrodbx jrodbx merged commit 843d5bc into square:master Jun 5, 2020
@oldergod
Copy link
Member

oldergod commented Jun 5, 2020

Thank you Stéphane

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