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

CORE-69: update Scala, sbt versions and associated plugins #1566

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

davidangb
Copy link
Contributor

@davidangb davidangb commented Oct 15, 2024

Ticket: https://broadworkbench.atlassian.net/browse/CORE-69

Updates the Scala and sbt versions, along with associated sbt plugins and utility docker image.

  • Scala to 2.13.15
  • sbt to 1.10.2
  • docker image to sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 (note the matching 1.10.2 and 2.13.15)
  • related updates to sbt-assembly, sbt-revolver, sbt-scoverage, sbt-scalafmt, sbt-scalafix plugins
  • sbt scalacOptions fix to address a breaking change in Scala 2.13.15
  • code-level fixes to address compile errors as of Scala 2.13.11 and later

This supersedes Scala Steward PRs #1419, #1540, #1548, #1550, #1555


PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

@@ -37,7 +37,7 @@ object Settings {
"-language:existentials", // Existential types (besides wildcard types) can be written and inferred
"-unchecked", // Enable additional warnings where generated code depends on assumptions.
"-Xcheckinit", // Wrap field accessors to throw an exception on uninitialized access.
"-Wconf:cat=deprecation:ws,any:e", // Fail the compilation if there are any warnings, except for deprecation warnings.
"-Wconf:any:e,cat=deprecation:ws", // Fail the compilation if there are any warnings, except for deprecation warnings.
Copy link
Contributor Author

@davidangb davidangb Oct 15, 2024

Choose a reason for hiding this comment

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

See the "Breaking Changes" section of https://github.com/scala/scala/releases/tag/v2.13.15: the order in which -Wconf is parsed has changed. This change necessary to retain functionality.

Of note: I am not sure this setting is actually what we want. A few lines above we specify:

"-deprecation", // Emit warning and location for usages of deprecated APIs.

but here we configure deprecations to be ws, which stands for "warning summary" - the summary suppresses the info about where the actual deprecations are in the codebase and just gives a summary like "12 deprecations….". BUT, to keep scope of this PR down I am not changing it here; this PR preserves existing functionality.

@@ -56,7 +56,7 @@ trait SamRequestContextDirectives {
def withSamRequestContext: Directive1[SamRequestContext] =
for {
clientIP <- extractClientIP
otelContext <- traceRequest
otelContext <- traceRequest()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of Scala 2.13.11, this caused compile to break. It's a warning, but we have -Wconf … any:e set, which causes warnings to be errors.

@@ -51,10 +51,10 @@ object SamAuditModelJsonSupport {
}
}

implicit val AuditInfoFormat = jsonFormat2(AuditInfo)
implicit val AuditInfoFormat: RootJsonFormat[AuditInfo] = jsonFormat2(AuditInfo)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of Scala 2.13.11, this caused compile to break. It's a warning, but we have -Wconf … any:e set, which causes warnings to be errors.

The error was "Implicit definition should have explicit type" and we had many instances of this issue; lots of lines of code changed below.

@@ -16,6 +16,7 @@ object Merging {
case PathList("META-INF", "kotlin-stdlib-common.kotlin_module") => MergeStrategy.first
case PathList("META-INF", "okio.kotlin_module") => MergeStrategy.first
case PathList("META-INF", "versions", "9", "OSGI-INF", "MANIFEST.MF") => MergeStrategy.first
case PathList("META-INF", "license", "LICENSE.mvn-wrapper.txt") => MergeStrategy.first
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some dependencies changed, such that we now have an assembly merge conflict on META-INF/license/LICENSE.mvn-wrapper.txt; added a case for this file.

@@ -1 +1 @@
sbt.version=1.6.2
sbt.version=1.10.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

of note: I did not change the sbt.version in the codegen_java_old directory. Why do we have an "old" directory committed to source control?

Copy link

sbtscala/scala-sbt:openjdk-17.0.2_1.7.2_2.13.10 \
sbt "set scalafmtOnCompile := false" "project pact4s" "testOnly *SamProviderSpec"
sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 \
bash -c "git config --global --add safe.directory /working/sam && sbt \"set scalafmtOnCompile := false\" \"project pact4s\" \"testOnly *SamProviderSpec\""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here and elsewhere, set the scala-sbt docker image to the version that uses sbt 1.10.2 and Scala 2.13.15.

The docker image upgrade must have updated its version of git, because it started running into the "dubious ownership" error; so, I also added the incantation to set the git safe.directory.

@davidangb davidangb requested review from a team, calypsomatic and kevinmarete and removed request for a team October 16, 2024 14:39
@davidangb davidangb merged commit 2cddf18 into develop Oct 16, 2024
25 checks passed
@davidangb davidangb deleted the da_CORE-69_updateScala branch October 16, 2024 15:27
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.

3 participants