-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Default to no fsc #6747
Default to no fsc #6747
Conversation
are you spying on Scala team meetings? 👀 we just discussed/suggested this this morning. in the longer run it might be nice to just rip up all the compilation-daemon stuff and throw it away, but we're running out of time for would-be-nices for 2.13. |
👍 to defaulting to |
@@ -16,8 +16,9 @@ class GenericRunnerSettings(error: String => Unit) extends Settings(error) { | |||
"-howtorun", | |||
"how", | |||
"how to run the specified code", | |||
List("object", "script", "jar", "repl", "guess"), | |||
List("object", "script", "jar", "repl", "fsc", "guess"), |
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 think -howtorun:fsc
is not easy to understand. Maybe script-compdaemon
is more clear?
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 considered -howtorun:resident
because fsc
is a brand name. When fsc
is a module, it will need the brand exposure, but resident
is generic; ideally, there will be multiple REPL frontends and backends, and some of the backends will support resident
flag. Or am I getting ahead of myself?
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 like script
in the name because that's "how it runs", as a script, just in a special way. alternatively add a -compdaemon
setting, but i think we want to get rid of it entirely, so i would not add something.
I also couldn't find out how to negate the negation, i.e. -nc:false
or so, is that possible?
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 think -nc:false
just works.
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.
Ah... Here's what tripped me up:
➜ sandbox git:(2.13.x) scala -nc Test.scala
hi
➜ sandbox git:(2.13.x) fsc -shutdown
[No compilation server running.]
➜ sandbox git:(2.13.x) scala Test.scala -nc
hi
➜ sandbox git:(2.13.x) fsc -shutdown
[Compile server exited]
➜ sandbox git:(2.13.x) fsc -shutdown
[No compilation server running.]
➜ sandbox git:(2.13.x)
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 doing a quick refactor of ScriptRunner to break out the functionality, and then -Yscriptrunner:com.github.somsnytt.BestScriptRunnerEver
.
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 remember there's a comment in settings.process about handling trailing options; I'll look if it's broken for Generic...Settings.
@@ -45,9 +46,10 @@ class GenericRunnerSettings(error: String => Unit) extends Settings(error) { | |||
|
|||
val nc = BooleanSetting( | |||
"-nc", | |||
"do not use the fsc compilation daemon") withAbbreviation "-nocompdaemon" withPostSetHook((x: BooleanSetting) => {_useCompDaemon = !x.value }) | |||
"do not use the fsc compilation daemon and shut it down if it is running").withAbbreviation("-nocompdaemon") | |||
//.withDeprecationMessage("scripts use cold compilation by default; use -howtorun:fsc to start the compilation daemon") |
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 in favor of adding the deprecation warning, so we can remove -nc
eventually
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'll need a better story for replacing fsc -shutdown
. Besides -howtorun:shutdown
. Not really sure anyone ever, anywhere, has ever used -howtorun
; I think I tweaked it once. Maybe it should be renamed to -whattorun
.
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.
LGTM, thanks!
@@ -62,9 +62,9 @@ class MainGenericRunner { | |||
case AsObject => | |||
ObjectRunner.runAndCatch(settings.classpathURLs, thingToRun, command.arguments) | |||
case AsScript if isE => | |||
Right(ScriptRunner.runCommand(settings, combinedCode, thingToRun +: command.arguments)) | |||
Right(!ScriptRunner(settings).runScriptText(combinedCode, thingToRun +: command.arguments).isDefined) |
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.
why do we discard the exception here?
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.
You never let me get away with anything. I was worn down by the refactor at that point. And by figuring out what to call it besides runCommand
. I'll take another look at how the values bubble up; there were other spots like CommonRunner
IIRC.
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.
Perhaps along with the rebase?
Let the script runner default to not firing up a compilation daemon. Accept `-howtorun:fsc` to mean use the server. Take `-nc` to mean shut it down. This is much more convenient than using `fsc -shutdown`.
ScriptRunner is an interface, with standard implementations for immediate compilation and resident compile server. A daemon killer knows how to shutdown the compile server.
Accept default, resident, shutdown, or custom class with a constructor that takes settings. `-nc` is a deprecated alias.
We don't seem to care if something ran unsuccessfully without throwing. Also, leverage NonFatal when throwing yourself on a hand grenade.
It could use another round. In ScriptRunner, withCompiledScript and runCompiled return boolean instead of passing back the exception; the handler body is also boolean-valued. Those could be flipped to |
// partest still uses the old name. once we re-in-source partest we | ||
// can get rid of this, but in the meantime, we'd rather not branch | ||
// partest just because one method got renamed | ||
def unwrap(x: Throwable): Throwable = rootCause(x) |
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.
Merit badge territory! 🎖️
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.
💯
case AsJar => | ||
JarRunner.runJar(settings, thingToRun, command.arguments) | ||
case Error => | ||
Right(false) | ||
None |
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.
This changes the exit value of scala Junk
from 1
to 0
. howToRun
sees no such class
.
Are there conditions where the other runners did not throw but would return Some(false)
for failure? Check failed compilation, for example.
Let the script runner default to not firing up a compilation daemon.
Accept
-howtorun:fsc
to mean use the server.Take
-nc
to mean shut it down. This is much more convenient thanusing
fsc -shutdown
.Edit: Use
-Yscriptrunner flavor
, where flavor isdefault
,resident
,shutdown
, or a class name.For
shutdown
to run, usescala -Yscriptrunner shutdown -e ""
. Without-e
or a file, it goes to REPL. An empty filename""
is not sufficient.