[SPARK-21708][BUILD] Migrate build to sbt 1.x #759

LorenzoMartini · 2021-04-19T19:45:46Z

Original PR description

Migrate sbt-launcher URL to download one for sbt 1.x.
Update plugins versions where required by sbt update.
Change sbt version to be used to latest released at the moment, 1.3.13
Adjust build settings according to plugins and sbt changes.

Migration to sbt 1.x:

enhances dev experience in development
updates build plugins to bring there new features/to fix bugs in them
enhances build performance on sbt side
eases movement to Scala 3 / dotty

No.

All existing tests passed, both on Jenkins and via Github Actions, also manually for Scala 2.13 profile.

Closes apache#29286 from gemelen/feature/sbt-1.x.

Authored-by: Denis Pyshev git@gemelen.net
Signed-off-by: Dongjoon Hyun dhyun@apple.com

Upstream SPARK-XXXXX ticket and PR link (if not applicable, explain)

[SPARK-21708][BUILD] Migrate build to sbt 1.x
Commit: apache@6daa2ae
PR: apache#29286

What changes were proposed in this pull request?

Bump sbt version to 1.x.

There are a few non-trivial changes related to versions.
First of all the original PR was introduced on top of spark 3.1 so we had to adapt a few things for it to work on top of spark 3.0

[Important] The sbt bump introduces a mima bump and that introduces some binary breaks. If comparing the binary breaks with version 2.4 of spark, there are 200+ listed breaks. However comparing with 3.0, we get away with the breaks that are also added as excludes in the original PR (MimaExcludes.scala contains them) and only 3 additional breaks. According to what discussed in this comment from upstream it is correct for us to compare with version 3.0 and the upstream commit included this change here only because they forgot to do it after releasing 3.0. You can see they encountered the same problem upstream with this bump, from this comment.
The breaks are listed under v30excludes instead of the upstream's v31excludes because current version is 3.0.0
[Workaround] The bump broke sbt unidoc. It attempts to generate docs for generated classes and fails with NoClassDef errors. Since we don't care about generated unidocs from sbt build, we can exclude the unidoc task for now.
[Small added detail] We have to avoid loading sbt/build from cache in circle builds because circle cached the existing file that points at old sbt location (bintray).
[Important note] I reduced circle parallelism from 12 to 8 for scala tests because there is an issue where too many parallel tests might interact one with the other and that causes the task to flake immensely. It seems like a lower parallelism reduces the chances of flaking at the expense of bringing the test time up from around 33 minutes to 42 minutes)

Why are the changes needed?

The SBT bump is needed because:

sbt 0.x (current) is only available in bintray, that is gonna shutdown in a matter of days
sbt 0.x introduces some test dependency on scala module 2.10 which is an issue when bumping avro version as 2.12 is needed.

Does this PR introduce any user-facing change?

No

LorenzoMartini · 2021-04-19T19:49:29Z

.circleci/config.yml

@@ -28,9 +28,6 @@ all-branches-and-tags: &all-branches-and-tags
 # Step templates

 step_templates:
-  restore-build-binaries-cache: &restore-build-binaries-cache


This had to be removed because it picks up older build/sbt and doesn't get the updated versions

LorenzoMartini · 2021-04-19T19:50:37Z

dev/run-tests.py

@@ -390,7 +390,8 @@ def build_spark_assembly_sbt(extra_profiles, checkstyle=False):
    if checkstyle:
        run_java_style_checks(build_profiles)

-    build_spark_unidoc_sbt(extra_profiles)
+    # TODO(lmartini): removed because broken, checks generated classes
+    # build_spark_unidoc_sbt(extra_profiles)


We don't need unidoc and this broke attempting to generate docs for autogenerated files. Didn't want to invest too much into this but can try if needed

LorenzoMartini · 2021-04-19T19:51:58Z

project/MimaBuild.scala

    val organization = "org.apache.spark"
-    val previousSparkVersion = "2.4.0"
+    val previousSparkVersion = "3.0.0"


This file is cleanly picked from upstream. It seems like they forgot to do this bump and added it with this PR out of convenience (context: apache#29286 (comment) and apache#22977 (comment))

If we keep the previous to 2.4.0 the number of binary breaks is 200+ so this is the best option anyways

LorenzoMartini · 2021-04-19T19:52:32Z

project/MimaExcludes.scala

+    // TODO(lmartini): Additional excludes not in upstream but unique to palantir fork
+    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.SparkContext.initializeForcefully"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.SparkContext.initializeForcefully"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.broadcast.Broadcast.initializeForcefully"),


These last 3 excludes had to be added in our fork only. It should be ok

LorenzoMartini · 2021-04-19T19:53:30Z

project/SparkBuild.scala

    dependencyOverrides ++= MavenHelper.fromPom { pom =>
      for {
        dep <- pom.getDependencyManagement.getDependencies.asScala
      } yield MavenHelper.convertDep(dep)
-    }.value.toSet
+    }.value.toSeq


Sbt got stricter and toSet was not accepted

LorenzoMartini · 2021-04-19T19:56:42Z

project/CirclePlugin.scala

-    }.value
+    test := (test andFinally Def.taskDyn {
+      copyTestReportsToCircle
+    }).value


