-
Notifications
You must be signed in to change notification settings - Fork 1.1k
REPL: extract into separate subproject #18536
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
I guess I need some help to greenify the test setup. But before we sink any additional time into that, a general review of the proposed changes would be great 🙇🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence on this. I like the idea of avoiding parts of the compiler accidentally depending on jline or other REPL dependencies. But I'm not sure about introducing two new jars to do it. So then we could imitate Scala 2 and have separate projects but ship it all as 1 jar - but is that build complication worth it? Perhaps Scala 2 is precedent for yes...
compiler/test-coursier/dotty/tools/coursier/CoursierScalaTests.scala
Outdated
Show resolved
Hide resolved
status: there's a few places where scala3-compiler uses the repl directly: ``` [error] -- [E006] Not Found Error: /home/mp/Projects/dotty/compiler/src/dotty/tools/MainGenericCompiler.scala:181:8 [error] 181 | repl.Main.main(properArgs.toArray) [error] | ^^^^ [error] | Not found: repl [error] | [error] | longer explanation available when compiling with `-explain` [error] -- [E006] Not Found Error: /home/mp/Projects/dotty/compiler/src/dotty/tools/MainGenericRunner.scala:193:8 [error] 193 | repl.Main.main(properArgs.toArray) [error] | ^^^^ [error] | Not found: repl [error] | [error] | longer explanation available when compiling with `-explain` [error] -- [E008] Not Found Error: /home/mp/Projects/dotty/compiler/src/dotty/tools/dotc/quoted/Interpreter.scala:30:19 [error] 30 |import dotty.tools.repl.AbstractFileClassLoader [error] | ^^^^^^^^^^^^^^^^ [error] | value repl is not a member of dotty.tools [error] -- [E008] Not Found Error: /home/mp/Projects/dotty/compiler/src/dotty/tools/dotc/util/ClasspathFromClassloader.scala:8:19 [error] 8 |import dotty.tools.repl.AbstractFileClassLoader [error] | ^^^^^^^^^^^^^^^^ [error] | value repl is not a member of dotty.tools [error] four errors found [error] (scala3-compiler / Compile / compileIncremental) Compilation failed ```
there doesn't seem to be anything repl-specific about it, and it's used in a few places in the compiler
as per Dale's comment scala#18536 (comment)
as per Dale's comment scala#18536 (comment)
55109f7
to
6d675ad
Compare
Thanks for the feedback! |
I don't think we should create a dist jar. And, on balance, I don't think the benefits mentioned from this change are worth the efforts and risks of making the change. |
A separate |
Alright. Thanks for taking the time and mental cycles to consider this proposal! |
(I didn't mean to push back as I didn't review the change, I just wanted to point out this consequence.) |
Currently the REPL implementation is part of the
scala3-compiler
project. This PR extracts it into a separatescala3-repl
project which helps to separate concerns. Main benefits:Trim dependency tree for scala3-compiler:
jline-reader
,jline-terminal
andjline-terminal-jna
are currently direct dependencies ofscala3-compiler
(see https://repo1.maven.org/maven2/org/scala-lang/scala3-compiler_3/3.3.1/scala3-compiler_3-3.3.1.pom). This PR moves those down toscala3-repl
, which helps all downstream builds as well.Simplify future REPL improvements by isolating the REPL code (which contributors might be less afraid to touch) from the core compiler code.
Implementation note 1: I moved the
MainGeneric*
intodist
- not sure if that's the most elegant solution to ensure we have a single runner/compiler for everything.Implementation note 2: not sure if scaladoc really needs the dist and repl jars in it's classpath.
n.b. the original idea came up in #17624