-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-12000] [Build] remove scala-compiler and scalap from json4s deps #10071
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
Conversation
|
Test build #46978 has finished for PR 10071 at commit
|
|
Woah, weird: Were we somehow implicitly depending on an undeclared transitive dependency? |
|
catalyst uses scala-compiler but it doesn't declare it explicitly. fixed. |
|
I think that there's a way to configure Maven enforcer such that the build will break if a project uses code from a dependency that it doesn't declare. I might look into configuring that to prevent this class of issue in the future. |
|
Actually, we might be able to get away with completely removing our dependency on the Scala compiler. It looks like the usage which broke the build is only used in some debugging code which is no longer used anywhere: /**
* Dumps the bytecode from a class to the screen using javap.
*/
object DumpByteCode {
import scala.sys.process._
val dumpDirectory = Utils.createTempDir()
dumpDirectory.mkdir()
def apply(obj: Any): Unit = {
val generatedClass = obj.getClass
val classLoader =
generatedClass
.getClassLoader
.asInstanceOf[scala.tools.nsc.interpreter.AbstractFileClassLoader]
val generatedBytes = classLoader.classBytes(generatedClass.getName)
val packageDir = new java.io.File(dumpDirectory, generatedClass.getPackage.getName)
if (!packageDir.exists()) { packageDir.mkdir() }
val classFile =
new java.io.File(packageDir, generatedClass.getName.split("\\.").last + ".class")
val outfile = new java.io.FileOutputStream(classFile)
outfile.write(generatedBytes)
outfile.close()
// scalastyle:off println
println(
s"javap -p -v -classpath ${dumpDirectory.getCanonicalPath} ${generatedClass.getName}".!!)
// scalastyle:on println
}
}As far as I know, that code may no longer even work in light of the recent Janino changes. Given this, my preference would be to simply remove that |
|
That sounds good to me, but it should be addressed in a separate PR. This is to fix the compiler bug we hit in |
|
Alright, I'll turn around and file a followup PR on the tails of this one to remove that. |
|
This didn't actually fix the bug ... The first time it worked but the bug appeared again in the second run ... |
|
Yeah, I was just coming here to comment on the same issue. I think that I had a similar experience yesterday with my JDK bumping fix, where it seemed to work on the first run but started failing later. |
|
@JoshRosen sgtm |
|
It does still work and I use it regularly. What is the motivation? |
|
Oh, I wanted to see if we could get rid of the Scala compiler dependency. Maybe we can modify it to not use that compiler classloader? EDIT: I guess we could also mark that dep. as provided if this is for use in the REPL. |
|
Test build #46984 has finished for PR 10071 at commit
|
|
retest this please |
|
Test build #47059 has finished for PR 10071 at commit
|
|
I found the exact fix for the bug (#10114). But I think it is still useful to remove scala-compiler/scalap deps from runtime. @JoshRosen Could you make a new JIRA for it? |
|
I created https://issues.apache.org/jira/browse/SPARK-12135 for removing scala-compiler and scalap from runtime deps. I'm closing this PR since this is not the fix for SPARK-12000. |
This could be a fix for SPARK-12000 because json4s pulls in scala-compiler:2.10.0 and scalap, which could cause the compiler bug. See json4s/json4s#108.
Another option is to upgrade json4s to 3.3.0, which removes scala-compiler/scalap from its runtime deps but depends on Scala 2.10.6.
cc @JoshRosen