Again, sbt got stricter. and the fancy tuple plugin stuff removed. This is the equivalent of the above

LorenzoMartini · 2021-04-20T07:33:09Z

project/plugins.sbt

-
-// need to make changes to uptake sbt 1.0 support in "com.cavorite" % "sbt-avro-1-7" % "1.1.2"
-addSbtPlugin("com.cavorite" % "sbt-avro" % "0.3.2")
+addSbtPlugin("com.cavorite" % "sbt-avro" % "2.1.1")
 libraryDependencies += "org.apache.avro" % "avro-compiler" % "1.10.1"


The upstream pr changed the version of avro compiler to 1.8.2 but we're already ahead of it

Migrate sbt-launcher URL to download one for sbt 1.x. Update plugins versions where required by sbt update. Change sbt version to be used to latest released at the moment, 1.3.13 Adjust build settings according to plugins and sbt changes. Migration to sbt 1.x: 1. enhances dev experience in development 2. updates build plugins to bring there new features/to fix bugs in them 3. enhances build performance on sbt side 4. eases movement to Scala 3 / dotty No. All existing tests passed, both on Jenkins and via Github Actions, also manually for Scala 2.13 profile. Closes apache#29286 from gemelen/feature/sbt-1.x. Authored-by: Denis Pyshev <git@gemelen.net> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

rshkv · 2021-04-20T10:03:28Z

project/SparkBuild.scala

-  lazy val sparkGenjavadocSettings: Seq[sbt.Def.Setting[_]] = Seq(
-    libraryDependencies += compilerPlugin(
-      "com.typesafe.genjavadoc" %% "genjavadoc-plugin" % unidocGenjavadocVersion.value cross CrossVersion.full),
+  lazy val sparkGenjavadocSettings: Seq[sbt.Def.Setting[_]] = GenJavadocPlugin.projectSettings ++ Seq(


Looks like this is why the unidoc check started failing.

rshkv · 2021-04-20T10:03:44Z

project/SparkBuild.scala

    scalacOptions ++= Seq(
-      "-P:genjavadoc:out=" + (target.value / "java"),


Or this actually

rshkv · 2021-04-20T10:10:03Z

project/SparkBuild.scala

-      analysis.infos.allInfos.foreach { case (k, i) =>
-        i.reportedProblems foreach { p =>
-          val deprecation = p.message.contains("is deprecated")
+      analysis.asInstanceOf[sbt.internal.inc.Analysis].infos.allInfos.foreach { case (k, i) =>
+        i.getReportedProblems foreach { p =>
+          val deprecation = p.message.contains("deprecated")


(This previously was is deprecated in our fork. But checking for deprecated only should give you a superset.)

LorenzoMartini mentioned this pull request Apr 19, 2021

[SPARK-21708][BUILD] Migrate build to sbt 1.x #752

Closed

LorenzoMartini force-pushed the lmartini/sbt-1.x branch from 11da97d to 05fe273 Compare April 19, 2021 19:49

LorenzoMartini commented Apr 19, 2021

View reviewed changes

LorenzoMartini force-pushed the lmartini/sbt-1.x branch from 6b73877 to 94b4feb Compare April 19, 2021 19:55

LorenzoMartini commented Apr 19, 2021

View reviewed changes

LorenzoMartini force-pushed the lmartini/sbt-1.x branch from f4de666 to fd66f3c Compare April 20, 2021 07:32

LorenzoMartini commented Apr 20, 2021

View reviewed changes

LorenzoMartini force-pushed the lmartini/sbt-1.x branch from 465e437 to e45a2f8 Compare April 20, 2021 09:12

rshkv reviewed Apr 20, 2021

View reviewed changes

project/SparkBuild.scala

scalacOptions ++= Seq(

"-P:genjavadoc:out=" + (target.value / "java"),

Copy link

rshkv Apr 20, 2021

Choose a reason for hiding this comment

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

Or this actually

rshkv reviewed Apr 20, 2021

View reviewed changes

rshkv approved these changes Apr 20, 2021

View reviewed changes

LorenzoMartini mentioned this pull request Apr 20, 2021

Re-enable unidoc for sbt build #760

Open

LorenzoMartini merged commit f779ffb into master Apr 20, 2021

LorenzoMartini deleted the lmartini/sbt-1.x branch April 20, 2021 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21708][BUILD] Migrate build to sbt 1.x #759

[SPARK-21708][BUILD] Migrate build to sbt 1.x #759

LorenzoMartini commented Apr 19, 2021 •

edited

Loading

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 20, 2021

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 19, 2021

LorenzoMartini Apr 20, 2021

rshkv Apr 20, 2021

rshkv Apr 20, 2021

rshkv Apr 20, 2021

		scalacOptions ++= Seq(
		"-P:genjavadoc:out=" + (target.value / "java"),

[SPARK-21708][BUILD] Migrate build to sbt 1.x #759

[SPARK-21708][BUILD] Migrate build to sbt 1.x #759

Conversation

LorenzoMartini commented Apr 19, 2021 • edited Loading

Original PR description

Upstream SPARK-XXXXX ticket and PR link (if not applicable, explain)

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorenzoMartini commented Apr 19, 2021 •

edited

Loading