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

[SPARK-26144][BUILD] build/mvn should detect scala.version based on scala.binary.version #23118

Closed
wants to merge 1 commit into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 22, 2018

What changes were proposed in this pull request?

Currently, build/mvn downloads and uses Scala 2.12.7 in Scala-2.11 Jenkins job. The root cause is build/mvn got the first match from pom.xml blindly.

exec: curl -s -L https://downloads.lightbend.com/zinc/0.3.15/zinc-0.3.15.tgz
exec: curl -s -L https://downloads.lightbend.com/scala/2.12.7/scala-2.12.7.tgz
exec: curl -s -L https://www.apache.org/dyn/closer.lua?action=download&filename=/maven/maven-3/3.5.4/binaries/apache-maven-3.5.4-bin.tar.gz

How was this patch tested?

Manual.

$ build/mvn clean
exec: curl --progress-bar -L https://downloads.lightbend.com/scala/2.12.7/scala-2.12.7.tgz
...
$ git clean -fdx
$ dev/change-scala-version.sh 2.11
$ build/mvn clean
exec: curl --progress-bar -L https://downloads.lightbend.com/scala/2.11.12/scala-2.11.12.tgz

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 22, 2018

cc @srowen , @shaneknapp , @dbtsai , @HyukjinKwon

I noticed this during investigating Scala-2.11 Jenkins jobs. Sorry for pinging you guys at Thanksgiving holiday. As I'm the one who merged Scala-2.12 default PR, I hope that we are able to keep Scala-2.11 profile cleanly like before.

@HyukjinKwon
Copy link
Member

Haha today's not holiday here :D.

@dongjoon-hyun
Copy link
Member Author

Thank you, @HyukjinKwon . :D

@@ -116,7 +116,8 @@ install_zinc() {
# the build/ folder
install_scala() {
# determine the Scala version used in Spark
local scala_version=`grep "scala.version" "${_DIR}/../pom.xml" | head -n1 | awk -F '[<>]' '{print $3}'`
local scala_binary_version=`grep "scala.binary.version" "${_DIR}/../pom.xml" | head -n1 | awk -F '[<>]' '{print $3}'`
local scala_version=`grep "scala.version" "${_DIR}/../pom.xml" | grep ${scala_binary_version} | head -n1 | awk -F '[<>]' '{print $3}'`
Copy link
Member

Choose a reason for hiding this comment

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

So after doing dev/change-scala-version.sh 2.11, scala.version and scala.binary.version are expected to be not matched?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review, @viirya .

Previously, it doesn't matched; scala.binary.version=2.11 and scala.version=2.12.7.
Now, they will be matched; scala.binary.version=2.11 and scala.version=2.11.12.
By default (without change-scala-version.sh 2.11), it's scala.binary.version=2.12 and scala.version=2.12.7.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to update dev/change-scala-version.sh to let it update <scala.version>?

Copy link
Member

@viirya viirya Nov 22, 2018

Choose a reason for hiding this comment

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

I have above question because I think after current change, scala.version and scala.binary.version are still not matched in pom.xml, isn't?

Or we just need to do as current change to download & install correct Scala and this is enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

@viirya . I think you are confused about the meaning of scala.version.
In maven/sbt, scala.version is controlled by profile -Pscala-2.11.
This build/mvn doesn't know the profile -Pscala-2.11. That's the problem this PR want to fix.

Copy link
Member

Choose a reason for hiding this comment

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

oh, I see. Thanks @dongjoon-hyun

@SparkQA
Copy link

SparkQA commented Nov 22, 2018

Test build #99173 has finished for PR 23118 at commit f4f51af.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@SparkQA
Copy link

SparkQA commented Nov 22, 2018

Test build #99193 has finished for PR 23118 at commit f4f51af.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Merged to master.

@asfgit asfgit closed this in 1d766f0 Nov 22, 2018
@dongjoon-hyun dongjoon-hyun deleted the SPARK-26144 branch November 22, 2018 22:58
@dbtsai
Copy link
Member

dbtsai commented Nov 26, 2018

Late to the party! Thanks @dongjoon-hyun for taking care of this.

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
…on `scala.binary.version`

## What changes were proposed in this pull request?

Currently, `build/mvn` downloads and uses **Scala 2.12.7** in `Scala-2.11` Jenkins job. The root cause is `build/mvn` got the first match from `pom.xml` blindly.

- https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-ubuntu-scala-2.11/6/consoleFull
```
exec: curl -s -L https://downloads.lightbend.com/zinc/0.3.15/zinc-0.3.15.tgz
exec: curl -s -L https://downloads.lightbend.com/scala/2.12.7/scala-2.12.7.tgz
exec: curl -s -L https://www.apache.org/dyn/closer.lua?action=download&filename=/maven/maven-3/3.5.4/binaries/apache-maven-3.5.4-bin.tar.gz
```

## How was this patch tested?

Manual.
```
$ build/mvn clean
exec: curl --progress-bar -L https://downloads.lightbend.com/scala/2.12.7/scala-2.12.7.tgz
...
$ git clean -fdx
$ dev/change-scala-version.sh 2.11
$ build/mvn clean
exec: curl --progress-bar -L https://downloads.lightbend.com/scala/2.11.12/scala-2.11.12.tgz
```

Closes apache#23118 from dongjoon-hyun/SPARK-26144.

Authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants