Can parse Plugin Repository declarations in settings.gradle

[oehme]

Motivation

To allow users to specify their custom repositories, Gradle first of all needs to be able to parse the new pluginRepositories DSL.

Proposed Change

Add a pluginRepositories {} block to the DSL of settings.gradle. The pluginRepositories block is parsed in the same phase as the buildScript block, before the remaining instructions in the settings file are parsed. A pluginRepositories block is only allowed as a top-level element. It introduces new PluginRepositoriesHandler and PluginRepository interfaces which work similarly to the RepositoryHandler and ArtifactRepository interfaces.

The InitialPassStatementTransformer and BuildScriptTransformer classes will be updated to be aware of the special handling needed to deal with the pluginRepositories DSL block.

Test Cases

• pluginRepositories block can be read from settings.gradle
• pluginRepositories block supports defining a maven plugin repository
• pluginRepositories block is not supported in ProjectScripts
• pluginRepositories block must come before other blocks in the settings.gradle script
• pluginRepositories block must be a top-level block (not nested)
• Only one pluginRepositores block is allowed in each script

Code Review

• Does the change work?
  • Try out the feature, executing it like a end-user would.
  • Check corner cases and try to break it in various ways.
  • Are the error messages informative?
• Is the change sufficiently tested?
  • Are there appropriate levels of unit & integration tests?
  • Do the tests indicate what they are actually testing?
  • Are the tests in the right location?
  • Can new tests be consolidated with any existing tests?
• Does it change the public API?
  • Has @Incubating been used appropriately?
  • Does it preserve backward compatibility?
• Is it sufficiently documented?
  • Is the user guide documentation in the best location?
  • Is the user guide documentation clear and informative?
  • If there should be samples, are they helpful and tested appropriately?
  • Is there sufficient API/DSL documentation?
  • Is the API/DSL documentation clear and informative?
  • Do the release notes clearly convey the value of the feature?
  • Do the release notes explain the feature and link to more documentation?
• Is it implemented well?
  • Does it minimize surface area (particularly public)?
  • Does it follow our coding conventions? (note that we don't have coding conventions formalized but it's something to keep in mind)
  • Does it use appropriate data structures?
  • Does it minimize potential for invalid states?
  • Does it suitably reuse existing infrastructure?
  • Does it suitably consider concurrency?
  • Does it suitably consider failure modes?
  • Does it suitably consider performance?
  • Does it introduce new types or move existing ones? Are they located in the appropriate package? (e.g. internal classes in internal packages)

[eljobe]

@oehme I've updated the description of this issue with a rough implementation plan and test cases. Let me know if it needs more detail. I'm also sure I've missed a test case or two, but can't think of them right now.

[eljobe]

I love it when a test case that you thought of before coding things up actually ends up showing a problem in your implementation.

Specifically "pluginRepositories block must be a top-level block (not nested)"

The plugins block ensures this preprocessing all of the top-level plugins ScriptBlocks using an AST parser and then removing them from the script before the script is run by the script runner for the initial pass. This means that the Script implementation doesn't actually need or have a plugins function. So, any occurrences of calls to the plugins block nested inside other blocks, will result in a MethodNotFound exception which effectively means that plugins blocks are only allowed in the top-level of the scripts.

We didn't do the same thing for the pluginRepositories block because it has considerably more complex structure than the plugins block and it would have been quite difficult to introduce an analog to the PluginUseScriptBlockMetadataExtractor for processing the pluginRepositories block so that we could omit the pluginRepositories method from the Script implementations.

I'm going to spend a little time looking at the ASTUtils class to see if there is any way to detect a nested pluginRepositories block cheaply. I don't anticipate being able to do it short of parsing the entire script.

[oehme]

I know we initially set out with the intention of "only allow it once and at the top level", but that might conflict with #23 (allow users to inspect the plugin repositories, e.g. restrictive enterprises trying to lock down their builds).

What we do need to make clear is that only the top-level block is taken into account when discovering the plugin repositories. Any modifications that are made after the initial pass must result in an exception.

[eljobe]

@oehme I don't think this goal is inherently in conflict with #23. We would just have a different method for accessing the PluginRepositoryHandler later in the build in a read-only manner which would allow for inspection. Am I missing something?

[oehme]

We could add another method with a different name, yes. But I'd find it pretty confusing if the pluginRepositories are not available under pluginRepositories later.

[eljobe]

I see what you mean. Most of the analogous constructs in our API do function that way.

[oehme]

For relative paths, the user currently has to write

pluginRepositories {
  maven {
    url file ("../repo")
  }
}

The file call should not be necessary, as the url method should evaluate against the rootDir.

[eljobe]

Note to self:

• Remove extends NamedDomainObjectList<PluginRepository> and friends from the PluginRepositoryHandler hierarchy.

[oehme]

@DanielThomas You can now try out the new functionality in the examples.

[eljobe]

@DanielThomas We've changed our process a little bit. When you are happy with this feature, please just add the "accepted" label to it.

[eljobe]

@DanielThomas It would be great if you or someone from your team could try out this functionality in our sample projects and let us know if you are happy with it.

[DanielThomas]

@rspieldenner is our on call guy this week, so I'll punt that to him; but I'll take a look by the end of the day if he doesn't get a chance.

[DanielThomas]

Sorry we didn't get to this last week. Looking now.