Less user-facing dependencies via classloader isolation

[alexarchambault]

This PR manages to isolate the core modules and dependencies of Ammonite (repl, interp, upickle, fastparse, …) from the user-facing API. Users effectively don't see the former, and can even load other versions of those if they want.

It moves the user-facing parts of interp and repl to new modules interp-api and repl-api.

It then manages to:

• launch Ammonite and its tests with a two-level classloader hierarchy (first classloader loading the user-facing things - interp-api, repl-api, etc., second one the rest, including the core of Ammonite), by using a custom test runner for the tests, and by packaging Ammonite via a coursier bootstrap rather than an assembly,
• have Ammonite pick the first classloader to later compile and load user code. This is done by default in the tests, and when one passes the --thin option to the Ammonite launcher.

[alexarchambault]

Overall, this allows not to expose plenty of dependencies, most notably fastparse, upickle, jline, etc. Users can in turn load their own versions of those if they want to.

This diff shows which ones are removed:

$ diff <(coursier resolve com.lihaoyi:ammonite_2.12.8:1.6.4-6-b7dafbe5 |sort) <(coursier resolve com.lihaoyi:ammonite-repl-api_2.12.8:1.6.4-6-b7dafbe5 |sort)
1d0
< com.github.javaparser:javaparser-core:3.2.5:default
3d1
< com.lihaoyi:acyclic_2.12:0.1.5:default
5d2
< com.lihaoyi:ammonite-interp_2.12.8:1.6.4-6-b7dafbe5:compile
8,10d4
< com.lihaoyi:ammonite-repl_2.12.8:1.6.4-6-b7dafbe5:compile
< com.lihaoyi:ammonite-runtime_2.12:1.6.4-6-b7dafbe5:compile
< com.lihaoyi:ammonite-terminal_2.12:1.6.4-6-b7dafbe5:compile
12d5
< com.lihaoyi:ammonite_2.12.8:1.6.4-6-b7dafbe5:compile
14d6
< com.lihaoyi:fastparse_2.12:2.1.0:default
18,25c10
< com.lihaoyi:scalaparse_2.12:2.1.0:default
< com.lihaoyi:sourcecode_2.12:0.1.5:default
< com.lihaoyi:ujson_2.12:0.7.1:default
< com.lihaoyi:upack_2.12:0.7.1:default
< com.lihaoyi:upickle-core_2.12:0.7.1:default
< com.lihaoyi:upickle-implicits_2.12:0.7.1:default
< com.lihaoyi:upickle_2.12:0.7.1:default
< com.lihaoyi:utest_2.12:0.6.4:default
---
> com.lihaoyi:sourcecode_2.12:0.1.3:default
29,33d13
< net.java.dev.jna:jna:4.2.2:default
< org.javassist:javassist:3.21.0-GA:default
< org.jline:jline-reader:3.6.2:default
< org.jline:jline-terminal-jna:3.6.2:default
< org.jline:jline-terminal:3.6.2:default
38d17
< org.scala-sbt:test-interface:1.0:default

[alexarchambault]

Ultimately, after some more clean-up of user facing stuff, we could enable mima on the user-facing modules, to ensure the API exposed to users doesn't change much.

[alexarchambault]

So a bit of explanation. This PR does two things:

• add new interp-api and repl-api modules, aimed at being user-facing, while interp and repl should be hidden from users.
• manage to actually have only those modules (and their transitive dependencies) be exposed to users, both in the tests and when using the Ammonite launcher.

The first point mostly moves things around (f0e41d9, 0d6c3ed, dd12153, 5e168d5, 972562a). It also introduces new bridges (ecc100b, 834fc4b), so that the internals and the dependencies of the frontend and source-related stuff can be hidden too (users only interact with these APIs, rather than by directly tangling with some internal stuff).

The second point is also two-fold, and some aspects can probably be tweaked or even done other ways. It consists in ensuring, both in the tests and with the Ammonite launcher, that Ammonite is started with a two classloader setup (with a first classloader with the interp-api and repl-api modules and their dependencies, and a second classloader with the other modules and dependencies, having the first classloader as parent). It then has the first classloader be picked up as a start, when compiling and classloading user code (that way, things from the second classloader are effectively hidden from users).

Getting a two classloader setup in the tests is done by commit 0bb3b6e. This relies on com-lihaoyi/mill#572. This basically passes the repl-api classpath (more exactly the test-api classpath…) via a Java property to the test runner, that then creates the two classloaders itself.

Getting those two classloaders with the Ammonite launcher is done in 238c166. This basically replaces the assembly, which merges all dependencies together, by "bootstraps" from coursier. Bootstraps embed the various dependency JARs as resources, along with files listing which ones should be loaded together (see mill -i 'amm[2.12.8].bootstrap' && unzip -l out/amm/2.12.8/bootstrap/dest/bootstrap.jar, things under coursier/bootstrap/launcher/jars/). They contain a tiny Java program, able to read those files, create the corresponding classloaders using the JARs from the resources, and run a main class from them.

cf5405a then ensures that compilation and classloading of user code starts from the classloader of repl-api, set up above. (In the tests, this is done by 0bb3b6e, look around Frame.createInitial).

Lastly, 4631aac, 35389c3, and 166b6e2, do minor refactorings that make things easier for other commits here. And 19f012d proceeds with hiding the upickle dependencies from users, by moving bits of the (user-facing) util module to (non user-facing) runtime.

[alexarchambault]

Overall, this adds a bit of complexity (new modules, only a subset of those are accessible to users, classloader hacks in tests and in the launcher). My opinion is it's worth it… This should make the user-facing stuff of Ammonite thinner, and hopefully more stable (in the sense "less changing"). Which should in turn make scripts (and, hmm, notebooks) easier to maintain.

