[SPARK-14574][BUILD][test-maven] Stop cross-building pure Java modules; simplify build target dirs #12334
Conversation
JoshRosen
changed the title
|
Jenkins, retest this please. |
|
Test build #55647 has finished for PR 12334 at commit
|
|
Test build #55650 has finished for PR 12334 at commit
|
|
Jenkins, retest this please. |
|
Test build #55668 has finished for PR 12334 at commit
|
|
Overall looks ok, just have a few minor questions:
|
| <parent> | ||
| <groupId>org.apache.spark</groupId> | ||
| <artifactId>spark-parent_2.11</artifactId> | ||
| <artifactId>spark-parent</artifactId> |
There was a problem hiding this comment.
Interesting, so parent is no longer Scala version specific? I would have thought it might be as it provides vars for the Scala version. I expect you've thought this through more than I have though
There was a problem hiding this comment.
Empirically, it looks like the generated POMS which are used for publishing end up with all variables in the <dependencies> section having been expanded according to whichever profile was active at publication time. For example, in http://central.maven.org/maven2/org/apache/spark/spark-core_2.10/1.6.1/spark-core_2.10-1.6.1.pom, there are no references to scala.binary.version in the <dependencies> section. There are references in the <build> section but I don't think that this is relevant to consumers of this POM since I don't expect people to depend on our child POMs in their own builds.
There was a problem hiding this comment.
Based on some of the comments in https://issues.apache.org/jira/browse/MNG-2971, https://issues.apache.org/jira/browse/MNG-4223, and other tickets I believe that the intent is that properties should be expanded when installing / deploying POMs.
It looks like a somewhat more explicit way of ensuring this expansion would be to use the http://www.mojohaus.org/flatten-maven-plugin/ plugin.
There was a problem hiding this comment.
It looks like there's a more complete summary of Maven's desired behavior at https://cwiki.apache.org/confluence/display/MAVENOLD/Artifact-Coordinate+Expression+Transformation
|
That cleanup sure is nice. Off the top of my head I don't know of anything else that needs changing, but we'll see how this flies. |
Whether we compile for multiple versions of Scala as part of the same CI build does not necessarily imply usage of SBT's cross-build feature (e.g. being able to do "sbt + package" to build for all Scala versions at the same time). I think that it's going to be prohibitively difficult to support the use of SBT's |
|
Going to close this for now as "later", since it's going to be prohibitively difficult to ensure that we continue to publish flattened poms and there's some measure of risk here given how parts of this (and the current build) appear to work by accident. |
That's doable, but my main gripe with the current way cross-compilation is done is that it requires you to pollute your working file set. You can't just say "build for Scala 2.10" with just command line options; you have to change all the poms. There are ways to reduce the annoyance (e.g. commit the pom changes and later remember to remove them from the branch), but if the build infrastructure handled that more cleanly, it would be great. |
….11 dep. Spark SQL's POM hardcodes a dependency on `spark-sketch_2.11`, which causes Scala 2.10 builds to include the `_2.11` dependency. This is harmless since `spark-sketch` is a pure-Java module (see #12334 for a discussion of dropping the Scala version suffixes from these modules' artifactIds), but it's confusing to people looking at the published POMs. This patch fixes this by using `${scala.binary.version}` to substitute the correct suffix, and also adds a set of Maven Enforcer rules to ensure that `_2.11` artifacts are not used in 2.10 builds (and vice-versa). /cc ahirreddy, who spotted this issue. Author: Josh Rosen <joshrosen@databricks.com> Closes #12563 from JoshRosen/fix-sketch-scala-version.
4 participants
Spark has a few modules which do not depend on Scala, including
launcher,unsafe,sketch, and thenetwork-*libraries. However, we currently cross build and publish these artifacts for different Scala versions.This patch refactors Spark's build so that pure-Java modules can be published once and so that their artifact ids do not include
_2.xxsuffixes. A consequence of this change is that pure Java modules' build output directories will be/target/classesrather than/target/scala-*/classes. For uniformity, I updated all projects, including Scala ones, to use this output directory convention. I think that the only reason to prefer SBT'scrossPathsconvention is if you're using SBT's cross-build support (which, AFAIK, doesn't currently work for Spark and isn't something that we're aiming to support).The
targetdirectory changes, in turn, allowed me to significantly simplify the launcher code and removed the need to detect and setSPARK_SCALA_VERSIONin the launcher scripts. This simplification will have a huge payoff when if we eventually have to simultaneously support Scala 2.10.x, 2.11.x, and 2.12.x, since there's now much less version detection logic to maintain./cc @rxin @marmbrus @ahirreddy @srowen @vanzin