First pass at adding a custom plugin portal #883

ethankhall · 2016-11-17T21:37:55Z

This is a first pass at adding the ability to define a custom plugin portal.

I don't like the names of most of the classes, I would appreciate some suggestions to fix it. I also put some of it in it's own package, but that was just to help keep it separate, but I am more than happy to remove the package.

This still needs tests but I wanted to open the review now to get some early feedback.

ldaley · 2016-11-18T04:56:20Z

I'm on my way back to AU. I'll look into this on my Monday. Thanks @ethankhall!

ethankhall · 2016-11-18T21:07:04Z

Awesome! Let me know what you think. If this is the path we want to take I'll add some tests for it.

ldaley · 2016-11-25T05:21:19Z

@ethankhall I think this is looking good. I've left some requests in there. The main theme is that we want to reduce the user implementable types, but add more type safety and avenues for evolution.

We also need to come up with a better name for this than “custom” :\ @oehme @eljobe any ideas?

ethankhall · 2016-11-25T15:50:25Z

@alkemist I didn't see any comments on the PR.

oehme · 2016-11-25T16:54:59Z

The current implementation is probably not what you want. It uses the (otherwise unused) code path for "isolated" plugins, which means that plugins resolved from such a custom repo will be isolated and unable to see any other plugins. You'll want to use the "legacy" (misleading name) code path instead.

A better name/DSL for this could be

pluginRepositories {
  ruleBased {
    description = "The Foobar plugins"
    artifactRepositories {
      maven {
        url = 'foobar'
      }
    }
    pluginResolution { plugin, target ->
      if (plugin.id == "foobar") {
        target.useModule("com.foo:bar:${plugin.version}")
      }
    }
  }
}

ethankhall · 2016-11-25T17:21:24Z

Using the Isolated code path can you access plugins in the same jar / dependencies of the plugin? That would solve an issue I have with plugins using different library versions that aren't compatible nicely.

Can plugins using the plugin portal configure other plugins? It sounds like not.

oehme · 2016-11-26T07:48:17Z

Plugins using the plugins block work just like when you use the buildscript block. They can configure other plugins etc. This custom one should be no exception, otherwise it would confuse users.

We could make the new isolated behavior opt-in of course, by offering another method on the target in the dsl I proposed above.

ethankhall · 2016-11-28T13:56:22Z

Sounds good. I'll work on making those changes. Will this api be avaliable via an Init Script / what would the api to access it look like?

oehme · 2016-11-28T14:22:34Z

There is currently no pluginRepositories API in init scripts. I think it makes perfect sense to add it. @alkemist had some concerns though that we might want two different concepts:

an init script needs some plugins itself and the repository should only be used for that init script
an init script adds plugin repositories to the build that it applies to

ethankhall · 2016-11-28T14:26:05Z

Alrighty, I'll want to add some API to configure this as part of an init script, but i'll work on the bit make the API better first.

ethankhall · 2016-11-29T04:05:05Z

@oehme I've done the API changes that you requested. Let me know if that's what your thinking.

ethankhall · 2016-11-29T17:57:59Z

@alkemist @oehme, how do you want to expose this in init scripts?

oehme · 2016-11-29T17:59:21Z

subprojects/core/src/main/java/org/gradle/api/internal/plugins/DefaultAppliedPlugin.java

There is a lot of unnecessary code churn here, probably caused by a rename refactoring. Could you revert that and use the interface type where applicable?

oehme · 2016-11-29T18:01:54Z

subprojects/core/src/main/java/org/gradle/plugin/repository/rules/PluginDependency.java

This name is missing a word I think, maybe "Handler"

oehme · 2016-11-29T18:02:53Z

...in-use/src/main/java/org/gradle/plugin/repository/internal/BackedByArtifactRepositories.java

Let's not introduce another type, but just change/rename the existing BackedByArtifactRepository.

oehme · 2016-11-29T18:04:27Z

...cts/core/src/main/java/org/gradle/plugin/repository/rules/RuleBasedArtifactRepositories.java

We don't need a new type here, let's use RepositoryHandler.

oehme · 2016-11-29T18:07:32Z

...rojects/core/src/main/java/org/gradle/plugin/repository/rules/RuleBasedPluginResolution.java

The other Gradle APIs don't define new interfaces, but instead provide a single parameter object and use an Action. I think we should stick with that pattern to keep it consistent.

oehme · 2016-11-29T18:12:36Z

