Convention mapping problem in Gradle 1.0-m8a when task extends another task and overrides its properties

Old Forum Archive
1

The following build script prints “Hello Gradle!” when executed with Gradle 1.0-m6 and “Hello world!” when run with 1.0-m8a:

import org.gradle.api.internal.ConventionTask
  defaultTasks 'hello'
  tasks.withType(Hello) {
 conventionMapping.message = { 'Hello world!' }
}
  task hello(type: Hello) {
 helloMessage = 'Hello Gradle!'
}
   class Greet extends ConventionTask {
 String message
    @Input
 public String getMessage() {
  return this.message
 }
    public void setMessage(String message) {
  this.message = message
  }
    @TaskAction
 void greet() {
  println 'Message From Greet task: ' + getMessage()
 }
}
  class Hello extends Greet {
 String helloMessage
    @Input
 @Override
 public String getMessage() {
  return this.helloMessage
 }
    @Override
 public void setMessage(String message) {
  this.helloMessage = message
  }
    @TaskAction
 void hello() {
  println 'Message From Hello task: ' + getMessage()
 }
}

I noticed that making “helloMessage” private makes it print “Hello world!” even when run with 1.0-m6. Any ideas?

Regards,

Detelin

2

What’s the exact output you see for both versions?

3

Using Gradle home…C:\gradle-1.0-milestone-6 > Using Gradle user home…D:\Users\bgde.gradle > :hello > Message From Greet task: Hello Gradle! > Message From Hello task: Hello Gradle!

Using Gradle home…C:\gradle-1.0-milestone-8a > Using Gradle user home…D:\Users\bgde.gradle > :hello > Message From Greet task: Hello world! > Message From Hello task: Hello world!

Unless I have missed something, I would expect to always get “Hello Gradle!” since this value is explicitly configured and should override the convention mapping value.

4

I can’t find that change that introduced this, but the new version is the correct behaviour.

Convention mappings are per property, not per instance variable. So the convention mapping applies to ‘Hello.message’ property which is never set explicitly, therefore the convention mapping is never invalidated. The m6 behaviour surprises me.

This is a rather strange setup with these task classes. I’m interested in what your real use case is as I see the “new” behaviour as correct, but I don’t know your case.

Also, you should avoid using ‘ConventionTask’. It’s better to use ‘DefaultTask’ and cast to ‘IConventionAware’ if you need to use convention mappings from Java. You don’t need to extend ‘ConventionTask’ to get convention mapping features.

5

The change should be introduced in Gradle 1.0-m8 because I just tried it with m7 and it works the same as in m6.

In my setup I rely on being able to extend a task and re-use its core functionality while modifying some bits by overriding certain properties. I also make extensive use of configuration via convention mapping to configure all tasks that inherit from certain base type. However with Gradle 1.0-m8 I’m not able to override values set via the convention mapping by merely overriding the getters :frowning:

6

So maybe I could move all the functionality in the overridden getters in new convention mapping closures…

7

Tasks should no nothing at all about convention mappings, so any solution that relies on them or is tailored towards them is going to have problems.

I still don’t quite understand your case though so I could be being too simplistic.

8

Did some digging, I suspect that the following change might lead to this behavior:

https://github.com/gradle/gradle/commit/2e69764a681a0aaaedf8801203ad0d97cd9d3585

The Gradle 1.0-m6 AbstractClassGenerator implementation took care to override setters if they are already defined, the new Gradle 1.0-m8a implementation seems to fail to do so because of this piece of code:

for (MetaBeanProperty property : settableProperties) {

Collection methodsForProperty = methods.get(property.getName());

if (methodsForProperty.isEmpty()) {

builder.addSetMethod(property);

} else if (conventionProperties.contains(property)) {

for (MetaMethod method : methodsForProperty) {

builder.overrideSetMethod(property, method);

}

}

}

While testing this I noticed that setter is added 2nd time even for convention properties, because ‘methods’ map is keyed by the setter name e.g. “setMessage” while property name is “message” and ‘methodsForProperty’ is always empty.

Does this make sense?

9

Found the cause, which isn’t the code above.

What’s being termed “set method” is not really a setter here. The code you found is making this work:

