Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
raboof opened this issue Jul 10, 2019 · 9 comments · Fixed by #330 or #334
Closed

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

raboof opened this issue Jul 10, 2019 · 9 comments · Fixed by #330 or #334
Assignees
Milestone

Comments

@raboof
Copy link
Contributor

raboof commented Jul 10, 2019

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
Copy link
Contributor Author

raboof commented Jul 10, 2019

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
Copy link
Collaborator

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:

  • set mimaPreviousArtifacts, if it makes sense for it to;
  • set mimaFailOnNoPrevious := false on specific projects that want to opt-out (or disablePlugins(MimaPlugin))
  • set mimaFailOnNoPrevious in ThisBuild := false to revert back to the previous behaviour where not having previous versions doesn't fail the task

@dwijnand
Copy link
Collaborator

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 dwijnand self-assigned this Jul 10, 2019
@dwijnand
Copy link
Collaborator

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
Copy link
Contributor

alexarchambault commented Jul 10, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@dwijnand dwijnand reopened this Jul 11, 2019
@raboof
Copy link
Contributor Author

raboof commented Jul 11, 2019

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
Copy link
Collaborator

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment