Skip to content

Conversation

@da-liii
Copy link
Contributor

@da-liii da-liii commented Sep 1, 2018

What changes were proposed in this pull request?

Improve build for Scala 2.12. Current build for sbt fails on the subproject repl:

[info] Compiling 6 Scala sources to /Users/rendong/wdi/spark/repl/target/scala-2.12/classes...
[error] /Users/rendong/wdi/spark/repl/scala-2.11/src/main/scala/org/apache/spark/repl/SparkILoopInterpreter.scala:80: overriding lazy value importableSymbolsWithRenames in class ImportHandler of type List[(this.intp.global.Symbol, this.intp.global.Name)];
[error]  lazy value importableSymbolsWithRenames needs `override' modifier
[error]       lazy val importableSymbolsWithRenames: List[(Symbol, Name)] = {
[error]                ^
[warn] /Users/rendong/wdi/spark/repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala:53: variable addedClasspath in class ILoop is deprecated (since 2.11.0): use reset, replay or require to update class path
[warn]       if (addedClasspath != "") {
[warn]           ^
[warn] /Users/rendong/wdi/spark/repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala:54: variable addedClasspath in class ILoop is deprecated (since 2.11.0): use reset, replay or require to update class path
[warn]         settings.classpath append addedClasspath
[warn]                                   ^
[warn] two warnings found
[error] one error found
[error] (repl/compile:compileIncremental) Compilation failed
[error] Total time: 93 s, completed 2018-9-3 10:07:26

How was this patch tested?

./dev/change-scala-version.sh 2.12

##  For Maven
./build/mvn -Pscala-2.12 [mvn commands]
##  For SBT
sbt -Dscala.version=2.12.6

@da-liii
Copy link
Contributor Author

da-liii commented Sep 1, 2018

@srowen This is a better solution for #22280

No hard-coded 2.11 or 2.12 any more.

@srowen
Copy link
Member

srowen commented Sep 1, 2018

Why the move of the source code in the 2.11 tree? looks like it's not in the right package dir now.

@SparkQA
Copy link

SparkQA commented Sep 1, 2018

Test build #95579 has finished for PR 22310 at commit 7c0b9b9.

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

@da-liii
Copy link
Contributor Author

da-liii commented Sep 2, 2018

@srowen It is about the sbt convention.

see my demo project: https://github.com/sadhen/spark-25298

@srowen
Copy link
Member

srowen commented Sep 2, 2018

@sadhen we use Maven though, not SBT. (SBT works through a plugin). But my point is the path .../repl/... which doesn't reflect the class's path. Scala and Java convention has always put code in a source tree according to its package. This also already works right now. I'm confused why you say this needs to change, and think it should simply be reverted.

@da-liii
Copy link
Contributor Author

da-liii commented Sep 3, 2018

@srowen Sorry I should have explained why I made these changes.

The follow steps failed to compile:

$ ./dev/change-scala-version.sh 2.12
$ ./build/sbt -Dscala-2.12 -Dscala.version=2.12.6
> project repl
> compile

The error messages are appended above.

After these changes, the commands are simplified and the compile works:

$ ./dev/change-scala-version.sh 2.12
$ ./build/sbt -Dscala.version=2.12.6
> project repl
> compile

@da-liii
Copy link
Contributor Author

da-liii commented Sep 3, 2018

The problem of package hierarchy is fixed.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Yeah OK so instead of ...

src/main/scala/...
scala-2.11/src/main/scala

we get

src/main/scala
src/main/scala-2.11

Yeah that's more logical, and if it matches SBT conventions better, OK.

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95595 has finished for PR 22310 at commit 77edff4.

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

@srowen
Copy link
Member

srowen commented Sep 3, 2018

Merged to master

@asfgit asfgit closed this in 546683c Sep 3, 2018
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