Skip to content

Fix plus Workaround for 13760 #14241

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

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions compiler/src/dotty/tools/MainGenericRunner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ case class Settings(
def withSave: Settings =
this.copy(save = true)

def noSave: Settings =
this.copy(save = false)

Comment on lines +84 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? There is save = false by default afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need this? There is save = false by default afaik

The purpose is to permit a script to opt-out of the -save option if necessary, without having to disable the -save option for all scripts (by removing -save from SCALA_OPTS). There is a new command line option -nosave that will reset save = false after SCALA_OPTS have been processed. The new option can be added to the hashbang line. Our new BashScriptTests test case verifies that it can override the -save option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds good

def withModeShouldBePossibleRun: Settings =
this.copy(modeShouldBePossibleRun = true)

Expand Down Expand Up @@ -135,6 +138,8 @@ object MainGenericRunner {
)
case "-save" :: tail =>
process(tail, settings.withSave)
case "-nosave" :: tail =>
process(tail, settings.noSave)
case "-with-compiler" :: tail =>
process(tail, settings.withCompiler)
case (o @ javaOption(striped)) :: tail =>
Expand Down Expand Up @@ -207,16 +212,18 @@ object MainGenericRunner {
case ExecuteMode.Script =>
val targetScript = Paths.get(settings.targetScript).toFile
val targetJar = settings.targetScript.replaceAll("[.][^\\/]*$", "")+".jar"
val precompiledJar = Paths.get(targetJar).toFile
val precompiledJar = File(targetJar)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Nio is newer and usually more respected that old io API, so I am curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this needed? Nio is newer and usually more respected that old io API, so I am curious

This is not needed, but it now matches how File instanced str created everywhere else in MainGenericRunner, and it's also more efficient (by directly initializing a File rather than creating a Path and then converting it to a File). However, perhaps we should leave it like it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you feel like File is OK, let it be then, I won't argue

val mainClass = if !precompiledJar.isFile then "" else Jar(targetJar).mainClass.getOrElse("")
val jarIsValid = mainClass.nonEmpty && precompiledJar.lastModified >= targetScript.lastModified
val jarIsValid = mainClass.nonEmpty && precompiledJar.lastModified >= targetScript.lastModified && settings.save
if jarIsValid then
// precompiledJar exists, is newer than targetScript, and manifest defines a mainClass
sys.props("script.path") = targetScript.toPath.toAbsolutePath.normalize.toString
val scalaClasspath = ClasspathFromClassloader(Thread.currentThread().getContextClassLoader).split(classpathSeparator)
val newClasspath = (settings.classPath.flatMap(_.split(classpathSeparator).filter(_.nonEmpty)) ++ removeCompiler(scalaClasspath) :+ ".").map(File(_).toURI.toURL)
if mainClass.nonEmpty then
ObjectRunner.runAndCatch(newClasspath :+ File(targetJar).toURI.toURL, mainClass, settings.scriptArgs)
ObjectRunner.runAndCatch(newClasspath :+ File(targetJar).toURI.toURL, mainClass, settings.scriptArgs).foreach { (t: Throwable) =>
t.printStackTrace()
}
Comment on lines +224 to +226
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it returning Unit instead of Option[Throwable]?

Copy link
Contributor

@BarkingBad BarkingBad Jan 11, 2022

Choose a reason for hiding this comment

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

I guess it was my fault that I don't assign the throwable and then print it if something went wrong. I think it should be done that way, then we will reuse Exception from line 228

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So something like this?

          val res = if mainClass.nonEmpty then
            ObjectRunner.runAndCatch(newClasspath :+ File(targetJar).toURI.toURL, mainClass, settings.scriptArgs)
          else
            Some(IllegalArgumentException(s"No main class defined in manifest in jar: $precompiledJar"))
          errorFn("", res)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, something like that seems reasonable

else
Some(IllegalArgumentException(s"No main class defined in manifest in jar: $precompiledJar"))
else
Expand Down
6 changes: 6 additions & 0 deletions compiler/test-resources/scripting/sqlDateError.sc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!bin/scala -nosave

def main(args: Array[String]): Unit = {
println(new java.sql.Date(100L))
System.err.println("SCALA_OPTS="+Option(System.getenv("SCALA_OPTS")).getOrElse(""))
}
20 changes: 20 additions & 0 deletions compiler/test/dotty/tools/scripting/BashScriptsTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,23 @@ class BashScriptsTests:
if valid then printf(s"\n===> success: classpath begins with %s, as reported by [%s]\n", workingDirectory, scriptFile.getName)
assert(valid, s"script ${scriptFile.absPath} did not report valid java.class.path first entry")

/*
* verify that individual scripts can override -save with -nosave (needed to address #13760).
*/
@Test def sqlDateTest =
val scriptBase = "sqlDateError"
val scriptFile = testFiles.find(_.getName == s"$scriptBase.sc").get
val testJar = testFile(s"$scriptBase.jar") // jar should not be created when scriptFile runs
printf("===> verify '-save' is cancelled by '-nosave' in script hashbang.`\n")
val (validTest, exitCode, stdout, stderr) = bashCommand(s"SCALA_OPTS=-save ${scriptFile.absPath}")
printf("stdout: %s\n", stdout.mkString("\n","\n",""))
if verifyValid(validTest) then
// the script should print '1969-12-31' or '1970-01-01', depending on time zone
// stdout can be polluted with an ANSI color prefix, in some test environments
val valid = stdout.mkString("").matches(""".*\d{4}-\d{2}-\d{2}.*""")
if (!valid) then
stdout.foreach { printf("stdout[%s]\n", _) }
stderr.foreach { printf("stderr[%s]\n", _) }
if valid then printf(s"\n===> success: scripts can override -save via -nosave\n")
assert(valid, s"script ${scriptFile.absPath} reported unexpected value for java.sql.Date ${stdout.mkString("\n")}")
assert(!testJar.exists,s"unexpected, jar file [$testJar] was created")