Reinstate Asciidoctor configuration

[wilkinsona]

I've just become aware of the asciidoctor configuration being removed in the latest versions of the Gradle plugin. I'd like to see if it could be reinstated as I think its removal is a small step backwards for users that want to use extensions. They now need to create a custom configuration and then configure the asciidoctor task to use it. This is quite a bit of boiler plate that was avoided in previous releases. I can't see a benefit for users that are not using extensions that offsets the cost for users that are. I've looked at this commit where, I think, the configuration was removed, but haven't been able to discern the reasoning. Apologies if I have missed something.

In addition to now requiring additional boilerplate, I think the removal of the asciidoctor configuration also makes this plugin less idiomatic. It's typical for Gradle plugins to have a configuration that allows dependencies to be added to their tasks' classpath. For example, Gradle's own Checkstyle plugin has a checkstyle configuration for this purpose.

If you are in agreement with the configuration being reinstated, I would be happy to try and contribute a pull request that does so.

[ghost]

Thanks @wilkinsona. I am unable to move to version 2.x of the Gradle plugin as I rely heavily on spring-asciidoctor-extensions.

As you say, I could "create a custom configuration and then configure the asciidoctor task to use it." Any instructions on how to do that would be very welcome.

[wilkinsona]

The proposal I've made here won't help you there, I'm afraid. What you need is for me to fix https://github.com/spring-io/spring-asciidoctor-extensions/issues/9 that you opened. Now that I've tackled https://github.com/spring-projects/spring-restdocs/issues/581, I can make a similar change to spring-asciidoctor-extensions if necessary.

[aalmiray]

Instructions on how to set an additional configuration can be found in the README of this project.

I believe the reason to remove the asciidoctor configuration is to avoid clashes between a configuration and a task having the same name. The checkstyle plugin does not have this problem because it creates a task per sourceSet, its name following a naming convention. I don’t think is advisable for the plugin to go back however users can create an asciidoctor configuration at their own peril.

[wilkinsona]

It wouldn't have to be named asciidoctor to enjoy the benefits of reduced boilerplate. I'd just like to see a configuration provided by default that can be used to add dependencies to the task's classpath. As long as that configuration's name is documented and well-known, the exact name doesn't really matter.

[aalmiray]

The boilerplate in question is then

configurations {
    asciidoctorExt
}

When a configurations block does not exist (3 lines) vs. 1 line when it already does. That’s not much of a gain if an implicit configuration is added, right? Plus, this requires automatic wiring of this configuration to every Asciidoctor task, something we want to avoid due to classpath issues.

OTOH having a provided configuration allows power users to hook into the magic, but can trip new users.

I think it’s better to keep configurations explicit.

[ysb33r]

I believe the reason to remove the asciidoctor configuration is to avoid clashes between a configuration and a task having the same name

@aalmiray is correct. Under certain conditions this will cause a problem. Not everybody sees this, but the plugin ecosystem is now quite complex and has been written to maintain compatibility over a large range of Gradle versions.

It wouldn't have to be named asciidoctor to enjoy the benefits of reduced boilerplate.

I think we can help you on that, but I have to think about a good solution. (and still keep with what @aalmiray said in https://github.com/asciidoctor/asciidoctor-gradle-plugin/issues/403#issuecomment-503479886)

I have also looked at the example that @jnizet has posted in https://github.com/spring-projects/spring-restdocs/issues/581#issuecomment-503443490. That is a concrete example to work from. Some of the issues will be covered in #404 instead.

[wilkinsona]

When a configurations block does not exist (3 lines) vs. 1 line when it already does.

I'd argue that it's 6 lines. I would expect the implicit configuration to be used by the asciidoctor task by default, which saves something roughly like the following:

asciidoctor {
    configurations asciidoctorExt
}

Plus, this requires automatic wiring of this configuration to every Asciidoctor task, something we want to avoid due to classpath issues

Need it be wired into every Asciidoctor task? I think it would be reasonable to only wire it into the default asciidoctor task, leaving users with separate control of the classpath of any tasks that they create themselves.

[ysb33r]

@wilkinsona Where is the source of the spring-restdocs gradle plugin? I have an idea but I would like to see what the best implementation would be.

[wilkinsona]

REST Docs doesn't have a Gradle plugin. It does have an Asciidoctor extension, however. Its source is in the various spring-restdocs-asciidoctor* modules here. We then document how to use that extension by adding a dependency upon it to the asciidoctor configuration from the older org.asciidoctor.convert plugin.

[ysb33r]

Aha, so it just an extension that has to end up on the classpath.

As I don't want to mess with the standard line of Gradle plugins with this, I might have a better solution to deal with this, it just depends on where it is going to find a home.

Assuming that there is a plugin called org.asciidoctor.jvm.spring-restdoc-conventions (or something spring-like), it can effectively do the equivalent of the following when applied

apply plugin : 'org.asciidoctor.jvm.convert'
apply plugin : 'org.asciidoctor.jvm.pdf'

configurations {
    asciidoctorSpring
}

dependencies {
    asciidoctorSpring "org.springframework.restdocs:spring-restdocs-asciidoctor:${SpringVersion}"
}

asciidoctor {
  configurations 'asciidoctorSpring'
  t.outputDir "${buildDir}/docs/html5"
  baseDirFollowsSourceDir()
}

asciidoctorPdf {
  configurations 'asciidoctorSpring'
  t.outputDir "${buildDir}/docs/pdf"
  baseDirFollowsSourceDir()
}

This could either live somewhere in Spring, or maybe there can a plugin for it within Asciidoctor Gradle. As I said it will need a home somewhere.

(I added PDF as well as someone mentioned it in the passing).

[wilkinsona]

AFAIK, the output dir and base dir follows source dir customisation are specific to @jnizet's needs. They aren't something that Spring REST Docs needs in general. The benefits of having an implicitly created configuration that can be used to add dependencies to the asciidoctor task extend beyond REST Docs. As such, I think it would be a shame of those benefits were only available by applying a REST Docs-specific plugin. I'd rather see a solution that's available to everyone that uses the new Asciidoctor Gradle plugin by it reinstating a (modified as needed) equivalent of the old plugin's behaviour.

[ysb33r]

The reason why we don;t want single global configuration is all about classpath pollution.

As a simple example, if anyone drops something on the classpath that has a snakeyaml transitive dependency, there is a good chance that it will break PDF generation. What happily works for one backend, might not work for another.

In addition, since 2.x, build script authors now have to option of using Gradle workers instead of an external JVM process(*). Gradle workers are very sensitive to classpaths - not our fault, it is due to an issue in Gradle.

Binary extensions and custom backends are about the only reason why an additional configuration is still needed. The reason why @aalmiray and I are loathe to just add the implicit configuration back is that it removes the explicit notion of saying here is an additional configuration that will be required.

Unfortunately there is the drawback that it affects some users, especially now a decent base such as for spring-restdocs.

Having said all of that we do add special configurations already. People who need to use additional GEMs that are not already packed as JARs now has to apply the org.asciidoctor.jvm.gems plugins which will among things also add the asciidoctorGems configuration. This is very explicit in its intent that only GEMs (and maybe JARs required by GEMs) should be added to it.

To some extent this is what I suggested using a special contention for spring-restdocs, whereby the configuration is called asciidoctorSpring, therefore declaring that the configuration is for use of dependencies needed for Spring & Asciidoctor. Whereas something like compile or testCompile is very programmer-focused and the likelyhood is that many dependencies are required to build the product, when building documents we are not concerned with what the required dependencies - they are simply infrastructure. Therefore managing thgem out of the way in some form is beneficial. That is among the reason that since 2.x of the Gradle plugins one no longer has to concern oneself with what version of the PDF, EPUB etc. backend to use. It is just a case of stating that PDF generation is needed and the plugin will supply a reasonable up to date version(**).

Having looked at some of the Spring examples now, I think a small conventions plugin could also strip a number of lines out of those Gradle scripts.

(*) We had to add an external process in 1.5 because Gradle daemon did not work with the way extensions were handled in earlier versions of the plugin. (**) It is possible to explicitly state the version, but most people don't care.

[wilkinsona]

Having looked at some of the Spring examples now, I think a small conventions plugin could also strip a number of lines out of those Gradle scripts.

Yeah, they certainly could. It's an idea that I've toyed with for a while but have never done anything about. Perhaps this can give me the necessary impetus to do something. For simplicity's sake, I could just target the new Asciidoctor Gradle plugin. I've opened https://github.com/spring-projects/spring-restdocs/issues/625.

[ysb33r]

Here is an example of what a conventions plugin could look like. I just put it on a branch of mine and added it as an extra plugin in the Asciidoctor Gradle project.

https://github.com/ysb33r/asciidoctor-gradle-plugin/tree/spring-restdocs/asciidoctor-gradle-jvm-spring-conventions/src/main/groovy/org/asciidoctor/gradle/jvm/spring

[wilkinsona]

Very interesting, thank you. Just to be clear, I'd really want any such plugin to be part of Spring REST Docs. I doubt that you intended anything different, but just wanted to be sure to avoid any confusion.

[jnizet]

I had a quick look at the example plugin, and although I find it nice to correctly set the inputs and outputs of the asciidoctor task, I would be wary of being too opinionated. For example, assuming that the test task is what generates the snippets, and that the generated docs should be in the jar would be too much for me.

[ysb33r]

Very interesting, thank you. Just to be clear, I'd really want any such plugin to be part of Spring REST Docs. I doubt that you intended anything different, but just wanted to be sure to avoid any confusion.

Great. I'll leave it there as a reference point. Just mention me on the spring-projects/spring-restdocs#625 issue when you have something ready and can to see whether it will be compatible with both 2.x & 3.x Gradle plugins.

[wilkinsona]

@jnizet Could you comment on https://github.com/spring-projects/spring-restdocs/issues/625, please? FWIW, I agree about not including the docs in the jar by default.

[ysb33r]

@wilkinsona Can I close this issue?

[wilkinsona]

I would still prefer that a configuration be created by default. It could only be wired into the asciidoctor task to provide complete freedom for user-created tasks. It need not be named asciidoctor to avoid a clash with the task's name. It may only save a handful of lines but, IMO, that's what convention over configuration is all about and the small individual savings soon mount up into something much bigger.

[ysb33r]

I revisited this today to see how feasible it will be in 3.0, but there are a number of opposing designs, none of which are a clear winner:

Single configuration: Single configuration used by all org.asciidoctor.gradle.jvm.AbstractAsciidoctorTask subclasses. Easier for user to configure, but when some task instances do not need the configuration, then more DSL will be required. For instance if an extension is only applicable to HTML generation, then the PDF task do not want it on the classpath.

One configuration per task instance: Thus asciidoctorClasspath to the asciidoctor task and asciidoctorPdfClasspath for the PDF task. This solves the previous problem, but if all tasks want tthe same configuration, then either

asciidoctorPdf {
    configurations = ['asciidoctorClasspath`]
}

OR

configurations {
    asciidoctorPdfClasspath.extendsFrom asciidoctorClasspath
}

will be required.

Lazy creation of configurations: Although the following implementation is possible

tasks.all { Task t ->
   if(t instanceof org.asciidoctor.gradle.jvm.AbstractAsciidoctorTask) {
     configurations.create( "${t.name}Classpath" )
   }
}

it will eagerly create all tasks even if not required. I don;t think there is currently a method on the NamedDomainContainer to add rules when objects are registered (not created). It is possible to work around this by creating a configuration for each known task within the plugin, but it does not allow for additional configurations to be automatically created when new tasks are added.

I'm still not the wiser as to the best solution to this.