liquibase / liquibase-gradle-plugin

A Gradle plugin for Liquibase
Other
200 stars 60 forks source link

liquibase-gradle-plugin should declare picocli as a transitive dependency #129

Open mr-serjey opened 1 year ago

mr-serjey commented 1 year ago

This issue is my reaction to the following note from README.md...

IMPORTANT: Liquibase now uses the picocli library to parse options, but for some reason that library isn't a transitive dependency of Liquibase itself, so if you want to use this plugin with Liquibase 4.4.0+, you'll have to add the liquibaseRuntime 'info.picocli:picocli:4.6.1' dependency to your build.gradle file.

Liquibase supports multiple integration interfaces defined in liquibase-core.jar in liquibase.integration package:

I believe, developers of liquidate-core expected the consuming projects to define necessary dependencies for the integration type of consumer's choice, rather than providing them transitively. (e.g. I would be surprised to receive picocli, spring and servlet api from liquibase-core).

So I believe since liquibase-gradle-plugin has integration via commandline interface, it is on 'liquibase-gradle-plugin' to provide necessary dependencies (e.g. picocli) as transitive dependencies.

I would recommend to define picocli dependency in liquibaseRuntime on the plugin level, or adopt ant integration interface.

stevesaliman commented 1 year ago

Your explanation of why Liquibase doesn't have picocli as a transitive dependency makes sense. Why bring it if you don't need it?

The gradle plugin didn't bring picocli in as a transitive dependency for a similar reason. The plugin isn't tied to any particular version of Liquibase, and there is no need to bring in a dependency if you're not using a version of Liquibase that requires it.

At this point, I think enough people have moved to a new enough version of Liquibase that it might make sense to just have it, but which version should it include? the plugin still doesn't know what version of LB you want, so it also doesn't know what version of picocli it depends on.

mr-serjey commented 1 year ago

I believe ideally the first move should be done on liquibase-core side:

  1. Following best practices the liquibase-core should expose optional dependency to an appropriate version of picocli (they even considered that here)
  2. Alternatively the should start publishing liquibase-cli into a public maven repository (together with the rest publications)
  3. Or create and publish liquibase BOM

Here is the issues that people already filed in support of these ideas:

If/when these changes done, the changes in liquibase-gradle-plugin are straight forward.

I can think of few alternatives, but they require the maintenance of the picocli to liquiba-core version mapping on liquibase-gradle-plugin side (which is far from ideal, but probably can be considered to support already published versions of liquibase-core)

landsman commented 1 year ago

The mention of another dependency in the documentation also surprised me. I would like to have it straight as a liquibase-gradle-plugin dependency and not really worry about it.

stevesaliman commented 1 year ago

The challenge is that the liquibase-gradle-plugin doesn't know what version of Liquibase you want to use, so it also doesn't know what version of picocli it needs, or even if it is needed.

Maintining mappings of Liquibase releases to the version of picocli needed means that I'd potentially need a new release of the plugin each time Liquibase had a new release - or at least whenever a new version of picocli was used. Less than ideal.

landsman commented 1 year ago

But you are talking about versions where there is some BC break, right? It's still a much better solution than letting this effort to users, to be honest.

filipelautert commented 1 year ago

@stevesaliman from the discussion here, I believe PR https://github.com/liquibase/liquibase/pull/5042 that will be included in the next liquibase release and adds picocli as an optional dependency could help on that.

filipelautert commented 1 year ago

@stevesaliman as per Florent closing comment on his own request (https://github.com/liquibase/liquibase/issues/2751#issuecomment-1792701463 ) , seems that replacing liquibase-core by liquibase-cli as a dependency resolves this. What do you think?

stevesaliman commented 1 year ago

The thought that comes to my mind is this: Does liquibase-cli include the classes from liquibase-core? The plugin doesn't necessarily need them, but the Groovy DSL will in order to parse groovy changes.

But if the cli includes the core, that would be a good solution for the plugin.

filipelautert commented 1 year ago

@stevesaliman liquibase-cli depends on liquibase-standard (that is core minus snowflake) . Pom is here: https://github.com/liquibase/liquibase/blob/master/liquibase-cli/pom.xml

But I just noticed that we don't publish liquibase-cli to maven repo... so until that is changed it won't help :/