Modifying a parent configuration using 'beforeResolve()' (see AntlrPlugin)

[lptr]

The antlr plugin uses a trick to add a default ANTLR dependency if no other was given:

antlrConfiguration.getIncoming().beforeResolve(new Action<ResolvableDependencies>() {
    public void execute(ResolvableDependencies resolvableDependencies) {
        DependencySet dependencies = antlrConfiguration.getDependencies();
        if (dependencies.isEmpty()) {
            dependencies.add(project.getDependencies().create("antlr:antlr:2.7.7@jar"));
        }
    }
});

There are two problems here:

• because we've already resolved the compile configuration (that extends antlr) for task dependencies, it's set to modified = false. When antlr gets a new dependency, the compile configuration isn't notified about the change, and when we ask it to resolve for results, it simply does not re-resolve.
• when we are resolving compile for task dependencies, the antlr parent configuration gets observed. When we resolve the antlr configuration for results (and add the dependency in beforeResolve()), it's already in OBSERVED state, and we inevitably print a deprecation warning.

[lptr]

daf2e0b fixes the first problem by marking the configuration as modified when a parent configuration is modified in a non-lenient way.

Still not sure how to solve the second problem, though.

[lptr]

One way of dealing with the second problem is to introduce a beforeObserved() listener, and do such business there.

[lptr]

The problem is, if we exposed beforeObserved(), it would make sense to expose the OBSERVED state on the public API as well.

[lptr]

