Skip to content
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

Add a consoleScalacOptions target #2948

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

mrdziuban
Copy link
Contributor

This will allow users to specify the exact set of scalac options that are used in the console

@mrdziuban
Copy link
Contributor Author

mrdziuban commented Jan 6, 2024

The CI failure seems unrelated to these changes but let me know if there's something I can do to resolve it

Never mind, there's no CI failure 😄

@@ -452,7 +457,7 @@ trait ScalaModule extends JavaModule with TestModule.ScalaModuleBase { outer =>
),
jvmArgs = forkArgs(),
envArgs = forkEnv(),
mainArgs = Seq("-usejavacp"),
mainArgs = (Seq("-usejavacp") ++ consoleScalacOptions()).distinct,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the currently hardcoded -usejavacp into the consoleScalacOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure -- is it required for the console to work? If so, then I would say no, as it would let users remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find enough information whether it is always neeed, but at least, there is a -usemanifestcp option in scalac, which looks like an alternative to me. And if someone wants to use it, she needs a way to remove the default.

I also think overriding a target while also re-using the super value is a rather common thing in Mill and would be ok here.

def consoleScalacOptions = super.consoleScalacOptions() ++ scalacOptions().filterNot(o => o == "-Xfatal-warnings")

Lastly, I think you should avoid the distinct, since it may break cli options that accept additional params, which are identical like Seq("-Xmaxerrs", "100", "-Xmaxwarns", "100"). A .distinct will remove the second "100".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on distinct, I've removed that. And your logic about -usejavacp makes sense, I've moved it to consoleScalacOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @lefou, it looks like -usejavacp is required. Without it, I get the error below. So do you think it makes sense to keep it hardcoded in the console command?

Welcome to Scala 2.13.12 (OpenJDK 64-Bit Server VM, Java 17.0.8).
Type in expressions for evaluation. Or try :help.

Failed to initialize compiler: object scala in compiler mirror not found.
** Note that as of 2.8 scala does not assume use of the java classpath.
** For the old behavior pass -usejavacp to scala, or if using a Settings
** object programmatically, settings.usejavacp.value = true.
Exception in thread "main" java.lang.NullPointerException: Cannot throw exception because "null" is null
	at scala.tools.nsc.CompilationUnits$CompilationUnit.<init>(CompilationUnits.scala:43)
	at scala.tools.nsc.CompilationUnits$CompilationUnit.<init>(CompilationUnits.scala:44)
	at scala.tools.nsc.interpreter.IMain$ReadEvalPrint.compile(IMain.scala:743)
	at scala.tools.nsc.interpreter.IMain.bind(IMain.scala:562)
	at scala.tools.nsc.interpreter.IMain.bind(IMain.scala:598)
	at scala.tools.nsc.interpreter.IMain.$anonfun$quietBind$1(IMain.scala:597)
	at scala.tools.nsc.interpreter.shell.ReplReporterImpl.withoutPrintingResults(Reporter.scala:63)
	at scala.tools.nsc.interpreter.IMain.quietBind(IMain.scala:597)
	at scala.tools.nsc.interpreter.shell.ILoop.interpretPreamble(ILoop.scala:940)
	at scala.tools.nsc.interpreter.shell.ILoop.$anonfun$run$3(ILoop.scala:981)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
	at scala.tools.nsc.interpreter.shell.ILoop.echoOff(ILoop.scala:94)
	at scala.tools.nsc.interpreter.shell.ILoop.$anonfun$run$2(ILoop.scala:981)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
	at scala.tools.nsc.interpreter.IMain.withSuppressedSettings(IMain.scala:1428)
	at scala.tools.nsc.interpreter.shell.ILoop.$anonfun$run$1(ILoop.scala:972)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
	at scala.tools.nsc.interpreter.shell.ReplReporterImpl.withoutPrintingResults(Reporter.scala:63)
	at scala.tools.nsc.interpreter.shell.ILoop.run(ILoop.scala:972)
	at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:88)
	at scala.tools.nsc.MainGenericRunner.run$1(MainGenericRunner.scala:92)
	at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:105)
	at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:113)
	at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)
1 targets failed
predef[2].console java.lang.Exception: Interactive Subprocess Failed (exit code 1)
    mill.util.Jvm$.runSubprocess(Jvm.scala:184)
    mill.util.Jvm$.runSubprocessWithBackgroundOutputs(Jvm.scala:152)
    mill.util.Jvm$.runSubprocess(Jvm.scala:88)
    mill.scalalib.ScalaModule.$anonfun$console$2(ScalaModule.scala:444)
    scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
    scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
    scala.Console$.withErr(Console.scala:193)
    mill.api.SystemStreams$.$anonfun$withStreams$2(SystemStreams.scala:62)
    scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
    scala.Console$.withOut(Console.scala:164)
    mill.api.SystemStreams$.$anonfun$withStreams$1(SystemStreams.scala:61)
    scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
    scala.Console$.withIn(Console.scala:227)
    mill.api.SystemStreams$.withStreams(SystemStreams.scala:60)
    mill.scalalib.ScalaModule.$anonfun$console$1(ScalaModule.scala:449)
    mill.define.Task$TraverseCtx.evaluate(Task.scala:71)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's keep it there, then. We can handle it once there is a real need for it. Thanks for checking!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done!

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the analog repl command accepts it's options via command arguments. Should we follow the same in console?

And if, do we still need a consoleScalacOptions target?

@mrdziuban
Copy link
Contributor Author

I think having console take command arguments may lead to a worse user experience, at least for the use case I'm trying to solve for. To achieve what I described in the docs -- using all the same scalacOptions except for -Xfatal-warnings -- a user would have to either:

  1. Pass all the scalacOptions on the command line
  2. Override the console command to call super.console with the correct default set of options

Maybe a hybrid of the two is worth exploring for both repl and console? i.e. something like this:

// `consoleScalacOptions` are ones that are always passed to the console
def consoleScalacOptions: T[Seq[String]] = T(Seq.empty[String])

// `consoleOptions` are ad-hoc options the user can pass on the command line
def console(consoleOptions: String*): Command[Unit] = T.command {
  // ...
  mainArgs = (Seq("-usejavacp") ++ consoleScalacOptions() ++ consoleOptions).distinct
  // ...
}

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you!

@lefou lefou merged commit 3169663 into com-lihaoyi:main Jan 24, 2024
38 checks passed
@lefou lefou added this to the 0.11.7 milestone Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants