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

Scalacheck version update #2312

Closed
wants to merge 1 commit into from
Closed

Scalacheck version update #2312

wants to merge 1 commit into from

Conversation

barambani
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 4, 2018

Codecov Report

Merging #2312 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2312   +/-   ##
=======================================
  Coverage   95.03%   95.03%           
=======================================
  Files         338      338           
  Lines        5878     5878           
  Branches      210      210           
=======================================
  Hits         5586     5586           
  Misses        292      292

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b77c388...b906493. Read the comment docs.

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the binary compatibly of cats-laws. We were hoping to have a cross build solution. Not sure how that looks now, cc @BennyHill

@barambani
Copy link
Contributor Author

My problem is in fact that I cannot use 1.14.0 in my projects with cats-laws 1.1.0. I'm forced to 1.13.5 as it fails with

[error] java.lang.NoSuchMethodError: org.scalacheck.Cogen$.apply(Lorg/scalacheck/Cogen;Lorg/scalacheck/Cogen;)Lorg/scalacheck/Cogen;
[error] 	at cats.laws.discipline.arbitrary$.<init>(Arbitrary.scala:21)
[error] 	at cats.laws.discipline.arbitrary$.<clinit>(Arbitrary.scala)
[error] 	at cats.laws.discipline.ApplicativeErrorTests$$anon$2.props(ApplicativeErrorTests.scala:49)
[error] 	at org.typelevel.discipline.Laws$RuleSet.collectParentProps(Laws.scala:80)
[error] 	at org.typelevel.discipline.Laws$RuleSet.$anonfun$collectParentProps$1(Laws.scala:80)
[error] 	at scala.collection.TraversableLike.$anonfun$flatMap$1(TraversableLike.scala:241)
[error] 	at scala.collection.immutable.List.foreach(List.scala:389)
[error] 	at scala.collection.TraversableLike.flatMap(TraversableLike.scala:241)
[error] 	at scala.collection.TraversableLike.flatMap$(TraversableLike.scala:238)
[error] 	at scala.collection.immutable.List.flatMap(List.scala:352)
[error] 	at org.typelevel.discipline.Laws$RuleSet.collectParentProps(Laws.scala:80)
[error] 	at org.typelevel.discipline.Laws$RuleSet.$anonfun$all$1(Laws.scala:83)
[error] 	at org.typelevel.discipline.AllProperties.<init>(Laws.scala:128)
[error] 	at org.typelevel.discipline.Laws$RuleSet.all(Laws.scala:83)
[error] 	at org.typelevel.discipline.Laws$RuleSet.all$(Laws.scala:83)
[error] 	at cats.laws.discipline.MonadErrorTests$$anon$2.all(MonadErrorTests.scala:36)
[error] 	at org.typelevel.discipline.scalatest.Discipline.checkAll(Discipline.scala:12)
[error] 	at org.typelevel.discipline.scalatest.Discipline.checkAll$(Discipline.scala:11)

may be you know if I have a workaround ?

@johnynek
Copy link
Contributor

johnynek commented Jul 4, 2018

This is why binary versioning is pretty tough. If we bump this for you, everyone using 1.13 is hosed. We could imagine expanding a matrix of compatibility.

Side note, I bet this is source compatible. One day, maybe we will have source builds that can deal with this.

Cc @non here is an example where maybe we could have minimized the changes in 1.14 to make it more compatible. We need to not view binary compatibility as binary and instead what is the probability a project is broken by a change. We still want to minimize that probability even when it is not zero.

@barambani
Copy link
Contributor Author

barambani commented Jul 4, 2018

I understand perfectly, thanks. Also I can stay on 1.13.5 for now as I don't see a way around this.
EDIT At this point I will close the PR if that's ok. Thanks.

@rossabaker
Copy link
Member

I would still appreciate a cross-build with this. Without cats-laws, scalacheck-1.14 might as well not exist for me.

@kailuowang
Copy link
Contributor

