Update AbstractArchiveTask to expose Provider API #7435

JLLeitschuh · 2018-10-17T23:16:14Z

Before this change, there was no easy way to rely upon the output
of the AbstractArchiveTask without using dependsOn.

Related: https://github.com/gradle/gradle-native/issues/893

Signed-off-by: Jonathan Leitschuh [email protected]

Contributor Checklist

Review Contribution Guidelines
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
Provide unit tests (under <subproject>/src/test) to verify logic
Update User Guide, DSL Reference, and Javadoc for public-facing changes
Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation
Recognize contributor in release notes

JLLeitschuh · 2018-10-17T23:17:16Z

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaBasePlugin.java

Should this TODO be removed now?

Yep, I think so

JLLeitschuh · 2018-10-17T23:19:02Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

I didn't really want this to show up in the generated docs, so I left it using the implNote tag, hope that's okay.

Yeah, it looks like the version of javadoc we're using doesn't like @implNote. You could also put this as a comment inside the method too.

JLLeitschuh · 2018-10-17T23:19:40Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

If anyone has a better name for this, let me know.

getExtension is the best name, but it's already taken. Maybe getArchiveExtension?

getExtension is already taken, I'll use getArchiveExtension.

big-guy

Thanks for taking the time on this. I've left some comments.

The test failures are due to the convention mapping no longer working from BasePlugin. We'd need to change

gradle/subprojects/plugins/src/main/java/org/gradle/api/plugins/BasePlugin.java

Lines 92 to 106 in c78d05f

    
           Callable<File> destinationDir; 
        
           if (task instanceof Jar) { 
        
               destinationDir = new Callable<File>() { 
        
                   public File call() throws Exception { 
        
                       return pluginConvention.getLibsDir(); 
        
                   } 
        
               }; 
        
           } else { 
        
               destinationDir = new Callable<File>() { 
        
                   public File call() throws Exception { 
        
                       return pluginConvention.getDistsDir(); 
        
                   } 
        
               }; 
        
           } 
        
           taskConventionMapping.map("destinationDir", destinationDir);

to something like:

if (task instanceof Jar) {
   task.getDestinationDirectory().set(project.getLayout().getBuildDirectory().dir(pluginConvention.getDistsDirName()));
} else {
   task.getDestinationDirectory().set(project.getLayout().getBuildDirectory().dir(pluginConvention.getLibsDirName()));
}

And deprecate the BasePluginConvention.get*Dirs methods.

big-guy · 2018-10-21T01:32:21Z

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaBasePlugin.java

Yep, I think so

big-guy · 2018-10-21T01:33:36Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

Could you make this a regular default constructor?

Sure.
It just seemed clearer to have this logic as close to where the field was declared as possible.
Happy to change it.

big-guy · 2018-10-21T01:33:51Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

Please make this final

big-guy · 2018-10-21T01:33:55Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

Please make this final

big-guy · 2018-10-21T01:34:08Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

Thanks for fixing all of these!

big-guy · 2018-10-21T01:36:05Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

I think our checks will want @since tags in the javadoc for all new methods.

Could you also add destinationDirectory and the new extension method below to subprojects/docs/src/docs/dsl/org.gradle.api.tasks.bundling.AbstractArchiveTask.xml as properties?

Sure. What is subprojects/docs/src/docs/dsl/org.gradle.api.tasks.bundling.AbstractArchiveTask.xml?

Done, I think.

big-guy · 2018-10-21T01:38:38Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

I think @marcphilipp just added this.

I think the behavior with the Provider/Property is that the error will be about an unset provider?

There's still an exception thrown, it's a different exception now though and it happens at a different phase in the build.

big-guy · 2018-10-21T01:39:38Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

getExtension is the best name, but it's already taken. Maybe getArchiveExtension?

big-guy · 2018-10-21T01:48:52Z

subprojects/core/src/main/java/org/gradle/api/tasks/bundling/AbstractArchiveTask.java

Yeah, it looks like the version of javadoc we're using doesn't like @implNote. You could also put this as a comment inside the method too.

