Facilitate using the plugin for custom reports

[nafg]

See #119

I think to avoid needing a special SBT configuration I would need to make similar changes in sbt-mima :(

But so far this PR enables the following diff to the Slick PR (again, see #119):

diff --git a/project/Versioning.scala b/project/Versioning.scala
index 457183cc..1f7ae653 100644
--- a/project/Versioning.scala
+++ b/project/Versioning.scala
@@ -1,16 +1,15 @@
 import com.typesafe.tools.mima.plugin.MimaKeys.mimaFindBinaryIssues
 import com.typesafe.tools.mima.plugin.MimaPlugin
 import coursier.version.Version
-import sbt.librarymanagement.CrossVersion
-import sbt.{AutoPlugin, Compile, Def, Defaults, Keys, config, inConfig, settingKey, taskKey}
+import sbt.{AutoPlugin, Compile, Defaults, Keys, config, inConfig, settingKey, taskKey}
 import sbtdynver.DynVerPlugin.autoImport.{dynver, dynverCurrentDate, dynverGitDescribeOutput}
 import sbtdynver.{DynVer, GitDescribeOutput}
 import sbtversionpolicy.SbtVersionPolicyMima.autoImport.versionPolicyPreviousVersions
-import sbtversionpolicy.SbtVersionPolicyPlugin.autoImport._
-import sbtversionpolicy.{DependencyCheckReport, Direction, SbtVersionPolicyMima, SbtVersionPolicyPlugin}
+import sbtversionpolicy.SbtVersionPolicyMima.previousVersionsFromRepo
+import sbtversionpolicy.SbtVersionPolicyPlugin.autoImport.{versionPolicyCheckDirection, versionPolicyDependencyIssuesReporter, versionPolicyIntention}
+import sbtversionpolicy._
 
 import java.util.Date
-import scala.jdk.CollectionConverters._
 
 
 object Versioning extends AutoPlugin {
@@ -49,31 +48,6 @@ object Versioning extends AutoPlugin {
   def versionFor(compat: Compatibility, date: Date = new Date): Option[String] =
     DynVer.getGitDescribeOutput(date).map(versionFor(compat, _))
 
-  // Code copied from sbt-version-policy
-  private lazy val previousVersionsFromRepo = Def.setting {
-    val projId = Keys.projectID.value
-    val sv = Keys.scalaVersion.value
-    val sbv = Keys.scalaBinaryVersion.value
-    val name = CrossVersion(projId.crossVersion, sv, sbv).fold(projId.name)(_ (projId.name))
-
-    val ivyProps = sbtversionpolicy.internal.Resolvers.defaultIvyProperties(Keys.ivyPaths.value.ivyHome)
-    val repos = Keys.resolvers.value.flatMap { res =>
-      val repoOpt = sbtversionpolicy.internal.Resolvers.repository(res, ivyProps, s => System.err.println(s))
-      if (repoOpt.isEmpty)
-        System.err.println(s"Warning: ignoring repository ${res.name} to get previous version")
-      repoOpt.toSeq
-    }
-    // Can't reference Keys.fullResolvers, which is a task.
-    // So we're using the usual default repositories from coursier here…
-    val fullRepos = coursierapi.Repository.defaults().asScala ++ repos
-    val res = coursierapi.Versions.create()
-      .withRepositories(fullRepos: _*)
-      .withModule(coursierapi.Module.of(projId.organization, name))
-      .versions()
-    res.getMergedListings.getAvailable.asScala
-  }
-
-
   override def trigger = allRequirements
 
   val previousRelease = settingKey[Option[String]]("Determine the artifact of the previous release")
@@ -136,7 +110,18 @@ object Versioning extends AutoPlugin {
             versionPolicyCheckDirection := Direction.both,
             versionPolicyIntention := BumpMinor,
             reportCompat := {
-              val dependencyIssues = versionPolicyFindDependencyIssues.value.toMap
+              val dependencyIssues =
+                versionPolicyDependencyIssuesReporter.value
+                  .apply(
+                    BumpMinor,
+                    SbtVersionPolicySettings.fromMimaArtifacts(
+                      SbtVersionPolicyMima.computePreviousArtifacts(
+                        Keys.projectID.value,
+                        previousRelease.value.toList
+                      )
+                    )
+                  )
+                  .toMap
               val mimaIssues = mimaFindBinaryIssues.value.map { case (module, problems) =>
                 module.withName(module.name.stripSuffix("_" + Keys.scalaBinaryVersion.value)) -> problems
               }

[julienrf]

Thank you for the PR @nafg !

I wonder why versionPolicyFindDependencyIssues is not enough?

I believe what we should do is to introduce new tasks similar to what versionPolicyFindDependencyIssues is to versionPolicyReportDependencyIssues, but for the tasks versionPolicyMimaCheck and versionPolicyCheck. They could be named versionPolicyFindMimaIssues, and versionPolicyFindIssues, for instance?

[nafg]

Thank you for the PR @nafg !

I wonder why versionPolicyFindDependencyIssues is not enough?

Sorry if I wasn't clear. The goal is to be able to choose the mode on the fly. That is, versionPolicyIntention is set in build.sbt to Compatibility.None because your next version is 4.0.0 so running versionPolicyReportDependencyIssues correctly does not fail the build, yet without having to modify build.sbt or rely on a bash script that calls sbt with set versionPolicyIntention ..., I want to be able to run something like versionPolicyFindDependencyIssues and get the levels of incompatibility of all breaking dependency changes, to generate user migration information.

If it already does that then I really missed the boat somehow. In that case I only need the Make previousVersionsFromRepo public commit, and lightbend-labs/mima#680.

If doesn't, but it's possible to refactor things so that versionPolicyFindDependencyIssues always returns everything and only versionPolicyReportDependencyIssues differentiates the types of issues to report based on the mode, that would be simpler, but a breaking change to the semantics of sbt-version-policy, and might require deeper knowledge of the codebase than I have to pull off.

I'm not sure I remember 100% why I wanted to expose the previous artifacts logic so I could specify it explicitly in my code. I can revisit whether that's really necessary.

[nafg]

I believe what we should do is to introduce new tasks similar to what versionPolicyFindDependencyIssues is to versionPolicyReportDependencyIssues, but for the tasks versionPolicyMimaCheck and versionPolicyCheck. They could be named versionPolicyFindMimaIssues, and versionPolicyFindIssues, for instance?

That would be nice to do, but it doesn't address the goal of this PR.

Maybe the title of the PR is unclear (I just edited it but still), since it is broken into a few distinct commits, each of which could in theory be their own PR (but they all work towards the same goal). The commit messages explain better, perhaps. For instance, here's a relevant one:

versionPolicyDependencyIssuesReporter task 

It provides a DependencyCheck.Reporter, which enables passing an arbitrary
compatibility intention and previous module IDs, while still computing
everything else from SBT.

Then, versionPolicyFindDependencyIssues is refactored to use
versionPolicyDependencyIssuesReporter, and only supply the compatibility
intention and previous module IDs to it, based on SBT settings as before.

I think in a way this comes back to what I said elsewhere, that providing functionality in an SBT-coupled way is very limiting. SBT is basically a functionally-implemented runtime (task engine) with an immutable AST (tasks and settings) for a language that strongly encourages global mutable state.

The fact that the behavior of versionPolicyFindDependencyIssues is tied to the value of versionPolicyIntention is analogous to the action-at-a-distance of global mutable state in other programming languages. And the fact that its behavior is only accessible by running an SBT task means it can only be used with that paradigm.

This is not a criticism of sbt-version-policy at all. It's the "default" that SBT effectively encourages. It definitely takes more work to do things differently. I'm just sharing how I see the thing that I'm struggling to achieve as part of a bigger picture.

My solution was to introduce a "named partially applied function" -- that is, you get an object with some of the parameters filled out, and you can then pass in the rest yourself. I could have just make the report method public, but it takes a LOT of parameters, and most of them are just being inferred from the build, IIRC.

I guess in a sense, in the context of an SBT plugin, settings can be divided into two categories. There are some which are a mechanism for introspecting the build (predefined settings), and some which are the knobs and levers for controlling the behavior of the plugin. Perhaps a pattern which could be helpful in general for plugins to solve this sort of issue, is to define two case classes, each of which combines the setting values of the respective category. Instances of these case classes would themselves be a convenience Setting. Then, the task implementation should be exposed as a regular method that takes an instance of the two settings, and the actual SBT Task would simply depend on the convenience settings and call the implementation method. Then users can call into the implementation with their own values and while getting the combined predefined settings easily.

IOW,

// A plugin
case class SbtInfoNeedByThisPlugin(x...)
case class PluginParams(...)

def taskImpl(sbtInfoNeedByThisPlugin: SbtInfoNeedByThisPlugin, params: PluginParams) = ???

...

sbtInfoNeedByThisPlugin := {
  val x = xSetting.value
  ...
  SbtInfoNeedByThisPlugin(x, ...)
},
pluginParams := ...,
task := {
  taskImpl(sbtInfoNeedByThisPlugin.value, pluginParams.value)
}

// User build that wants to use `taskImpl` with different `PluginParams` than specified in the build
taskImpl(sbtInfoNeedByThisPlugin.value, pluginParams.value.copy(...))

Just a random thought...

[nafg]

Maybe I should just forget this whole thing and accept that SBT's solution to "global state" is configurations and stick to my original version of the Slick PR which has to introduce a new configuration...

[nafg]

A use case for specifying the artifacts or version on the fly is e.g. if I want to find out how a PR compares to its merge base. For example maybe I want to automatically label all the PRs that contain breaking changes. There can be many PRs in a breaking release, some of which don't contain breaking changes but running mima against the last release would indicate that breaking changes were found, but that doesn't tell me if there are breaking changes within the PR itself.

[julienrf]

Thank you for the thorough explanation. Now it’s clearer to me.

I think in a way this comes back to what I said elsewhere, that providing functionality in an SBT-coupled way is very limiting

I agree. I would need more time to think about a better way to work with the design of sbt… I think the approach you found (with a separate config) is interesting.

I don’t think we should merge your “reporter” proposal for now, however I am happy to add *FindIssues settings if that helps you.

[nafg]

Are you saying that none of the 3 commits in this PR (not counting the one that updates versionPolicyIntetion) are changes you are willing to accept? Any of them individually would be helpful to me (and I believe a lot of people too, once they realize they no longer have to use mima filters as a crutch and that automatically generating the list of breaking changes in a breaking release is possible).

Could you at least share why you reject them?

[julienrf]

@nafg Actually I am not sure. I agree that it would be very useful to expose the functionality of this sbt-plugin in a way that can be used outside of the usual sbt builds (including alternative build tools). I think we need to think a little bit about how to do it well.

I guess in a sense, in the context of an SBT plugin, settings can be divided into two categories. There are some which are a mechanism for introspecting the build (predefined settings), and some which are the knobs and levers for controlling the behavior of the plugin.

Here, how would you classify the settings of sbt-version-policy?

[nafg]

@nafg Actually I am not sure. I agree that it would be very useful to expose the functionality of this sbt-plugin in a way that can be used outside of the usual sbt builds (including alternative build tools). I think we need to think a little bit about how to do it well.

Now I'm worried that my philosophical "rant" made the objective of this PR, less clear, not more :(

The goal of this PR has nothing to do with that. I only lamented that if SBT didn't encourage a very SBT-specific (global-state-like) way of doing things, this PR wouldn't even be necessary. But other than that "how to make this build-tool agnostic" is a distraction from the goal here, and I'm sorry for causing confusion.

I guess in a sense, in the context of an SBT plugin, settings can be divided into two categories. There are some which are a mechanism for introspecting the build (predefined settings), and some which are the knobs and levers for controlling the behavior of the plugin.

Here, how would you classify the settings of sbt-version-policy?

I guess I was unclear again. :(

If you look at the code of versionPolicyFindDependencyIssues (before the refactor of 7dc19ac) it reads a LOT of SBT settings. But most of those are predefined, not defined by the plugin. In other words the reason the task looks at those settings is because it wants to populate the parameters of DependencyCheck.report with information about the build that it introspects from SBT.

Then there are settings that the plugin defines, in its own autoImport section. These are "inputs" to the plugin, its own knobs and levers to control it. And currently the only way to set them is in the SBT way -- global state that applies both to the Find task and the Report task, and is meant to be set to values such that the latter fails the build if things aren't correct, thus preventing using the Find task to learn about breaking changes that are allowed (because this is a breaking release).

What 7dc19ac does is to let you keep the "global" (within the SBT scope of course, but that's the one we're in) settings in place, and still give other queries (other intents and/or artifacts) on the fly (as parameters in code, not by what's effectively the side effect of reading mutable state set elsewhere). So I could define one custom task to get all changes since the last release by passing in BinaryAndSourceCompatible, and another custom task to check the breakingness of a single commit or PR by passing in custom artifacts, without needing something "outside" the task to set settings to different values with whatever ramifications that has.

The other commits are just making some "business logic" available directly.

[nafg]

Do you feel that any of these changes degrade the codebase quality? Or are you worried about the cost of maintaining additional public APIs? Or do you have some other concern?

[julienrf]

My main concern is maintaining additional public APIs.

I think I understood well your goal, maybe I didn’t express myself clearly :) I think it would indeed be useful to be able to get an incompatibility report independent of an sbt build, not only to support alternative build tools, but also to produce incompatibilitiy logs like you would like to do. I both cases, we want to be able to call the logic of versionPolicyCheck with different parameters than those defined in an sbt build (at least some of them, not all of them, as you noticed).

[nafg]

Ok so what's the plan? If I merge the Slick PR with the inelegant separate-SBT-configuration solution, I will feel a tiny bit disappointed but then probably have higher priorities than following up on this at all. Or I could leave it open, if there is a solution that can happen soon, and then update the PR to use it.

Should I wait for something better to happen, or should I give up on this effort altogether?

My main concern is maintaining additional public APIs.

Does that mean you don't feel my use case justifies additional APIs, or do you mean that these additional APIs aren't polished enough to go in? If the latter is there anything you would accept short of the huge refactor that we touched on?

[julienrf]

Ok so what's the plan?

I would like to take a bit more time to think about it.

Does that mean you don't feel my use case justifies additional APIs, or do you mean that these additional APIs aren't polished enough to go in? If the latter is there anything you would accept short of the huge refactor that we touched on?

The latter. I think your use case makes a lot of sense, and we should support it in sbt-version-policy. I just need more time to think about it.

[nafg]

Ok thanks. I'll leave the Slick PR open and work on other things meanwhile, until I hear an update on this (or it becomes necessary to move forward without this)

[nafg]

Hi any update?

[julienrf]

Hey, sorry for being silent for so long!

I am interested in merging this feature. Overall, I think the design is good, I have left minor comments.

Do you mind adding a bit more documentation in the code, and using more self-explanatory identifiers? (I know this is currently not the case everywhere but I would like to enforce this convention going forward)

Also, would you be interested in adding some documentation in the README?

Should we have something similar to report binary and source incompatibilities, BTW?

[julienrf]

Suggested change

	lazy val previousVersionsFromRepo = Def.setting {
	private[sbtversionpolicy] lazy val previousVersionsFromRepo = Def.setting {

[julienrf]

Suggested change

	def fromMimaArtifacts(artifacts: Set[sbt.ModuleID]) = {
	private def fromMimaArtifacts(artifacts: Set[sbt.ModuleID]) = {

[julienrf]

Suggested change

	sv: String,
	scalaVersion: String,

[julienrf]

Would you mind adding some documentation?

[nafg]

Hi I probably won't be able to look into this until after Passover. Also at this point I would have to rewrite the code in Slick in tandem. I will try to remember to do it then.

[nafg]

Should we have something similar to report binary and source incompatibilities, BTW?

Dale recently merged and released the PR that I opened in sbt-mima for that purpose

If you have a vision of a more unified API go for it...

[julienrf]

FTR, we added several features that will facilitate the generation of compatibility reports, especially the task versionPolicyExportCompatibilityReport (in #189). At the time of writing, the reports only show the level of compatibility but they could be expanded to also explain which issues broke the compatibility (see

sbt-version-policy/sbt-version-policy/src/main/scala/sbtversionpolicy/CompatibilityReport.scala

Lines 56 to 57 in 9cc7721

	// TODO add issue details
	// "issues" -> ujson.Obj("dependencies" -> ujson.Arr(), "api" -> ujson.Obj())

).