Cross building on Scalacheck would require a cross build on discipline as well (which is already on ScalaCheck 1.14).
According to this poll [https://twitter.com/kailuowang/status/993853527998025728]
4% of the users would feel pain if we break the binary compatibility of cats-laws.
I feel it might be easier and (also better in the long run) to help the whole community move to Scala 1.14.

For now I am going to schedule it for 1.2

@kailuowang kailuowang added this to the 1.2 milestone Jul 5, 2018
@kailuowang
Copy link
Contributor

kailuowang commented Jul 5, 2018

The cross-build solution suggested seems involves Sbt 1.2 (just released RC1) and sbt-projectmatrix . both very young and I am not sure if they support scalajs yet. My proposal is that we release 1.2 with ScalaCheck 1.14 for now and, if we really need to, we can find a way to release a cats-laws and cats-testkit with 1.2.0-ScalaCheck1.13

Update: alleycats-laws also need to be cross build

@djspiewak
Copy link
Member

@kailuowang Should be possible to do this without SBT 1.2 or a separate plugin, unless I'm missing something. djspiewak/smock does something actually even a bit more intricate than what you're trying to accomplish here: https://github.com/djspiewak/smock/blob/master/build.sbt#L17-L31

@rossabaker
Copy link
Member

Another irritant is that this matrix is only applicable to three modules. It can still be done, but I think the stack o' hacks is one deeper than smock's.

@djspiewak
Copy link
Member

@rossabaker I agree the stack-o-hacks is a little deeper (especially since it involves discipline, which is a separate repo), but the approach seems relatively general to me.

@rossabaker
Copy link
Member

The biggest worry I see in dropping 1.13 is that scalatest is not there yet. specs2 and minitest have already switched to 1.14. Imposing 1.14 becomes more tenable if we have all the major test libraries on board.

I tried to analyze downstream projects, but I don't know how to filter test-scoped from runtime dependencies. The latter set of projects would need to rebuild for their own consumers. This includes cats-effect, circe, and http4s. It would be nice if these projects all followed cats' lead, whether that's a crossbuild or a simple upgrade.

@kailuowang
Copy link
Contributor

kailuowang commented Jul 5, 2018

I spent the last 2 hours trying to write a cross build hack in cats. The fact that we only have 3 modules that need to be cross build+ we cross build scalajs + we use sbt-release makes it more complex than my limited sbt skills can handle.
As pointed out by @rossabaker above, if cats go cross build on ScalaCheck, downstream projects also have to setup cross build. Now I am even less sure if we should try pursue this through some sbt hack.

I suspect that Scalatest's integration with ScalaCheck doesn't suffer a binary compatibility issue. At least discipline was able to update to ScalaCheck 1.14 without problems. The BC issue reported by @barambani is on Cogen, which seems to have no mentions in ScalaCheck

@djspiewak
Copy link
Member

  • we use sbt-release makes it more complex

I forgot about sbt-release. I hate sbt-release. A lot. That definitely puts things a bit beyond the pale.

As pointed out by @rossabaker above, if cats go cross build on ScalaCheck, downstream projects also have to setup cross build. Now I am even less sure if we should try pursue this through some sbt hack.

My proposal, if we do pursue an SBT hack, would be to only apply it to the 2.13 build. Downstream projects have no expectations at that point. For the most part, they're only going to be affected if they depend on cats-laws, and especially if they pass along cats-laws as a dependency. Broadly, this means cats-effect is going to get the worst of it, but most other projects shouldn't care. Endpoint downstream projects, which don't cross-build across Scala versions, shouldn't be affected at all.

@rossabaker
Copy link
Member

I suspect cats-effect-laws has few consumers, though its TestContext is handy even if not checking instances. http4s-testing is of narrow interest, though it could be a lot more compelling if we'd pour more effort into arbitrary requests for fuzzing. I suspect circe-testing is the most depended upon project that depends on cats-laws. I have used this in several endpoint projects.

@kailuowang
Copy link
Contributor

@djspiewak Are you suggesting we cross build ScalaCheck on Scala 2.13? or are you suggesting we have 2.13 build on ScalaCheck 1.14 and the rest 2.10-2.12 on ScalaCheck 1.13?

@djspiewak
Copy link
Member

@kailuowang At least for the time being, yes. The concept basically being that we can push a 2.13 build by depending on 1.14, but the rest of all the things can still depend on 1.13 and retain compatibility. Obviously this would be a temporary situation.

@djspiewak
Copy link
Member

djspiewak commented Jul 5, 2018

BTW, this should be relatively achievable by doing something like:

libraryDependencies += "org.scalacheck" %% "scalacheck" % {
  CrossVersion.partialVersion(scalaVersion.value) match {
    case Some((2, major)) if major < 13 => "1.13.5"
    case Some((2, major)) if major >= 13 => "1.14.0"
  }
} % Test

More or less. (note: isolating this to a custom setting key will be more elegant and allow reuse) The differences between the two frameworks in terms of source compatibility (should be negligible) then can be factored out using the normal cross-building mechanisms (e.g. src/main/scala-2.13/).

@travisbrown
Copy link
Contributor

@rossabaker I'm not too worried about bumping the ScalaCheck version in circe-testing to 1.14, given that we have no guarantees of compatibility between minor versions before 1.0, and I almost certainly wouldn't cross-build for 1.13 / 1.14 even if cats-laws did (unless someone paying me wanted it or someone opened a PR and promised to maintain it forever).

@ghost
Copy link

ghost commented Jul 6, 2018

We were hoping to have a cross build solution.

involves Sbt 1.2 (just released RC1) and sbt-projectmatrix

Actually the newer crossProject that we don't use yet.

And I'm actually camping at the moment, typing away under canvas ⛺ But reading the above does suggest a better/proper cross versioning build is worth the effort including for related projects such as discipline,etc that, IMHO, should remain separate.

But the fact remains that the (ideal) required infra projects are not there yet - sbt, crossproject, etc. But I believe it would be quite straightforward to mimic the output, but I suggest gitter would be better for me to air my thoughts for now

@ghost
Copy link

ghost commented Jul 6, 2018

@djspiewak
Copy link
Member

But the fact remains that the (ideal) required infra projects are not there yet - sbt, crossproject, etc.

@BennyHill @kailuowang If you could describe your ideal infrastructural solution to this problem, what would it be? I don't mind whipping up SBT plugins which solve plumbing projects like this (actually, I quite enjoy that sort of thing). The only reason I haven't is I don't have any immediate inspiration for what a good solution would look like.

@ghost
Copy link

ghost commented Jul 6, 2018

what a good solution would look like.

Is perhaps the root problem 😜 And awesome offer, @djspiewak , btw 👍

My (our?) current thoughts I've summarized as per gitter link^^, but the end result would be that cats 1.2 would still scalacheck 1.3 and be fully bin compat, but we would have another lib with a classifier of say "BC1", in the same way that classifiers are added for different target architectures and/or environments - and that would use SC 1.4 and any other different library versions.

@kailuowang
Copy link
Contributor

kailuowang commented Jul 6, 2018

@djspiewak I believe sbt-projectmatrix (cc @eed3si9n) is trying to provide a unified solution for cross- building. For now, we need something that can work with sbt-crossproject. I can imagine the usage would be something like

lazy val laws = crossProject(JSPlatform, JVMPlatform).in(file("."))
  .settings(
    commenSettings,
    moduleName := "cats-laws",
     libraryDependencies += 
        "org.scalacheck" %%% "scalacheck" % "1.14.0"
  )

lazy val lawsJVM = laws.jvm 
lazy val lawsJS = laws.js  

val lawsSC1_13 = laws.settings(   
     libraryDependencies += 
       "org.scalacheck" %%% "scalacheck" % "1.13.5",
     versionSuffix := "ScalaCheck1.13"   
  )

lazy val lawsSC1_13JVM = lawsSC1_13.jvm 
lazy val lawsSC1_13JS = lawsSC1_13.js  

Obviously We'll also need to add lawsSC1_13JVM and lawsSC1_13JS to root modules.

This should result in testing and publishing additional cats-laws versions with ScalaCheck 1.13.5 and a version suffix ScalaCheck1.13 , e.g. cats-laws 1.2.0-ScalaCheck1.13

If we use this setup the only magic the plugin needs to provide is probably just appending the versionSuffix, if set, to the release version.

Update: I forgot to mention, that at least for now, integration with sbt-release would be great for Cats. (maybe a task to append the suffix? thus can be run as a release step command?)

@djspiewak
Copy link
Member

@BennyHill @kailuowang I'll give it some thought. I will warn you though that sbt-release integration is probably relatively low on my motivational TODO list. This is for two reasons:

  • Fundamentally, I disagree with the existence of sbt-release. It strongly encourages a lot of anti-patterns (like -SNAPSHOT releases) and doesn't achieve anything that cannot be done in a more structured way with sbt-git and aliases
  • sbt-release does some WEIRD MOJO with the internal SBT state monad, which makes it quite difficult to extend in non-trivial ways.

It's doable, just a massive pain. :-)

Anyway, I'll look into it and see if I can come up with something.

@johnynek
Copy link
Contributor

johnynek commented Jul 6, 2018

To me, the ideal solution is for the community to work towards source builds (like Rust and Haskell have).

It’s not a quick fix, but if we had source builds we would detect these issues at compile time, and avoid many of them due to how much easier source compatibility is than binary for scala.

@kailuowang
Copy link
Contributor

@djspiewak I am open to a replacement of sbt-release in Cats. The main benefit of sbt-release is that the release process enabled by it is scripted and well known. As long as the replacement is equally straightforward for a new maintainer to do a Cats release, I am all for it.

@djspiewak
Copy link
Member

djspiewak commented Jul 6, 2018

@johnynek I'm still not convinced that source dependencies are the promised land. They just have a very different set of tradeoffs, and they haven't been in wide use since the days of gnu autoconf, so the warts aren't as familiar or as widely understood. I've spent enough time with Gentoo and similar to be extremely leery of source dependencies, though obviously a Linux distribution is a slightly bigger problem than a PL ecosystem.

At any rate, we've had this discussion in parts before. :-) And it's more future-speculative-ish.

@kailuowang This is what I use on my projects, which in turn enables this versioning outcome. (basically semver with stable snapshots and better major/minor bump determination)

It wouldn't be hard to split this away from sbt-spiewak and make it a bit more configurable. I've been meaning to extend it with support for matrix releases (e.g. for smock). Basing things around addCommandAlias does have some downsides, but it carries considerably fewer gotchas than the sbt-release strategy of explicitly fiddling with SBT's internal state.

I agree that scripting, repeatability, and accessibility are 100% essential components of any release process.

@kailuowang
Copy link
Contributor

It wouldn't be hard to split this away from sbt-spiewak and make it a bit more configurable. I've been meaning to extend it with support for matrix releases

@djspiewak That would be amazing. As a side note, in regards to mima check, we check against all previous minor versions with patch ver 0, i.e. 1.0.0, 1.1.0 etc, and all previous bug-fix version under the same minor, code here.

@ghost
Copy link

ghost commented Jul 6, 2018

@kailuowang / anyone

Your source example above looks good, 👍 , but I wonder if that should be for a cats 2.0 release when we break BC? So for now, 1.2, we would have instead :

lazy val laws = crossProject(JSPlatform, JVMPlatform).in(file("."))
  .settings(
    commenSettings,
    moduleName := "cats-laws",
     libraryDependencies += 
        "org.scalacheck" %%% "scalacheck" % "1.13.5"
  )

lazy val lawsJVM = laws.jvm 
lazy val lawsJS = laws.js  

val lawsSC1_14 = laws.settings(   
     libraryDependencies += 
       "org.scalacheck" %%% "scalacheck" % "1.14.0",
     versionSuffix := "ScalaCheck1.14"   
  )

lazy val lawsSC1_14JVM = lawsSC1_14.jvm 
lazy val lawsSC1_14JS = lawsSC1_14.js  

So for now , breaking BC is opt-in, but for a major release, old library versions are opt-in.

And before I disappear, I kinda agree with @djspiewak re source builds - they're cool, but I don't think they will solve all the problems that folk want 😄

@kailuowang
Copy link
Contributor

@BennyHill agree, we should make default not binary breaking.

@djspiewak
Copy link
Member

Quick update: I haven't forgotten about this! I spent a bit of time playing around with how I wanted this to work, ran into a major blocker, and found a nice solution to it last night in a random revelation.

ArturGajowy added a commit to ArturGajowy/rchain that referenced this pull request Dec 6, 2018
cats-laws 1.x depends on scalacheck 1.13.5, which is binary-incompatible
with the newer 1.14.0 - causing NoSuchMethodErrors in law tests for
MonadError

See typelevel/cats#2312
@kailuowang
Copy link
Contributor

An update: I tried several approaches for cross releasing for scalacheck 1.14, they either don't work or introduce a heck amount of complexity to the build. I ran out of steam for this. Considering the fact that other libs need to do the same thing, I start to believe that cost wise, it's probably just easier to have the whole ecosystem upgrade to scalacheck 1.14 at one go - i.e. cats upgrades it in 1.6 and projects follows.

@johnynek
Copy link
Contributor

PS could we upstream our concerns about better binary compatibility checking (cc @non ) or maybe introduce typeclasses to abstract out scalacheck and allow users to work with hedgehog, scalacheck, etc... via a module.

@barambani
Copy link
Contributor Author

barambani commented Mar 7, 2019

This is made obsolete by #2734 and can be closed when that's merged.

@kailuowang kailuowang closed this Mar 7, 2019
@barambani barambani deleted the scalacheck-update branch March 7, 2019 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants