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

Migrate to sbt-typelevel #485

Merged
merged 13 commits into from
Feb 1, 2022
Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jan 23, 2022

Closes #480. Supersedes #483.

Apologies to @osleonard if you were working on this already. This turned out be a very complex migration.

  1. MiMa was not being run in CI, so there were some binary-incompatible changes in 0.9.x series. I'll add a comment about this below.
  2. The benchmarks were not being compiled in CI, and currently do not compile in Scala 3, so I was unable to add them back to CI. This should be fixed in follow-up work and the benchmarks should be compiled in CI.
  3. The scalac flags were updated to the standard used by most Typelevel projects. Some existing code could no longer compile and had to be modified.
  4. Lots of warnings everywhere, so I disabled fatal warnings in CI. These should be fixed and re-enabled in follow-up work.
  5. The diff seems big, but a lot of it is headers.

Comment on lines -79 to 106
implicit val predicateMonoidK: MonoidK[Predicate] = new MonoidK[Predicate] {
@deprecated("Retained for bincompat", "0.9.1")
def predicateInstance: MonoidK[Predicate] = predicateMonoidK

implicit def predicateMonoidK: MonoidK[Predicate] = new MonoidK[Predicate] {
override def empty[A]: Predicate[A] = Predicate.empty
override def combineK[A](l: Predicate[A], r: Predicate[A]): Predicate[A] = l.union(r)
}
Copy link
Member Author

@armanbilge armanbilge Jan 23, 2022

Choose a reason for hiding this comment

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

So we are in a bit of a hole wrt to bincompat. Bincompat with 0.9.0 was broken in #269 (comment).

[error] core: Failed binary compatibility check against org.typelevel:cats-collections-core_2.12:0.9.0 (e:info.versionScheme=early-semver)! Found 5 potential problems
[error]  * static method predicateInstance()cats.MonoidK in class cats.collections.Predicate does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.collections.Predicate.predicateInstance")
[error]  * method predicateInstance()cats.MonoidK in object cats.collections.Predicate does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.collections.Predicate.predicateInstance")
[error]  * abstract method predicateInstance()cats.MonoidK in interface cats.collections.PredicateInstances does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.collections.PredicateInstances.predicateInstance")
[error]  * abstract synthetic method cats$collections$PredicateInstances$_setter_$predicateMonoidK_=(cats.MonoidK)Unit in interface cats.collections.PredicateInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.collections.PredicateInstances.cats$collections$PredicateInstances$_setter_$predicateMonoidK_=")
[error]  * abstract method predicateMonoidK()cats.MonoidK in interface cats.collections.PredicateInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.collections.PredicateInstances.predicateMonoidK")

I was able to restore it by making these changes here.

However, changing the predicateMonoidK from a val to a def now breaks bincompat on Scala 3:

[error] core: Failed binary compatibility check against org.typelevel:cats-collections-core_3:0.9.3 (e:info.versionScheme=early-semver)! Found 1 potential problems (filtered 34)
[error]  * synthetic static method $init$(cats.collections.PredicateInstances)Unit in interface cats.collections.PredicateInstances does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.collections.PredicateInstances.$init$")

Based on my reading of lightbend-labs/mima#448 this seems like a legitimate issue. However, it is relatively minor and Scala 3-only, so perhaps it would be okay to look the other way on this one ... I think fixing it would require splitting sources across Scala 2 / 3.

@johnynek would appreciate your thoughts here.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh... I'd rather break bin-compat on scala 3 since I assume it is much less commonly used.

It is really weird that changing from val to def breaks binary compatibility... that's a real shame. Maybe that will be fixed in later scala 3?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how they can fix it binary-compatibly, unless they always generate dummy $init$ methods. If vals generated an $init$ method in the past, then they must continue to generate it going forward.

@osleonard
Copy link
Collaborator

Closes #480. Supersedes #483.

Apologies to @osleonard if you were working on this already. This turned out be a very complex migration.

1. MiMa was not being run in CI, so there were some binary-incompatible changes in 0.9.x series. I'll add a comment about this below.

2. The benchmarks were not being compiled in CI, and currently do not compile in Scala 3, so I was unable to add them back to CI. This should be fixed in follow-up work and the benchmarks should be compiled in CI.

3. The scalac flags were updated to the standard used by most Typelevel projects. Some existing code could no longer compile and had to be modified.

4. Lots of warnings everywhere, so I disabled fatal warnings in CI. These should be fixed and re-enabled in follow-up work.

5. The diff seems big, but a lot of it is headers.

@armanbilge its ok started working on it yesterday, since you've made it happen it also fine

@armanbilge
Copy link
Member Author

@osleonard I'm sorry about that, I should have checked in first :( in any case, there's still much more work to be done to "fully" adopt sbt-typelevel in cats-collections, such as fixing all the warnings so you can enable fatal warnings in CI. I leave that to the maintainer team here :)

@armanbilge
Copy link
Member Author

Ok, this PR should be ready. I filtered the MiMa issue on Scala 3 and created these follow-ups:

Copy link
Collaborator

@gemelen gemelen left a comment

Choose a reason for hiding this comment

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

Looks good besides known issues

@osleonard osleonard merged commit 809d466 into typelevel:master Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to sbt-typelevel
4 participants