lightbend-labs / mima

A tool for catching binary incompatibility in Scala
Apache License 2.0
459 stars 71 forks source link

"mimaPreviousArtifacts not set" check may be too strict #328

Closed raboof closed 5 years ago

raboof commented 5 years ago

The check introduced in #263 may be inconvenient when you're cross-building and only have previous artifacts for some of the upstream versions you are targeting.

raboof commented 5 years ago

Can that be fixed with something like mimaFailOnNoPrevious := scalaVersion.value != "2.13.0"?

If so perhaps documenting that option in the error message and the release notes might be sufficient?

dwijnand commented 5 years ago

Yeah, the release notes should (have) the details of the change in behaviour that shipped with #289 and the mitigation strategies that are described there:

dwijnand commented 5 years ago

Oh, and perhaps an example for pre-release Scala.

I'm not sure how much should be added to the error message... Maybe at least add a URL for more info?

dwijnand commented 5 years ago

call notes:

2.13.0-M4 -> 2.13

CrossVersion.partialVersion(scalaVersion.scala)

mimaFailOnNoPrevious in ThisBuild := CrossVersion.partialVersion(scalaVersion.scala) == Some((2, 13))

^ when cross-building to 2.13 before the project has released a 2.13, don't fail MiMa

https://twitter.com/dwijnand/status/1100075109719654405/photo/1

add docs to README about mimaFailOnNoPrevious and add link to it in error message

alexarchambault commented 5 years ago

Wouldn't it be possible to disable MimaPlugin by default (and have users explicitly call enablePlugins(MimaPlugin) when they need it)? Kind of like what was done recently for ScriptedPlugin. The check wouldn't be too strict that way, I think.

dwijnand commented 5 years ago

The problem is you can't enable or disable a plugin build-wide, so I prefer a setting-based approach.

So then it's a question of what the default should be, according to the most common case(s).

For scripted it makes sense to opt-in because 99% of builds have at most 1 sbt plugin project - either it's a multi-project build with 1 project being an sbt plugin or it's a 1 sbt plugin project build, in either case you only need to add scripted to 1 project (which, btw, should be done with enablePlugins(SbtPlugin), I recently figured out).

For MiMa I think it makes sense to opt-out instead.

dwijnand commented 5 years ago

I've reconsidered that that's not good enough.

There's been quite a fallout this change, see the impact:

https://github.com/pulls?q=author%3Ascala-steward+is%3Apr+%22sbt-mima-plugin+to+0.4.0%22

Also, have a look at https://github.com/lightbend/mima/pull/331/files where we have to use isScala213OrLater twice, once when defining mimaPreviousArtifacts and once to define mimaFailOnNoPrevious.

I think the problem stems from not being able, in mimaReportBinaryIssues, to determine whether mimaPreviousArtifacts is empty because it wasn't set or whether it was purposely set to the empty set. We should look to do this another way.

raboof commented 5 years ago

There's been quite a fallout this change

(I would have expected #299 to introduce many failures, which would be as intended - but indeed many of those seem to be due to "no previous artifacts" (too))

I think the problem stems from not being able, in mimaReportBinaryIssues, to determine whether mimaPreviousArtifacts is empty because it wasn't set or whether it was purposely set to the empty set

What if we don't set a default value for mimaPreviousArtifacts at all? That would break += but I suspect (and google seems to confirm) most people would be using := anyway?

dwijnand commented 5 years ago

I'm not a fan of not setting keys, but sbt does have ways in which that can be used.

I'm proposing the use of reference equality in #334 instead, which isn't the best of techniques either, but it fixes the issue (IMO).