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

fix 13618 - undo windows wildcard classpath globbing #13619

Merged
merged 4 commits into from Sep 30, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 35 additions & 15 deletions compiler/src/dotty/tools/MainGenericRunner.scala
Expand Up @@ -105,17 +105,20 @@ object MainGenericRunner {
case "-run" :: fqName :: tail =>
process(tail, settings.withExecuteMode(ExecuteMode.Run).withTargetToRun(fqName))
case ("-cp" | "-classpath" | "--class-path") :: cp :: tail =>
val globdir = cp.replaceAll("[\\/][^\\/]*$", "") // slash/backslash agnostic
val (tailargs, cpstr) = if globdir.nonEmpty && classpathSeparator != ";" || cp.contains(classpathSeparator) then
(tail, cp)
val cpEntries = cp.split(classpathSeparator).toList
val singleEntryClasspath: Boolean = cpEntries.nonEmpty && cpEntries.drop(1).isEmpty
Copy link
Member

Choose a reason for hiding this comment

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

Isn't just cpEntries.size == 1 enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be okay, although a wildcard classpath directory could have thousands of jar files, so size could be expensive.
Should I change it?

Copy link
Member

Choose a reason for hiding this comment

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

We can leave it done your way if you feel that it is better.

val globdir: String = if singleEntryClasspath then cp.replaceAll("[\\\\/][^\\\\/]*$", "") else "" // slash/backslash agnostic
def validGlobbedJar(s: String): Boolean = s.startsWith(globdir) && ((s.toLowerCase.endsWith(".jar") || s.toLowerCase.endsWith(".zip")))
val (tailargs, newEntries) = if singleEntryClasspath && validGlobbedJar(cpEntries.head) then
// reassemble globbed wildcard classpath
// globdir is wildcard directory for globbed jar files, reconstruct the intended classpath
val cpJars = tail.takeWhile( f => validGlobbedJar(f) )
val remainingArgs = tail.drop(cpJars.size)
(remainingArgs, cpEntries ++ cpJars)
else
// combine globbed classpath entries into a classpath
val jarfiles = cp :: tail
val cpfiles = jarfiles.takeWhile( f => f.startsWith(globdir) && ((f.toLowerCase.endsWith(".jar") || f.endsWith(".zip"))) )
val tailargs = jarfiles.drop(cpfiles.size)
(tailargs, cpfiles.mkString(classpathSeparator))

process(tailargs, settings.copy(classPath = settings.classPath ++ cpstr.split(classpathSeparator).filter(_.nonEmpty)))
(tail, cpEntries)

process(tailargs, settings.copy(classPath = settings.classPath ++ newEntries.filter(_.nonEmpty)))

case ("-version" | "--version") :: _ =>
settings.copy(
Expand Down Expand Up @@ -204,16 +207,15 @@ object MainGenericRunner {
val targetScript = Paths.get(settings.targetScript).toFile
val targetJar = settings.targetScript.replaceAll("[.][^\\/]*$", "")+".jar"
val precompiledJar = Paths.get(targetJar).toFile
def mainClass = Jar(targetJar).mainClass.getOrElse("") // throws exception if file not found
val jarIsValid = precompiledJar.isFile && mainClass.nonEmpty && precompiledJar.lastModified >= targetScript.lastModified
val mainClass = if !precompiledJar.isFile then "" else Jar(targetJar).mainClass.getOrElse("")
val jarIsValid = mainClass.nonEmpty && precompiledJar.lastModified >= targetScript.lastModified
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)
val mc = mainClass
if mc.nonEmpty then
ObjectRunner.runAndCatch(newClasspath :+ File(targetJar).toURI.toURL, mc, settings.scriptArgs)
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"))
else
Expand Down Expand Up @@ -241,4 +243,22 @@ object MainGenericRunner {
e.foreach(_.printStackTrace())
!isFailure
}

def display(settings: Settings)= Seq(
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it dead code? I guess it's for debugging purposes, maybe we could add some flag for displaying this debug, or let's remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about an environment variable, maybe something like this:

   lazy val debug = Option(System.getenv("RUNNER_DEBUG")) != None
   [...]
   if debug then printf("settings: %s\n", display(settings))

I also don't mind removing it, I can always add it back temporarily, if needed.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove it.

s"verbose: ${settings.verbose}",
s"classPath: ${settings.classPath.mkString("\n ","\n ","")}",
s"executeMode: ${settings.executeMode}",
s"exitCode: ${settings.exitCode}",
s"javaArgs: ${settings.javaArgs}",
s"scalaArgs: ${settings.scalaArgs}",
s"residualArgs: ${settings.residualArgs}",
s"possibleEntryPaths: ${settings.possibleEntryPaths}",
s"scriptArgs: ${settings.scriptArgs}",
s"targetScript: ${settings.targetScript}",
s"targetToRun: ${settings.targetToRun}",
s"save: ${settings.save}",
s"modeShouldBePossibleRun: ${settings.modeShouldBePossibleRun}",
s"modeShouldBeRun: ${settings.modeShouldBeRun}",
s"compiler: ${settings.compiler}",
)
}
8 changes: 8 additions & 0 deletions compiler/test-resources/scripting/unglobClasspath.sc
@@ -0,0 +1,8 @@
#!bin/scala -classpath 'dist/target/pack/lib/*'

// won't compile unless the hashbang line sets classpath
import org.jline.terminal.Terminal

def main(args: Array[String]) =
val cp = sys.props("java.class.path")
printf("unglobbed classpath: %s\n", cp)
10 changes: 6 additions & 4 deletions compiler/test/dotty/tools/scripting/BashScriptsTests.scala
Expand Up @@ -97,11 +97,13 @@ class BashScriptsTests:
printf("===> verify SCALA_OPTS='@argsfile' is properly handled by `dist/bin/scala`\n")
val envPairs = List(("SCALA_OPTS", s"@$argsfile"))
val (validTest, exitCode, stdout, stderr) = bashCommand(scriptFile.absPath, envPairs)
printf("stdout: %s\n", stdout.mkString("\n","\n",""))
if validTest then
val expected = s"${workingDirectory.toString}"
val List(line1: String, line2: String) = stdout.take(2)
printf("line1 [%s]\n", line1)
val valid = line2.dropWhile( _ != ' ').trim.startsWith(expected)
val expected = s"${workingDirectory.norm}"
val output = stdout.find( _.trim.startsWith("cwd") ).getOrElse("").dropWhile(_!=' ').trim
printf("output [%s]\n", output)
printf("expected[%s]\n", expected)
val valid = output.startsWith(expected)
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")

Expand Down
38 changes: 32 additions & 6 deletions compiler/test/dotty/tools/scripting/ClasspathTests.scala
Expand Up @@ -18,19 +18,19 @@ class ClasspathTests:
val packBinDir = "dist/target/pack/bin"
val packLibDir = "dist/target/pack/lib"

// only interested in classpath test scripts
val testScriptName = "classpathReport.sc"
val testScript = scripts("/scripting").find { _.getName.matches(testScriptName) } match
case None => sys.error(s"test script not found: ${testScriptName}")
case Some(file) => file

def exists(scriptPath: Path): Boolean = Files.exists(scriptPath)
def packBinScalaExists:Boolean = exists(Paths.get(s"$packBinDir/scala"))

/*
* verify classpath reported by called script.
*/
@Test def hashbangClasspathVerifyTest = {
// only interested in classpath test scripts
val testScriptName = "classpathReport.sc"
val testScript = scripts("/scripting").find { _.getName.matches(testScriptName) } match
case None => sys.error(s"test script not found: ${testScriptName}")
case Some(file) => file

val relpath = testScript.toPath.relpath.norm
printf("===> hashbangClasspathVerifyTest for script [%s]\n", relpath)
printf("bash is [%s]\n", bashExe)
Expand All @@ -57,6 +57,32 @@ class ClasspathTests:

assert(hashbangClasspathJars.size == packlibJars.size)
}
/*
* verify classpath is unglobbed by MainGenericRunner.
*/
@Test def unglobClasspathVerifyTest = {
val testScriptName = "unglobClasspath.sc"
val testScript = scripts("/scripting").find { _.getName.matches(testScriptName) } match
case None => sys.error(s"test script not found: ${testScriptName}")
case Some(file) => file

val relpath = testScript.toPath.relpath.norm
printf("===> unglobClasspathVerifyTest for script [%s]\n", relpath)
printf("bash is [%s]\n", bashExe)

if packBinScalaExists then
val bashCmdline = s"SCALA_OPTS= $relpath"
val cmd = Array(bashExe, "-c", bashCmdline)

cmd.foreach { printf("[%s]\n", _) }

// test script reports the classpath it sees
val scriptOutput = exec(cmd:_*)
val scriptCp = findTaggedLine("unglobbed classpath", scriptOutput)
val classpathJars = scriptCp.split(psep).map { _.getName }.sorted.distinct
//classpathJars.foreach { printf("%s\n", _) }
assert(classpathJars.size > 1)
}


//////////////// end of tests ////////////////
Expand Down
2 changes: 1 addition & 1 deletion compiler/test/dotty/tools/scripting/ScriptingTests.scala
Expand Up @@ -19,7 +19,7 @@ class ScriptingTests:
f.getAbsolutePath.replace('\\', '/')

// classpath tests managed by scripting.ClasspathTests.scala
def testFiles = scripts("/scripting").filter { ! _.getName.startsWith("classpath") }
def testFiles = scripts("/scripting").filter { ! _.getName.toLowerCase.contains("classpath") }

def script2jar(scriptFile: File) =
val jarName = s"${scriptFile.getName.dropExtension}.jar"
Expand Down