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

Execute the configuration actions lazily #618

Merged
merged 2 commits into from
Jun 19, 2020
Merged

Execute the configuration actions lazily #618

merged 2 commits into from
Jun 19, 2020

Conversation

nedtwigg
Copy link
Member

The only substantive change is that instead of this in the legacy SpotlessExtension...

public <T extends FormatExtension> void format(String name, Class<T> clazz, Action<T> configure) {
T format = maybeCreate(name, clazz);
configure.execute(format);
}

...we add a field to FormatExtension, and populate it in each call to SpotlessExtension::format(name, clazz, action)

final List<Action<FormatExtension>> modernLazyActions = new ArrayList<>();

public <T extends FormatExtension> void format(String name, Class<T> clazz, Action<T> configure) {
maybeCreate(name, clazz).modernLazyActions.add((Action<FormatExtension>) configure);
}

Instead of executing the closures when they are declared, we store them, and only execute them when the task is configured:

project.afterEvaluate(unused -> {
spotlessTask.configure(task -> {
// now that the task is being configured, we execute our actions
for (Action<FormatExtension> lazyAction : formatExtension.modernLazyActions) {
lazyAction.execute(formatExtension);
}
// and now we'll setup the task
formatExtension.setupTask(task);
});
});

In my head, I thought that we would be able to remove the afterEvaluate block, but now I see that it was a mirage. If someone eagerly creates the task, and then calls java { blah }, it's important for that blah to have effect, but it wouldn't if you don't do the afterEvaluate.

It seems to me that this is as lazy as it could possibly be - we don't describe any of the properties of a task, or even execute the code that describes those those properties, until/unless that task is being configured. It also seems to me like it's quite difficult to avoid the afterEvaluate, which I guess is the point of the Provider<> stuff. We could change FormatExtension so that it applied itself directly against the SpotlessTask, rather than storing its own field for encoding, etc. The only hard part against that is cases where a user sets UTF-8 at the level of the Spotless block, but has some legacy subproject where encoding is CP-1252, cases like that.

Is afterEvaluate in any long-term trouble? Whaddya think @bigdaz?

@bigdaz
Copy link
Contributor

bigdaz commented Jun 19, 2020

@nedtwigg, I think your change makes sense to defer the possibly expensive configuration of each FormatExtension until we know that the output is really required. The only downside would be if someone queries the values of the extension before the "owning" task is created and configured.

Is afterEvaluate in any long-term trouble?

Very long term, we'd like to deprecate and remove. In the meantime, it's just considered poor form. In this case it's usage probably won't bite, but when layering multiple plugins the multiple afterEvaluate blocks can be problematic, particularly in the order they are executed.

The use of Property/Provider types would replace both the lazy configuration you propose for FormatExtension as well as the afterEvaluate. All properties on FormatExtension would be declared as either Provider or Property, with no setters on FormatExtension. You can then wire these properties directly into the task configuration at any time, since the values are ready lazily.

One nice thing is that you can declare FormatExtension as an abstract class, and simply add abstract getters for each property. This is called a "managed property" and Gradle will take care of instantiating the Provider for each property.

We are currently working on better documentation on what we consider to be idiomatic Gradle usage with Gradle 6+. Following these patterns will ensure that plugins/builds work better with the upcoming configuration-cache as well as providing a more concise, consistent user experience.

@nedtwigg
Copy link
Member Author

I look forward to the docs! Fwiw, I'm a big fan of all the cool properties that you get from lazily computing immutable values, ala LazyForwardingEquality, so I think I should like the Property/Provider stuff. But so far I don't, because they seem difficult to coordinate.

What I mean by coordinate, is that I usually don't need a lazily-computed immutable integer, or lazily computed immutable String - I need a lazily computed immutable state, which usually has multiple components, and the inputs which eventually determine that state often have a different shape than the state they determine.

For example, if a user says ratchetFrom 'branch', that gets lazily turned into one immutable sha for the repository root, and also a set of immutable shas for each git tree that happens to be at the root of each subproject. So we have input {String reference} -> state { Sha rootTree, Map<Project, Sha> perProjectTrees }.

Also, when determining that input, mutability is quite handy. It's nice to apply a plugin which sets a bunch of defaults, and then be able to override values here and there on a per-project basis. Definitely there can be some footguns there, but the beauty of the groovy scripts, imo, is how easy it is for naive scripting logic to do what people expect.

So to me, the idea of "mutate inputs" -> afterEvaluate -> "now everything is frozen, compute immutable states out of the inputs" is easy to use and easy to reason about. The only problem is that composing afterEvaluate is disallowed, but I don't understand why the afterEvaluate calls can't be trampolined.

You have thought about this much more than me, I'm sure, and I look forward to the future releases, but my hope is that Gradle continues to have as few Gradle-specific concepts as possible. So far it has mostly been "normal script operating on normal values builds a task graph". If it becomes "normal script operating on special gradle-y values builds a task graph", I worry a bit about how much "magic" script and plugin authors will have to master. But you guys are batting like 99%, so I'm on board.

</rant>. No response expected, just wanted to share my thoughts since I've got a Gradle person's attention for a bit :). I'll merge this, and let it rest for a bit in case we have other ideas for things to improve.

@nedtwigg nedtwigg merged commit aa42229 into main Jun 19, 2020
@nedtwigg
Copy link
Member Author

Part of #601

@nedtwigg nedtwigg deleted the feat/lazy branch June 19, 2020 17:48
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.

2 participants