Corral, shade & embed jline. #4563
Conversation
Code that depends on jline is now in package `scala.tools.nsc.interpreter.jline`. To make this possible, remove the `entries` functionality from `History`, and add the `historicize` method. Also provide an overload for `asStrings`. Clean up a little along the way in `JLineHistory.scala` and `JLineReader.scala`. Next step: fall back to an embedded jline when the expected jline jar is not on the classpath. The gist of the refactor: https://gist.github.com/adriaanm/02e110d4da0a585480c1
|
Manual testing of falling back when old jline is on class path: |
|
Manually testing sbt: |
|
History repeats itself in parallel dimensions. In case it is any solace. ---------- Forwarded message ---------- jline 2.12.1 is now part of the kulla repositories. So when building full JDK, no additional parameters to make are needed, and the jshell command should work. Regarding the scripts in langtools/repl/scripts, as jline is repackaged to jdk.internal.jline, the official jline.jar cannot be used anymore. [snip instructions] Unfortunately, this won't work on Windows, as a little bit of native code is needed there. Please let me know if it is important to have these scripts work also for Windows. |
|
Unless there's a conflict, we still use the jline provided on the class path as before. |
|
I'm trying to reproduce a build on the Spark side and check that things work as intended. That's hard because some things moved around and the spark-repl won't compile anymore. I'll let you know as soon as I get some fixes in there, but my first impression is that these changes might break way more clients than simply shading the dependency. I really don't think there's anyone relying on that. |
|
Happy to try myself if you can post some hints on how to get started. Can also see about moving some classes back if needed -- would be good to know which of these were considered public, because we certainly assumed they are internal. In any case, the only classes that moved depend directly on jline. It doesn't seem like a good idea to reuse them while also putting a conflicting jline on the class path. We only guarantee compiler API stability through inclusion in the community build (or manually, in this case), so I don't think it's an issue per se to move internal classes around. My take on this is that there are orders of magnitude fewer programmatic consumers of the repl (sbt, spark,..) than there are repl users on, say, windows who may once in a while benefit from swapping the jline jar (maybe after 2.11 goes EOL but jline keeps evolving). We also only promise best-effort compiler API stability, i.e., we can only maintain stability for dependencies we know about (community build, manual). |
|
I understand there are no compatibility guarantees, I'm just noticing that it's way more work than I anticipated. Part of the problem is that spark-repl is 50% of a fork, with a few places where changes are significant. I can get rid of a lot of duplicated code (involving references to JLine code as well), but some things are impossible to reuse without some more changes in the Scala REPL. For instance, static members of Lastly, I'd like to add Spark to the community build, but so far there are other forks (like genjavadoc) that are compiler plugins, so they need to be either part of the community build as well, or removed (my preference goes to this one). |
|
I recognize these are all real challenges, but I would hope this PR helps in overcoming them since it makes the InteractiveReader interface configurable and also makes the core of the REPL independent of JLine. It also sounds like some of the problems you mentioned are orthogonal to this. Since we have more REPL refactorings in the works (presentation compiler for auto-complete) I suggest the Scala team takes a look at the spark repl. Are there instructions anywhere on how to build it? Ideally as a separate module from the rest of Spark? I can spend all day today on this. |
|
I doubt one day would be enough, but all the code is in https://github.com/apache/spark/tree/master/repl You can build it with Maven or Sbt, it's pretty straight forward. You'll notice though that you won't be able to use 2.11.7-SNAPSHOT as a dependency because of the said compiler plugin dependencies, so you'll need to find some workaround (mine is to replace 2.11.6 directly on the classpath, inside the IDE). Spark build instructions for 2.11 Edit: Some of the issues I mentioned may seem orthogonal, but I didn't find a way to use the Scala REPL without fixing those first (other than copy-pasting a more recent version of the REPL from your PR). |
|
Ok, I got somewhat farther. I'm happy with the amount of code I managed to scrap. Things seem to work fine on the Spark side, but the problem is I don't understand why :) I see this in the output Nevertheless, I get completion and history on the command prompt. The Hive CLI works as well, meaning that the assembly packaged jline 0.9.94. I manually checked and the |
|
Cool & bizarre at the same time! |
|
Past experience tells me this is trouble down the line. We need to find out what fails and why it works... |
|
Agreed. I tried locally with both jars on the class path and cannot reproduce: |
|
Could you recompile with a compiler that prints the stack trace for the exception in the Failure? |
|
When I swap the order of the jars, I get: |
|
Since spark knows the first try is going to fail, can you try with |
|
I should probably make that |
|
Here's the snippet for ILoop that will print the stack trace for these failures: |
As usual, the repl will use whatever jline 2 jar on the classpath, if there is one. Failing that, there's a fallback and an override. If instantiating the standard `jline.InteractiveReader` fails, we fall back to an embedded, shaded, version of jline, provided by `jline_embedded.InteractiveReader`. (Assume `import scala.tools.nsc.interpreter._` for this message.) The instantiation of `InteractiveReader` eagerly exercises jline, so that a linkage error will result if jline is missing or if the provided one is not binary compatible. The property `scala.repl.reader` overrides this behavior, if set to the FQN of a class that looks like `YourInteractiveReader` below. ``` class YourInteractiveReader(completer: () => Completion) extends InteractiveReader ``` The repl logs which classes it tried to instantiate under `-Ydebug`. # Changes to source & build The core of the repl (`src/repl`) no longer depends on jline. The jline interface is now in `src/repl-jline`. The embedded jline + our interface to it are generated by the `quick.repl` target. The build now also enforces that only `src/repl-jline` depends on jline. The sources in `src/repl` are now sure to be independent of it, though they do use reflection to instantiate a suitable subclass of `InteractiveReader`, as explained above. The `quick.repl` target builds the sources in `src/repl` and `src/repl-jline`, producing a jar for the `repl-jline` classes, which is then transformed using jarjar to obtain a shaded copy of the `scala.tools.nsc.interpreter.jline` package. Jarjar is used to combine the `jline` jar and the `repl-jline` into a new jar, rewriting package names as follows: - `org.fusesource` -> `scala.tools.fusesource_embedded` - `jline` -> `scala.tools.jline_embedded` - `scala.tools.nsc.interpreter.jline` -> `scala.tools.nsc.interpreter.jline_embedded` Classes not reachable from `scala.tools.**` are pruned, as well as empty dirs. The classes in the `repl-jline` jar as well as those in the rewritten one are copied to the repl's output directory. PS: The sbt build is not updated, sorry. PPS: A more recent fork of jarjar: https://github.com/shevek/jarjar.
|
So, @dragos, does this mean the PR LGTY? We are hoping to stage the release today or tomorrow. |
|
/cc @ScrapCodes |
|
no LGTM yet. I need to track down those failures, I'll look into it today. |
|
Looks good to me, I have tried with spark. This should relieve us of the trouble two jlines on classpath. @dragos I am unsure of your concerns, I can share the code that I used to build spark with this PR and it works fine. Basically I could get rid of explicit dependence on jline. This should not be harmful hopefully ? |
|
@ScrapCodes it works all right, check my comment though for my concerns. Does your log show two |
|
@adriaanm After adding debugging code on the Scala REPL side I can see the second one succeeds. But once I remove it the stream still prints two |
|
LGTM. If you want to have a look at the Spark REPL code (still in progress), check this commit, or even better, this file. The Scala REPL interface that's used is down to:
I am not happy with the last one ( |
|
Great! I have a slight amount of polish for this PR waiting for me on my computer at the office. As soon as I'm at my desk, I'll push it. I'd be curious to understand this weird failure you're seeing, though, and whether it's alleviated by setting the |
|
@adriaanm Are you ok with the API surface? Should we add a hook for custom initialization routines? |
|
Sure let's add those. Thanks for the links, that'll do for inspiration I think :) |
|
On second thought, I'm still in favor of improving customizing the repl, but this PR is only about shading JLine. Its intent was not to nail down the REPL's API surface -- that's going to take more time, and this release needs to be cut today. |
|
/nothingtoseehere -- the build was green, and my push -f was only rewording and more verbosity for a debug message |
As usual, we still expect a jline 2 jar on the classpath for the repl's console.
However, we now support classpaths that do not provide a (compatible) one.
The core of the repl (
src/repl) no longer depends on jline.The jline interface is now in
src/repl-jline.If the property
repl.readeris set to the FQN of a class thatlooks like
YourInteractiveReaderbelow, we try to reflectively instantiate it.Otherwise, we interface to the standard jline on the class path by
instantiating
scala.tools.nsc.interpreter.jline.InteractiveReader.If that fails (a linkage error will result if jline is missing
or if it's not binary compatible -- the constructor exercises
quite a bit of the interface, so that should be sufficient
to detect versions).
If this also fails, we instantiate the class
scala.tools.nsc.interpreter.jline_embedded.InteractiveReader,which interfaces to the embedded jline. This package is derived
from the bytecode generated for
scala.tools.nsc.interpreter.jline._The repl logs which classes it tried to instantiate under
-Ydebug.The embedded jline + our interface to it are generated by the
quick.repltarget.The build now also enforces that only
src/repl-jlinedepends on jline.The sources in
src/replare now sure to be independent of it,though they do use reflection to instantiate a suitable subclass
of
InteractiveReader, as explained above.The
quick.repltarget builds the sources insrc/replandsrc/repl-jline,producing a jar for the
repl-jlineclasses, which is then transformed usingjarjar to obtain a shaded copy of the
scala.tools.nsc.interpreter.jlinepackage.Jarjar is used to combine the
jlinejar and therepl-jlineinto a new jar,rewriting package names as follows:
org.fusesource->scala.tools.fusesource_embeddedjline->scala.tools.jline_embeddedscala.tools.nsc.interpreter.jline->scala.tools.nsc.interpreter.jline_embeddedClasses not reachable from
scala.tools.**are pruned, as well as empty dirs.The classes in the
repl-jlinejar as well as those in the rewritten oneare copied to the repl's output directory.
PS: The sbt build is not updated, sorry.
PPS: A more recent fork of jarjar: https://github.com/shevek/jarjar.