@ethankhall Let's tackle init scripts in another PR. I think the most straightforward thing would be to allow the pluginRepositories {} block in init scripts and let it mean "add those repositories to the list of plugin repositories of the build I'm applied to`. That's probably the most common use case which should have a convenient syntax. @alkemist what do you think?

ldaley · 2016-11-25T05:12:08Z

subprojects/core/src/main/java/org/gradle/plugin/repository/portal/CustomPluginProvider.java

This should take a structured object, and return some kind of result object. Something like this …

CustomPluginResolutionResult resolvePlugin(PluginRequest plugin, CustomPluginResolutionResultFactory resultFactory)

CustomPluginResolutionResult should be a sealed type hierarchy, with two subtypes: ResolvedCustomPluginResolutionResult and UnresolvedCustomPluginResolutionResult. These latter two should be final and all should have package scoped constructors.

CustomPluginResolutionResultFactory should have methods that create instances of these two types. e.g.

interface CustomPluginResolutionResultFactory { ResolvedCustomPluginResolutionResultFactory resolved(Object dependencyNotation) UnresolvedCustomPluginResolutionResultFactory unresolved(@Nullable String reason) }

ResolvedCustomPluginResolutionResultFactory will have a Dependency getDependency().

The reason for all this is that we now have some communicative types, and more options for safely evolving this API if necessary.

I'd prefer having a found/notFound callback instead of a factory plus return type.

ldaley · 2016-11-25T05:12:45Z

...jects/core/src/main/java/org/gradle/plugin/repository/portal/CustomPluginRepositorySpec.java

I think we can just inline the provider method here. I don't think we need two types.

ldaley · 2016-11-25T05:13:41Z

...jects/core/src/main/java/org/gradle/plugin/repository/portal/CustomPluginRepositorySpec.java

I don't think this needs to return a repository. The RepositoryHandler is effectively the container of repositories.

@oehme @eljobe it might actually be the case that this is needed because of

gradle/subprojects/plugin-use/src/main/java/org/gradle/plugin/use/internal/DefaultPluginRequestApplicator.java

Line 111 in 527f9bf

repositories.removeAll(pluginArtifactRepositories);

- can you please explain why we need to do this.

Because we want to prepend instead of append repositories. The global plugin repositories should have precedence over the ones defined in the buildscript block of a project. It's an unlikely edge case, users will rarely mix the two.

ldaley · 2016-11-30T00:49:04Z

...ojects/core/src/main/java/org/gradle/plugin/repository/internal/PluginRepositoryFactory.java

This method name is a little long winded. Perhaps just rules would be ok.

ldaley · 2016-11-30T00:51:29Z

subprojects/core/src/main/java/org/gradle/plugin/repository/RuleBasedPluginRepository.java

I'm quite confused by this interface. Is it the repository? It seems that it's more like a builder for a repository. What's the usage pattern here?

I guess I was thinking a repository is something that provides plugins. I think this kinda follows the PluginPortal way of doing things, maybe it's a Portal instead?

It's the same pattern like the other plugin repository subtypes. The user configures it and it has an internal method that turns into a plugin resolver.

ldaley · 2016-11-30T00:51:50Z

subprojects/core/src/main/java/org/gradle/plugin/repository/RuleBasedPluginRepository.java

Should be Action<? super X>

ldaley · 2016-11-30T01:06:36Z

@ethankhall looks like I don't know how to use GitHub code reviews. I added a bunch of comments, but it looks like they weren't saved (or whatever). They should be there now. Some of them no longer hold after the change requests from @oehme.

As for the init script issue, I agree with @oehme.

I'm not quite understanding how the usage patterns of the new API. I think this is indicative of it being a little convoluted.

@ethankhall @oehme @eriwen the quickest way forward here is probably to meet for 30 minutes to discuss synchronously. @eriwen would you mind making this happen?

ethankhall · 2016-11-30T01:15:28Z

@alkemist sounds good! I'm generally avaliable to meet.

oehme · 2016-11-30T13:58:36Z

@alkemist The user-facing DSL is shown in one of my comments above. Adjusted for the latest changes it looks like this:

pluginRepositories {
  rules {
    description = "The Foobar plugins"
    artifactRepositories {
      maven {
        url = 'foobar'
      }
    }
    pluginResolution {
      if (requestedPlugin.id == "foobar") {
        target.useModule("com.foo:bar:${requestedPlugin.version}")
      }
    }
  }
}

It closely follows the patterns of existing plugin repository types as well as patterns of our dependency resolution API.

I'm up for a short meeting, just let me know when.

Outdated.

Moving PluginId to be a public Interface, and then created an implementation called DefaultPluginId Added the first phase of interfaces (inside the class, will move out later) so that I can start working on the API under the hood.

- Moved PluginRequest to InternalPluginRequest - Created PluginRequest that InternalPluginRequest extends - Made the code compile for RuleBasedPluginRepository

Removed PluginId interface and made DefaultPluginId be a class that we share and part of the public API.

Removing extra classes added by earlier changes: - Using Actions now - Using the RepositoryHandler

Also fixing issue Luke pointed out with Action<X> should be Action<? super X>.

PluginId was pulled into an interface so that we can control the creation of those instances.

Added notFound(reason) to the API per Stefan's suggestion.

ethankhall · 2017-01-19T01:23:55Z

@eriwen I fixed the merge conflict on this guy.

PR: #883

eriwen · 2017-01-21T03:45:05Z

This was rebased and merged onto master with tests fixed up by #1226. Closing as delivered.

PR: #883

1) PluginResolutionServiceResolver was renamed to PluginPortalResolver, which broke tests that set the url for the portal. 2) We need to make sure to move the buildscript repositories after the plugin repositories

This reverts commit a1f3919.

This reverts commit e996ec7.

oehme · 2017-02-07T12:56:49Z

Closing as superseded by #1343

eriwen added a:feature A new functionality from:contributor labels Nov 17, 2016

eriwen assigned eriwen and ldaley Nov 17, 2016

oehme suggested changes Nov 29, 2016

View reviewed changes

ldaley previously requested changes Nov 30, 2016

View reviewed changes

ldaley reviewed Nov 30, 2016

View reviewed changes

subprojects/core/src/main/java/org/gradle/plugin/repository/RuleBasedPluginRepository.java Outdated

Copy link

Contributor

ldaley Nov 30, 2016

Choose a reason for hiding this comment

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

Should be Action<? super X>

ethankhall mentioned this pull request Nov 30, 2016

Making the pluginRepositories avaliable in the init scripts #952

Closed

eriwen added the waiting-for:gradle-team label Dec 5, 2016

ethankhall added 12 commits January 18, 2017 17:21

Working on the changes per Stefan's request.

603c140

Moving PluginId to be a public Interface, and then created an implementation called DefaultPluginId Added the first phase of interfaces (inside the class, will move out later) so that I can start working on the API under the hood.

Refactored names of classes, and finished up api

37633d5

- Moved PluginRequest to InternalPluginRequest - Created PluginRequest that InternalPluginRequest extends - Made the code compile for RuleBasedPluginRepository

Code cleanup & fixing compilation

3b2bd7e

Removing the interface and renaming impl

1e15cb8

Removed PluginId interface and made DefaultPluginId be a class that we share and part of the public API.

Applying Stefan's Comments

37c5f26

Removing extra classes added by earlier changes: - Using Actions now - Using the RepositoryHandler

Removing code that should have be removed earlier

fd1b855

Public interface uses rules(...) to make the api more unwiledy

a9eb555

Also fixing issue Luke pointed out with Action<X> should be Action<? super X>.

Moved PluginId to an interface & docs

fb10da3

PluginId was pulled into an interface so that we can control the creation of those instances.

Added more docs

8bd6aea

Fixing use pf PluginId when it should have been DefaultPluginId

10addaa

Adding tests and fixing a bug

5564a73

Fixing isolated classpath, added test.

f5c09f4

Added notFound(reason) to the API per Stefan's suggestion.

eriwen added a commit that referenced this pull request Jan 20, 2017

Add @SInCE annotations for new public APIs

d224848

PR: #883

eriwen added a commit that referenced this pull request Jan 20, 2017

Ignore a couple tests while we get them fixed up from #883

8cb8d42

eriwen closed this Jan 21, 2017

eriwen added was:delivered and removed a:feature A new functionality waiting-for:gradle-team labels Jan 21, 2017

eriwen added a commit that referenced this pull request Jan 22, 2017

Add @SInCE annotations for new public APIs

02ae8d7

PR: #883

eriwen added a commit that referenced this pull request Jan 22, 2017

Ignore a couple tests while we get them fixed up from #883

e996ec7

eriwen added a commit that referenced this pull request Jan 24, 2017

Revert "Fixing test issues with #883"

76997cf

This reverts commit a1f3919.

eriwen added a commit that referenced this pull request Jan 24, 2017

Revert "Ignore a couple tests while we get them fixed up from #883"

b3e4f09

This reverts commit e996ec7.

eriwen reopened this Jan 24, 2017

eriwen mentioned this pull request Feb 3, 2017

Allow specifying global buildscript classpath repo from init script for script plugins #1323

Closed

ethankhall mentioned this pull request Feb 7, 2017

Refactor to pluginManagement DSL #1343

Closed

oehme closed this Feb 7, 2017

First pass at adding a custom plugin portal #883

First pass at adding a custom plugin portal #883

Uh oh!

Conversation

ethankhall commented Nov 17, 2016

Uh oh!

ldaley commented Nov 18, 2016

Uh oh!

ethankhall commented Nov 18, 2016

Uh oh!

ldaley commented Nov 25, 2016

Uh oh!

ethankhall commented Nov 25, 2016

Uh oh!

oehme commented Nov 25, 2016

Uh oh!

ethankhall commented Nov 25, 2016

Uh oh!

oehme commented Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethankhall commented Nov 28, 2016

Uh oh!

oehme commented Nov 28, 2016

Uh oh!

ethankhall commented Nov 28, 2016

Uh oh!

ethankhall commented Nov 29, 2016

Uh oh!

ethankhall commented Nov 29, 2016

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!

oehme commented Nov 29, 2016

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!

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!

ldaley commented Nov 30, 2016

Uh oh!

ethankhall commented Nov 30, 2016

Uh oh!

oehme commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethankhall commented Jan 19, 2017

Uh oh!

eriwen commented Jan 21, 2017

Uh oh!

oehme commented Feb 7, 2017

Uh oh!

Reviewers

Assignees

oehme commented Nov 26, 2016 •

edited

Loading

oehme commented Nov 30, 2016 •

edited

Loading