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

Update deps, add Scala 3 support, drop Scala 2.12 #134

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

migesok
Copy link
Contributor

@migesok migesok commented Sep 5, 2024

Compared to conhub 1.2.1 the changes are source and binary compatible with these exceptions:

  • potential source compat breakage: NamedDispatcher doesn't have an implicit argument in the constructor anymore but it appears to be unused even in the internal Evolution codebase

Changes highlights:

  • updated plugins and dependencies
  • added Scala 3.3.3 cross-compilation
  • removed Scala 2.12 support
  • introduced sbt-version-policy plugin and MiMa bincompat checks
  • fixed compilation warnings, minor test refactoring
  • replaced CurrentThreadExecutionContext usage with direct calls to ExecutionContext.parasitic since we do not support 2.12 anymore
  • Scala 3 compiler complained about NamedDispatcher implicit arguments in the constructor. Since the class is unused even in the internal Evolution codebase, implicit modifier was removed and the type was marked as deprecated.

Compared to conhub 1.2.1 the changes are source and binary compatible with
these exceptions:
- potential source compat breakage: NamedDispatcher doesn't have an implicit argument in the constructor anymore
  but it appears to be unused even in the internal Evolution codebase

Changes highlights:
- updated plugins and dependencies
- added Scala 3.3.3 cross-compilation
- removed Scala 2.12 support
- introduced sbt-version-policy plugin and MiMa bincompat checks
- fixed compilation warnings, minor test refactoring
- replaced CurrentThreadExecutionContext usage with direct calls to ExecutionContext.parasitic since
  we do not support 2.12 anymore
- Scala 3 compiler complained about NamedDispatcher implicit arguments in the constructor.
  Since the class is unused even in the internal Evolution codebase, implicit modifier was removed and
  the type was marked as deprecated.
@migesok migesok self-assigned this Sep 5, 2024
@coveralls
Copy link

coveralls commented Sep 5, 2024

Pull Request Test Coverage Report for Build 10723408465

Details

  • 8 of 29 (27.59%) changed or added relevant lines in 8 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+17.8%) to 49.741%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/scala/com/evolutiongaming/conhub/ConHub.scala 0 1 0.0%
src/main/scala/com/evolutiongaming/conhub/MemberEventSubscribe.scala 0 1 0.0%
src/main/scala/com/evolutiongaming/conhub/ConHubSerializer.scala 0 2 0.0%
src/main/scala/com/evolutiongaming/conhub/ConStates.scala 2 4 50.0%
src/main/scala/com/evolutiongaming/conhub/transport/SendMsg.scala 1 16 6.25%
Files with Coverage Reduction New Missed Lines %
src/main/scala/com/evolutiongaming/conhub/ConStates.scala 2 83.64%
Totals Coverage Status
Change from base Build 9989427472: 17.8%
Covered Lines: 192
Relevant Lines: 386

💛 - Coveralls

@migesok migesok requested a review from mr-git September 5, 2024 15:29
project/Dependencies.scala Show resolved Hide resolved

object Akka {
private val version = "2.6.19"
private val version = "2.6.21"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should bump Akka because it's patch versions are incompatible, so we can potentially get this error in downstream dependencies if they depend on Akka 2.6.19

java.lang.IllegalStateException: You are using version 2.6.21 of Akka, but it appears you (perhaps indirectly) also depend on older versions of related artifacts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe everyone should be on 2.6.21 already, it doesn't seem to be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

and most of our OSS base akka support libraries are on 2.6.21 for months already

Copy link

Choose a reason for hiding this comment

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

No, we use .20 via overrides. The fact that our libraries are on .21 is a problem and not a reason for others to bump it too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid that it will sound arrogant, but prohibiting upgrades for all other projects because your one cannot or doesn't want to update is not a solution we can accept.

You are free to put overrides in your project or to not update some library at your own risk.

Please review changes in akka (https://github.com/akka/akka/releases/tag/v2.6.21) - they are very small and, most probably, will not have negative affect on your project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stand with @mr-git here - Akka 2.6.21 is more than 1 year old now. If there is an internal reason for us to use older versions internally, there is no reason to have an outdated version in the opensource. Updating Akka 19 and 20 to 21 is trivial and by just trying to hold it in transitive dependencies is a loosing game. There are already dependencies which will bring 2.6.21 transitively (like safe-akka). If you want safeguards, they should be on the library client side - dependency overrides and such.

Choose a reason for hiding this comment

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

@vilunov, am I right that we (including you) are using Akka 2.6.21 now? Shall we unblock the PR?

project/Dependencies.scala Show resolved Hide resolved
@edubrovski
Copy link
Contributor

@migesok Overall it looks like you're making the assumption that Scala libraries adhere to SEMVER and if major version hasn't changed then it's safe to bump. Unfortunately this is often not the case with Scala libraries. I propose to keep MRs small. If you need Scala 3 I think it's better to only update what's needed for Scala 3 in this MR/

@migesok
Copy link
Contributor Author

migesok commented Sep 6, 2024

@migesok Unfortunately it is not possible since dependencies also have to have Scala 3 support and many of them didn't.
They have to come in one set of changes. I can comment on individual dependency version changes if you wish.

@edubrovski
Copy link
Contributor

overall LGTM but I'd like to wait for one more person to approve before merging


object Akka {
private val version = "2.6.19"
private val version = "2.6.21"
Copy link

Choose a reason for hiding this comment

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

No, we use .20 via overrides. The fact that our libraries are on .21 is a problem and not a reason for others to bump it too.

Copy link
Contributor

@edubrovski edubrovski left a comment

Choose a reason for hiding this comment

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

I'll approve after we resolve the discussion about Akka version

@migesok
Copy link
Contributor Author

migesok commented Sep 10, 2024

@vilunov confirmed that the issue has been resolved
I'm going ahead with the merge and release

@migesok migesok merged commit ac33161 into master Sep 10, 2024
4 of 5 checks passed
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.

6 participants