insert compiler libraries head of user-specified classpath - fix for 13552

[philwalk]

Reordered scalaArgs ahead of -script <scriptName> arguments when implementing ExecuteMode.Script, as expected by scripting.Main.

Expanded argument file, following the pattern in dotty.tools.dotc.config.CliCommand.

[BarkingBad]

Maybe we could add some unit tests, for example in CoursierTests suite?

[philwalk]

Maybe we could add some unit tests, for example in CoursierTests suite?

I'm working on adding some unit tests. I ran into a puzzle, though, perhaps someone can suggest what might be going on.
From my build environment, after running sbt dist/pack, the following command lines should be equivalent, if not identical:

bin/scala compiler/test-resources/scripting/classpathReport.sc  # this works as expected

dist/target/pack/bin/scala compiler/test-resources/scripting/classpathReport.sc # this works

compiler/test-resources/scripting/classpathReport.sc # this fails catastrophically in Windows

In my environment, calling the script directly prints almost half a million compiler error messages (a bit more than 452,000, to be more precise). Looking at the first few error messages, it seems that the compiler is trying to parse jar files as if they were scala sources:

�[31m-- [E103] Syntax Error: dist\target\pack\lib\antlr-runtime-3.5.1.jar:1:0 -------------------------------------------------------------------------------------------------------------------------------�[0m

�[31m1 |�[0mPK��

  |�[31m^^^^�[0m

  |Illegal start of toplevel definition

longer explanation available when compiling with `-explain`
�[31m-- Error: dist\target\pack\lib\antlr-runtime-3.5.1.jar:2:0 ---------------------------------------------------------------------------------------------------------------------------------------------�[0m

�[31m2 |�[0m

If the Windows jvm doesn't see a semicolon in a wildcard classpath, it treats it as a glob reference and expands it to a list of files. See #10761 and #11633.
My hunch is that somehow the wildcard classpath specified in the hashbang is being passed to the jvm unexpanded, although it's not obvious to me why the 3 command lines have differing results.

My hunch was correct. Here's the jvm invocation, with compiler libraries classpath replaced by $CP for readability:

"c:\opt\jdk/bin/java" -classpath "$CP" dotty.tools.MainGenericRunner -classpath "$CP" -classpath 'dist/target/pack/lib/*' compiler/test-resources/scripting/classpathReport.sc

The reason the problem is hidden in the first 2 command lines above is because the hashbang line is discarded, avoiding the problem.

The fix for this was implemented in #11633, and is mentioned in the scala3-3.0.2 dist/bin/scala script.
Here's the relevant section (lines 78-83):

    -cp*|-classpath*) # partial fix for #10761
      # hashbang can combine args, e.g. "-classpath 'lib/*'"
      CLASS_PATH="${1#* *}${PSEP}"
      let class_path_count+=1
      shift
      ;;

[BarkingBad]

I just tested it on Linux and all 3 commands succeed, however, the last one has many many libraries on classpath, few I recognize from scaladoc

[philwalk]

I just tested it on Linux and all 3 commands succeed, however, the last one has many many libraries on classpath, few I recognize from scaladoc

I'll commit what I have so far, and file a bug report on the bug regression issue.

I added a test to CoursierScalaTests to verify acceptance of an @argumentFile on the hashbang line. The argument file adds . (the current directory) to the classpath. A stronger test would add something not already on the command line and verify that it can be imported, but that will have to wait for later.

[philwalk]

Changes in this commit:

• refactor CliCommand.expandArg method to make it available in MainGenericRunner
• set script.path property in MainGenericRunner before calling dotty.tools.scripting.Main
• remove obsoleted tests in dotty.tools.scripting.ClasspathTests
• update tests in dotty.tools.scripting.BashScriptsTests
• add a new test to CoursierScalaTests to verify @arguments file doesn't cause compiler libraries to be removed from classpath
• append path separator to user classpath in dist/bin/scala to avoid jvm globbing of a single wildcard classpath entry
• fix 2 Windows classpath bugs (partial regressions of broken support for use of wildcards in the compiler classpath on Windows #10761)

The Windows classpath bugs have to be avoided before the jvm is called, since the jvm expands wildcard classpaths before passing control to MainGenericRunner.

[BarkingBad]

Good work, but there are some things to fix before merging.

[BarkingBad]

To fix test:
in line 158
val newClasspath = (settings.classPath :+ ".").flatMap(_.split(classpathSeparator).filter(_.nonEmpty)).map(File(_).toURI.toURL)
in line 180
val newClasspath = (settings.classPath.flatMap(_.split(classpathSeparator).filter(_.nonEmpty)) ++ removeCompiler(scalaClasspath) :+ ".").map(File(_).toURI.toURL)

You can create some private function to handle that. The problem is that we try to map to files strings that actually can be enumerated classpath with classpath separators

[philwalk]

Ouch, I commited bad code (it doesn't compile) ... will commit correct version soon.
Sorry for the noise.

[BarkingBad]

Changes look good, just three little nitpicks

[philwalk]

It looks as though we don't check to see if there is a pre-compiled jar (created via -save option from a previous compile), but we always recompile the script source. The speedup of directly executing a script .jar can easily be several orders of magnitude.

The relevant section of dist/bin/scala in scala3-3.0.2 is lines 173-176:

167 case "${execute_mode-}" in
168 script)
169   if [ "$CLASS_PATH" ]; then
170     script_cp_arg="-classpath '$CLASS_PATH'"
171   fi
172   setScriptName="-Dscript.path=$target_script"
173   target_jar="${target_script%.*}.jar"
174   if [[ $save_compiled == true && "$target_jar" -nt "$target_script" ]]; then
175     eval "\"$JAVACMD\"" $setScriptName -jar "$target_jar" "${script_args[@]}"
176     scala_exit_status=$?
177   else

I have an implementation I will commit after some more testing.
In a Windows environment, It reduces startup time by 3 seconds on a simple script that prints sys.props("sun.java.command").

[philwalk]

I found and fixed 2 problems, and will commit after some more testing.
In BashScriptsTests, there was a platform dependency in a script output validation.
In MainGenericRunner, the settings.scriptArgs, should have been passed to the precompiled jar instead of settings.residualArgs.

For some reason, I can't do the following in my environment, so I have to run the full test suite.

testOnly dotty.tools.coursier.*

[smarter]

For some reason, I can't do the following in my environment, so I have to run the full test suite.

The tests are in their own sbt configuration so you need to run:

scala3-compiler-bootstrapped/Scala3CompilerCoursierTest/test

[BarkingBad]

Looks good

[philwalk]

@BarkingBad
I found a bug related to this PR, and it doesn't yet have test coverage.
Should I create a new PR?

Here's the bug and the one-line fix.

On line 108 of MainGenericRunner:

      val globdir = cp.replaceAll("[\\/][^\\/]*$", "") // slash/backslash agnostic

the regex should have quadruple backslashes, like this:

      val globdir = cp.replaceAll("[\\\\/][^\\\\/]*$", "") // slash/backslash agnostic

The following script fails to compile without the fix:

#!bin/scala -classpath 'dist/target/pack/lib/*'

// import won't compile unless the hashbang line is effective in setting the classpath
import org.jline.terminal.Terminal

def main(args: Array[String]) =
  val cp = sys.props("java.class.path")
  printf("unglobbed classpath:\n%s\n", cp)

I can incorporate this test in dotty.tools.scripting.ClasspathTests

[BarkingBad]

Yes, fresh PR seems reasonable. Please mention me when you create one

[philwalk]

fix and new unit test added by #13619