Before this change, there was no easy way to rely upon the output of the AbstractArchiveTask without using dependsOn. Related: https://github.com/gradle/gradle-native/issues/893 Signed-off-by: Jonathan Leitschuh <[email protected]>

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh · 2018-10-24T22:07:46Z

subprojects/plugins/src/main/java/org/gradle/api/plugins/BasePlugin.java

+                            }
+                        };
+                    }
+                    task.getDestinationDirectory().set(project.getLayout().getBuildDirectory().dir(project.provider(destinationDir)));


Thank you @big-guy for working on this with me!

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh · 2018-10-25T20:51:01Z

Supposedly I introduced some binary incompatibility?? @big-guy can you tell me what that means so I can try and fix it?

big-guy · 2018-10-25T20:57:38Z

Thanks @JLLeitschuh, I merged this locally, fixed a couple of things and pushed it to master. This'll be in 5.1

JLLeitschuh · 2018-10-26T14:18:26Z

Thanks!

JLLeitschuh commented Oct 17, 2018

View reviewed changes

JLLeitschuh force-pushed the provider-api-on-abstract-archive-task branch from e99e9a3 to 9b4c427 Compare October 17, 2018 23:21

big-guy requested changes Oct 21, 2018

View reviewed changes

Update AbstractArchiveTask to expose Provider API

aff5155

Before this change, there was no easy way to rely upon the output of the AbstractArchiveTask without using dependsOn. Related: https://github.com/gradle/gradle-native/issues/893 Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh mentioned this pull request Oct 22, 2018

The Gradle Groovy DSL generator should support @apiNote, @implSpec and @implNote #7486

Open

Make all fields in the AbstractArchiveTask be backed by properties

4c5e939

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh force-pushed the provider-api-on-abstract-archive-task branch from 9b4c427 to 4c5e939 Compare October 22, 2018 18:37

This was referenced Oct 23, 2018

Add in nullability annotations to getters and setters in `AbstractArc… #7509

Closed

AbstractArchiveTask missing nullability annotations on various setter properties gradle/kotlin-dsl-samples#1206

Closed

Fixup failing tests after AbstractArchiveTask changes

14194b9

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh commented Oct 24, 2018

View reviewed changes

Merge branch 'master' into provider-api-on-abstract-archive-task

a6abefd

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh force-pushed the provider-api-on-abstract-archive-task branch from 4d68c55 to a6abefd Compare October 24, 2018 22:09

Remove unused CompileStatic import

666240a

Signed-off-by: Jonathan Leitschuh <[email protected]>

JLLeitschuh force-pushed the provider-api-on-abstract-archive-task branch from c92f28f to 666240a Compare October 25, 2018 15:54

big-guy closed this Oct 25, 2018

JLLeitschuh mentioned this pull request Oct 26, 2018

No value ListProperty and SetProperty shouldn't allow adding elements #7485

Closed

devoto13 mentioned this pull request Jan 7, 2019

Build failure with Gradle 5.1 GradleUp/shadow#446

Closed

elusiveeagle mentioned this pull request Aug 9, 2019

Unable to set output archive name nebula-plugins/gradle-ospackage-plugin#338

Open

	Callable<File> destinationDir;
	if (task instanceof Jar) {
	destinationDir = new Callable<File>() {
	public File call() throws Exception {
	return pluginConvention.getLibsDir();
	}
	};
	} else {
	destinationDir = new Callable<File>() {
	public File call() throws Exception {
	return pluginConvention.getDistsDir();
	}
	};
	}
	taskConventionMapping.map("destinationDir", destinationDir);

Update AbstractArchiveTask to expose Provider API #7435

Update AbstractArchiveTask to expose Provider API #7435

Uh oh!

Conversation

JLLeitschuh commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Checklist

Gradle Core Team Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

big-guy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh commented Oct 25, 2018

Uh oh!

big-guy commented Oct 25, 2018

Uh oh!

JLLeitschuh commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JLLeitschuh commented Oct 17, 2018 •

edited

Loading

JLLeitschuh Oct 21, 2018 •

edited

Loading

JLLeitschuh Oct 21, 2018 •

edited

Loading

JLLeitschuh Oct 22, 2018 •

edited

Loading