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

Cross-compile for Scala 3 #227

Merged

Conversation

grzegorz-bielski
Copy link
Contributor

@grzegorz-bielski grzegorz-bielski commented Nov 16, 2023

Updated all deps besides cassandra-driver, moved to testcontainers instead of cassandra-launcher like in here, removed pureconfig derivation and cross compiled against 3.3.3, 2.13.13 and 2.12.19.

TODO:
- [ ] add https://github.com/scalacenter/sbt-version-policy - Will do it in separate PR.

@grzegorz-bielski grzegorz-bielski changed the title Cross-compile for Scala 3 [WIP] Cross-compile for Scala 3 Nov 16, 2023
@grzegorz-bielski grzegorz-bielski changed the title [WIP] Cross-compile for Scala 3 Cross-compile for Scala 3 Mar 6, 2024
@Z1kkurat Z1kkurat requested review from rtar and Z1kkurat March 6, 2024 15:29
@coveralls
Copy link

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8457153266

Details

  • 73 of 73 (100.0%) changed or added relevant lines in 13 files are covered.
  • 8 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+12.7%) to 89.319%

Files with Coverage Reduction New Missed Lines %
scassandra/src/main/scala/com/evolutiongaming/scassandra/Masked.scala 1 0.0%
scassandra/src/main/scala/com/evolutiongaming/scassandra/ReconnectionConfig.scala 1 40.0%
scassandra/src/main/scala/com/evolutiongaming/scassandra/QueryConfig.scala 1 82.35%
scassandra/src/main/scala/com/evolutiongaming/scassandra/SocketConfig.scala 1 75.0%
scassandra/src/main/scala/com/evolutiongaming/scassandra/LoadBalancingConfig.scala 1 11.11%
scassandra/src/main/scala/com/evolutiongaming/scassandra/ReplicationStrategyConfig.scala 1 77.78%
scassandra/src/main/scala/com/evolutiongaming/scassandra/SpeculativeExecutionConfig.scala 2 20.0%
Totals Coverage Status
Change from base Build 7741500935: 12.7%
Covered Lines: 669
Relevant Lines: 749

💛 - Coveralls

Copy link
Contributor

@Z1kkurat Z1kkurat left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rtar
Copy link
Contributor

rtar commented Mar 8, 2024

I did not double check if all properties are tested in unit tests. May be will do later.

@grzegorz-bielski
Copy link
Contributor Author

@rtar, @Z1kkurat do you think we can merge it soon?

@Z1kkurat
Copy link
Contributor

@rtar, @Z1kkurat do you think we can merge it soon?

@rtar i have no objections, let's merge?

@rtar
Copy link
Contributor

rtar commented Mar 20, 2024

Yes, let's do it!

@rtar
Copy link
Contributor

rtar commented Mar 20, 2024

One note, though. If it was not tested for binary compatibility, it might need to be released as a new major version with all the consequences involved.

@Z1kkurat
Copy link
Contributor

I just thought: does pureconfig derive codecs that can read only dash-separated names, or does it allow dots too? In other words, is it able to read only "connect-timeout", or can it read connect.timeout too? If the latter is the case then we need to support that

@grzegorz-bielski
Copy link
Contributor Author

grzegorz-bielski commented Mar 20, 2024

@Z1kkurat I believe . would mean nesting in HOCON.

By default pureconfig maps from camelCase values in case classes to kebab-case values in config files. This can be configured by ProductHint during derivation, but we weren't changing it.

@Z1kkurat
Copy link
Contributor

Z1kkurat commented Mar 25, 2024

One note, though. If it was not tested for binary compatibility, it might need to be released as a new major version with all the consequences involved.

there were no changes to the API though, why do you think the compatibility could be broken?

@Z1kkurat
Copy link
Contributor

@rtar Grzegorz updated the version to 6.0.0, as you asked (though I think it was unnecessary). Shall we merge this, please, or do you have any other concerns? This PR is quite big, and I don't want to delay it for longer

@rtar
Copy link
Contributor

rtar commented Mar 27, 2024

I suggest to merge it. I think it is fine to release it as 5.x if binary compatibility is maintained.

@Z1kkurat
Copy link
Contributor

@grzegorz-bielski, without going over all the files again, could you confirm only internals were changed without changes to signatures and APIs? I'd honestly prefer to keep the version to 5.x, otherwise it's going to be a lot of pain further down the road

@grzegorz-bielski
Copy link
Contributor Author

grzegorz-bielski commented Mar 27, 2024

@Z1kkurat The only change in the main API was splitting the implicit config readers to a separate trait like AuthenticationConfigImplicits, which is then mixed with original object like AuthenticationConfig. Variable names were not renamed and type signatures not modified, so the change should be backwards compatible.

Edit: I found one issue, which is now fixed.

@grzegorz-bielski
Copy link
Contributor Author

grzegorz-bielski commented Mar 27, 2024

I have set up the sbt-version-policy locally ( I don't want to include it in this PR to reduce the scope)

After f175a86 sbt scassandra / mimaReportBinaryIssues doesn't report any issues anymore but sbt scassandra / versionPolicyCheck returns:

[error]   ch.qos.logback:logback-classic: missing dependency
[error]   ch.qos.logback:logback-core: missing dependency

Not sure why, since I didn't remove it. 🤔

@rtar
Copy link
Contributor

rtar commented Mar 27, 2024

I suggest to merge it, and I think we can consider it binary compatible (i.e. release it as 5.x).

I suspect one of your updated dependencies removed logback, hence is your error. You can add it manually back, but I think it is fine without it, either.

@grzegorz-bielski
Copy link
Contributor Author

@rtar Should I change the version to 5.1.0 or can we leave at 5.0.3-SNAPSHOT ?

@rtar
Copy link
Contributor

rtar commented Mar 27, 2024

I am not sure, but I think you should use 5.0.3-SNAPSHOT if it is source-compatible and binary-compatible, and use 5.1.0-SNAPSHOT if it is only binary-compatible.

I think you might also use 5.1.0-SNAPSHOT to show the significance of having Scala 3 support.

To summarize, it is up to you 😊

@grzegorz-bielski
Copy link
Contributor Author

Since it also removes the transitive logback dependency then let's go with 5.1.0-SNAPSHOT and :shipit:

@Z1kkurat Z1kkurat merged commit 3e9ffb3 into evolution-gaming:master Mar 28, 2024
4 checks passed
@grzegorz-bielski grzegorz-bielski deleted the chore/scala3-cross-compile branch March 28, 2024 21:33
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.

4 participants