There is an implementation for this in f219a85. It breaks compatibility, though. :(

[bigdaz]

Thanks for your deep analysis. It really helps clarify the problem. I think that the behaviour of 'beforeResolve' is currently broken, at the very least unhelpful.

In Gradle, we're using 'beforeResolve' to set up a default dependency if none is explicitly added. I'd expect any extending configurations to have these default dependencies configured as well.

So if "compile extends antlr" then I think it would be more helpful if 'beforeResolve' hooks on antlr were fired before 'compile' is resolved: this would mean that we could replace the 'beforeResolve' implementation with the 'beforeObserved' hook that you've added (and not add any new API). This would be a 'potentially breaking change' but in many respects a bug that is now fixed. We are changing the meaning from "before explicitly resolved" to "before participates in resolution".

Does that sound like it would work?

Benefit: beforeResolve can be used to predictably modify a parent configuration, rather than the end result being dependent on order that configurations are resolved Problem: it would be hard to guarantee that 'afterResolve' is fired symmetrically with 'beforeResolve', since 'beforeResolve' would be fired on any usage in resolution, but afterResolve would need to wait until explicity resolved. I'm not sure if this is really a problem.

[lptr]

I think that would be great. I explored that route first, too, and bumped into the requirement that beforeResolve() and afterResolve() should be called symmetrically. It seems that we either need to break that symmetry, or break the API by introducing a new event. My guess is that breaking that symmetry would be less of a problem, but I haven't yet looked into it.

[adammurdoch]

The basic problem is that using beforeResolve() has never worked as a reliable way to add defaults. I'd just add a way to provide defaults when the configuration is empty.

We should not change the semantics of beforeResolve() and afterResolve().

[lptr]

If we fix the order of calling beforeObserve(), that could serve as a place. Maybe calling this something that doesn't include "observe" in its name would make it look less like exposing internals.

Or do you plan a separate DSL just for this? What was the problem with using beforeResolve() before?

[lptr]

I did some deeper investigation. It's possible to make this work by using beforeObserve(). We can still introduce a new DSL for declaring default dependencies, and it can work via beforeObserve() (which can remain internal).

To do this, the order of calls on a Configuration must look like this:

• call beforeObserve(), but do not set state = OBSERVED -- potentially adding more dependencies
• resolve the dependencies of the configuration
• do not set state = OBSERVED here either

We must only set state = OBSERVED on visited configurations after the traversal has finished. Otherwise we'd be emitting warnings about modifying an observed configuration when a dependency configuration adds a default dependency upon being observed.

This means we need some way to collect visited configurations during traversal.

*

There's another approach to supporting a new DSL to specify a default dependency. This would rely on a configuration to be able to return its default dependencies when there are no other dependencies declared.

Something ugly like this:

public DependencySet getAllDependenciesOrFallBackToDefaults() {
  if (allDependencies.isEmpty()) {
    return allDependencies;
  } else {
    return defaultDependencies;
  }
}

I like it a lot that this solution would not depend on last-minute mutation. One downside is that we couldn't really call dependency listeners like all() in any meaningful way. Not sure if this is a problem, though. After all, getAllDependencies() only returns the default dependencies now after the configuration has been resolved, which is a bit confusing already.

This solution would need a simpler implementation, and we could remove beforeObserve() completely. And this would also enable us to mark configurations as observed during any point in the traversal of the graph, because there would be no mutation as such.

[lptr]

Some other questions about a new DSL for adding default dependencies:

• what happens when a configuration is copied? Do we copy the default dependencies, fold them into the regular dependencies (and filter them when needed), or ignore them?
• when both a configuration and its parent has defaults, which defaults should take precedence if both configurations are otherwise empty? It should be the parent's, right?

[lptr]

A couple more questions about how this new DSL should work:

• do defaults trigger when there is no dependency declared in dependencies, or in allDependencies? I.e. do we trigger if we have inherited dependencies, but not do not declare any own dependencies? I think checking allDependencies.isEmpty() makes more sense from the user's standpoint, but I could argue either way.
• do we apply substitution rules to default dependencies? I'd think yes.

[lptr]

@adammurdoch, @bigdaz, can you please comment? Thanks.

[adammurdoch]

I would do something like this:

1. Add a whenEmpty(Action<? super DependencySet>) method to Configuration
2. When the configuration is about to be observed, and it has no dependencies (ignoring inherited) then invoke the action, passing the configuration's dependency set to the action.
3. Repeat for parent configurations.

Other comments:

• A copy of a configuration copies the action, not the defaults.
• Default dependencies are no different in any way to explicit dependencies - they are inherited, they are subject to substitution rules, etc.

[lptr]

What should happen to all the places where we rely on getDependencies()/getAllDependencies()? In many places it would make sense to process default dependencies, too. Shall we run whenEmpty() from getAllDependencies() perhaps?

If not, what happens to code that wants to process all dependencies, even default ones? Like these:

• https://github.com/prezi/gradle/blob/7b6feaba6736f8b48598af1517719f5f50badce1/subprojects/plugins/src/main/groovy/org/gradle/api/plugins/JavaPlugin.java#L111
• https://github.com/prezi/gradle/blob/0c8366aac6f14626a8dc60f2fae6ff9c1fb59e45/subprojects/core/src/main/groovy/org/gradle/api/internal/artifacts/dependencies/DefaultProjectDependency.java#L77
• https://github.com/prezi/gradle/blob/751c4263b5a9df4814e624b36fd042dc98b283d8/subprojects/ide/src/main/java/org/gradle/plugins/ide/internal/resolver/DefaultIdeDependencyResolver.java#L202

(In some cases the best solution would be to rely on ResolutionResult if they want to process true dependencies, but still.)

Would introducing getEffectiveDependencies() solve anything?

IIUC, default dependencies added via beforeResolve() were not always processed in these places, as the presence of default dependencies depended on whether or not the Configuration had already been resolved or not. That said, I don't think we should leave it like that.

[lptr]

We'll also need to figure out how to solve ShortcircuitEmptyConfigsArtifactDependencyResolver. Previously, beforeResolve() was already called when we got here, but now we don't know if whenEmpty was already called here:

https://github.com/prezi/gradle/blob/b73a407120212b5539eed669836239174cf71955/subprojects/dependency-management/src/main/java/org/gradle/api/internal/artifacts/ivyservice/ShortcircuitEmptyConfigsArtifactDependencyResolver.java#L51-51

[lptr]

Here's a very simple implementation that doesn't solve any of the previous problems: https://github.com/gradle/gradle/pull/424