[alexarchambault]

@lihaoyi If you're ok with the general idea, I can totally split that PR, for easier review:

• the minor refactorings of 4631aac and all first,
• adding interp-api and repl-api,
• picking the right classloader if it's there, cf5405a,
• two classloader setup in the tests,
• two classloader setup in the launcher,
• no more upickle in the user-facing API.

[alexarchambault]

Lastly, about the bootstrap stuff replacing assemblies, I went for that as I'm familiar with bootstraps (and they have the advantage of not tangling with bytecode too). But I guess using stuff like jarjar to just rename a bunch of classes (those not accessible to users) could work too.

[lihaoyi]

@alexarchambault I'm fine with the general idea, but this is a large PR and I might take some time to get around to reviewing it. Hope that's ok!

[alexarchambault]

Yep, no problem! (This doesn't block anything on my side…)

[lihaoyi]

@alexarchambault I skimmed this, it looks good to me. My one request would be to make the classpath-isolation configurable, and maybe opt-in from the command line via a flag in Cli.scala. I think having the messy classpath is a reasonable default for "casual" usage.

My own usage involves files/subprocesses/http/json reasonably heavily even if it isn't necessary for ammonite's own user-facing API, and if want to use Ammonite as a tool for learners/small-projects then the benefit of having all those utilities already bundled outweighs the costs of a slightly polluted classpath.

Another place where the classpath pollution is helpful is when embedding Ammonite in a larger application: in this scenario having access to all your application classpath is the point of the embedding.

Advanced users like yourself who want a cleaner classpath to better control the runtime environment can then just flip the switch.

If you can get tests passing, I think I would be happy to merge this

[alexarchambault]

There's still some slight perf regression due to the use of "bootstraps" (that embed the dependencies as resources, so we have "jar-in-jars") instead of assemblies (with no such indirection). I have some workarounds in the making…

[lihaoyi]

Sounds good!

Before merging, also please update the PR description with a more verbose description of the significant changes in the PR. Ideally someone reading would be able to get a good summary of the changes without wading through the large number of changed files

[alexarchambault]

Ok, I updated the description.

[lihaoyi]

@alexarchambault I've noticed some performance regressions that I think arise from this PR. The default REPL (without --thin) is definitely much less responsive than it used to be. Any idea if this could have caused that? We should probably sort that out before tagging another stable release

[lihaoyi]

Now that we have this infrastructure in place, here is a bunch more packages it might make sense to keep out of the --thin classpath:

• shapeless & argonaut; maybe we could make coursier us uPickle instead, since we already use it heavily in the rest of Ammonite/Mill?

• jline? Not sure how easy this would be to remove, but I don't think a user ever needs this directly (only transitively through the JLineFrontEnds)

• coursier/internal/shaded/fastparse and coursier/internal/shaded/sourcecode: we already use the libraries in Ammonite anyway, so coursier could just depend on those to avoid duplication

• scala-compiler: we need this to compile stuff, and the user has access to it through some of the interp.* and repl.* APIs, but it's conceivable we could remove it if we were willing to provide that functionality through a narrower interface

os-lib/pprint/sourcecode seem difficult to remove, since we expose os.Paths in many APIs and want to allow a user to add custom pretty-printers.

coursier.bootstrap.launcher seems like something a user shouldn't need to interact with, but also not clear if it's feasible to remove

[alexarchambault]

@lihaoyi You're probably seeing the same kind of slowdown I saw before https://github.com/lihaoyi/Ammonite/pull/941#issuecomment-505542171… I mostly fixed it when --thin is passed, but there's still a penalty if it's not.

The old assembly logic is still around if needed (mill 'amm[2.13.0].assembly'). I'm fine releasing the assembly until this is sorted out. I can open a PR doing that.

I'll add a lengthier explanation here tomorrow about how the "bootstrap" works, to give a better idea about what (I think) is going on.

[alexarchambault]

About hiding more stuff when --thin is passed:

• shapeless and argonaut (and argonaut-shapeless) are brought by coursier. I'd like to switch Ammonite over to the Java API of coursier, that has no dependencies (but still has some of the same feel as the scala API). That'll remove those dependencies.
• jline is brought by scala-compiler (coursier resolve com.lihaoyi:ammonite-replApi_2.13.0:1.6.9 --what-depends-on jline:jline), it can probably be manually excluded in a first time.
• the coursier/internal stuff correspond to shaded libraries. I'm not sure these can be "unshaded", unless I publish some coursier modules where fastparse isn't shaded. I'd prefer to go the other way around actually, and have Ammonite rely on a coursier module shading more stuff, via its Java API (see above). Plus these are under a namespace marked as internal, and I don't think these are usable anyway (shading doesn't adjust the "scala annotations", that still refer to non shaded namespaces, making compilation fail when code refers to those)
• scala-compiler could probabaly be hidden by reworking a bit the API, yeah

I think os-lib could be removed too. I managed to do it locally some time ago, that requires changing some os.Path to java.nio.file.Path in the API in particular.

[lihaoyi]

Swapping some of the APIs to java.nio.file.Path sounds reasonable. I'd say we add the new java.nio based APIs to the bridge traits, and add a new bridge trait that only gets used in --thin that only has java.nio

[alexarchambault]

Couldn't an implicit os.Path to java.nio conversion do? Enabled only when --thin is not passed.

If necessary, it could even be restricted to the API calls, with some indirection (like defining a implicit case class Path(path: java.nio.file.Path) in ammonite.repl.api, having the api accept ammonite.repl.api.Paths, and add an implicit os.Path to ammonite.repl.api.Path conversion when --thin is not there).