class GreetTask extends DefaultTask {
  String foo
}
  task greet(type: GreetTask) {
  foo "bar"
}

That is, adding a “set method” that is the name of the property so you don’t have to use assignment.

The change is here:

https://github.com/gradle/gradle/commit/4dbb79fa3dcc58dcac6f8c3d4c1fcea4f6b87c59#L2L117

Previously, the a convention had not been invalidated it would only be used if the default instance value was null or an empty map/collection. This was a bug. If a convention mapping has been set and has not been invalidated, it should always be returned no matter what the default instance value is.

So to be clear, the m6 and earlier behaviour was a bug and m8 has the correct behaviour.

10

Thanks Luke,

Everything makes sense now, you are right that the current behavior is the correct one, but maybe this should be documented somewhere (if it is not already). After working with Gradle for some time, I figured that tasks must read their properties via the getters to trigger convention mapping lookup, and somehow I was convinced that if the getter returns ‘null’ then the convention value would be used. After reading through the code and seeing how this is implemented it is now clear to me that invalidation is done via the generated setter. Probably this is considered as internal implementation that should not be exposed in the user docs, but I think that if it is documented it would help people rather than confuse them. I will also fix all our tasks to extend from the DefaultTask instead of the ConventionTask as you suggested. By the way, is there a clean approach to make any instance convention aware, so far I have been using:

myConventionAwareTypeInstance = project.gradle.services.get(Instantiator).newInstance(MyType, project.getFileResolver())

but I would like not to use internal stuff if possible?

Thanks a lot!

11

So it turns out the situation gets more complex. It turns out you’re right in a sense.

Previously a convention mapping would only apply if the decorated setter had not been explicitly set and if the “natural” value was not “defaultish”. The idea of of not applying convention mappings when the “natural” value is not “defaultish” is to work around some awkwardness with convention mappings and collections. The change that occurred in m7 is that we only treat non empty collections as non “defaultish” where as previously we also interpreted ‘null’ in this manner.

As we work towards making convention mapping public, we are trying to simplify it to remove this whole “defaultish” business as it complicates things too far. But, if you have a good reason for keeping the existing behaviour now would be a good time to raise it.

I was surprised to see convention mapping in the docs because it’s not public, I’ll remove it. Thanks.

There’s no public API for creating enhanced objects at this time, except for extensions.

http://gradle.org/docs/nightly/javadoc/org/gradle/api/plugins/ExtensionContainer.html#create(java.lang.String%2C%20java.lang.Class%2C%20java.lang.Object...)

It only mentions making objects ‘ExtensionAware’ but they are also convention aware.

What kind of objects do you need to enhance?

12

For example - Gradle’s DefaultOSGiManifest - currently instances created via project.osgiManifest() are not convention aware, but I would like them to be. The reason is, because we are having a special task that switches project’s version to a release one (similar to ‘:releaseVersion’ task in Gradle’s own build), however since osgi manifests are created at project evaluation time they might read project’s ‘version’ before it’s being overridden by any ‘release’ task (actually we are detecting the presence of any ‘release’ task by examining start parameters). If osgi manifest is convention aware, this is easily fixed by using convention mapping:

def OsgiManifest osgiManifest() {

Instantiator instantiator = project.gradle.services.get(Instantiator)

instantiator.newInstance(DefaultOSGiManifest, project.fileResolver).with {

conventionMapping.version = { project.version }

return it

}

}

I would prefer not to use internal classes but so far I was unable to find another approach.

13

There’s no non internal alternative for that if you want class decoration.

You could do this though…

OsgiManifest osgiManifest() {
    new DefaultOsgiManifest(project.fileResolver) {
      String getVersion() { project.version }
    }
  }

Same result, unless you are using other “enhanced” features.

14

Thanks,

I might switch to that, unfortunately the DefaultOsgiManifest does not always access the ‘version’ via the getter but happens to reference it via the property on one place (in setAnalyzerProperties), but this is a different topic and I will address it separately.

Thank you very much for helping me figure the property issue out, I will make sure the setters are always called to invalidate any convention values, also try to create new task types and avoid extending existing ones where possible.

Regads,

Detelin