fix for #10761 ; expand wildcard classpath entries #11633

philwalk · 2021-03-06T22:41:56Z

This fixes #10761

Wildcard classpath entries are expanded, which solves 2 problems:

The -save option requires expanded classpath wildcard entries in jar manifest
The runtime classpath requires expanding -classpath wildcard entries

The solution is not os-dependent, the jar manifest Class-Path property requires expanded wildcards.

The callback classpath:String parameter in compiler/src/dotty/tools/scripting/ScriptingDriver.scala was changed to classpathEntries:Seq[String] so the expanded classpath could also be seen by compiler/src/dotty/tools/scripting/Main.scala and could to be written to the jar manifest.

Two similar use-cases fixed by the PR each required a different fix:

A classpath consisting of a single wildcard entry, specified on the command line (fixed by appending $PSEP to the classpath):

$ scala -classpath 'lib/*' script.sc

A classpath consisting of a single wildcard entry, specified in a hashbang (fixed by accepting '-classpath' and 'lib/*' passed together as a single argument to dist/bin/scala). An example of this use-case:

#!/opt/scala3/bin/scala -classpath 'lib/*'
def main(args: Array[String]): Unit =
  val psep = sys.props("path.separator")
  for entry <- sys.props("java.class.path").split(psep) do
    printf("%s\n",entry)

…manifest; fixes #10761

michelou · 2021-03-08T06:51:53Z

I'm wondering if the Scripting execution engine is the only place in the Scala 3 compiler that has to take care of wildcard classpaths. Any thoughts about it ?!

philwalk · 2021-03-08T21:15:56Z

I'm not aware of where else in the Scala 3 compiler there might also be a need to expand classpath. I wasn't able to produce problems except those that this PR addresses.

smarter · 2021-03-08T21:19:11Z

I'm not aware of where else in the Scala 3 compiler there might also be a need to expand classpath

My comment in #10761 (comment) points you to the code in the compiler which currently handles expanding wildcard, reusing that code would be good.

philwalk · 2021-03-09T19:11:27Z

@smarter - thanks, I updated the PR.

compiler/src/dotty/tools/scripting/ScriptingDriver.scala now uses the expandPath() method in compiler/src/dotty/tools/io/ClassPath.scala.

expandPath() was extended to include classpath entries with forward slash, regardless of OS.

Test class added to verify:

multiple wildcard entries in the classpath
bogus classpath entries don't cause a crash

michelou · 2021-03-09T19:42:59Z

@philwalk I'm uncomfortable with your addition on line 135 in file io/ClassPath.scala. Can you explain how testing both wildSuffix (equals File.separator + "*") and "/*" does make sense in the context of this PR ?

philwalk · 2021-03-09T19:56:18Z

@michelou
The existing wildSuffix test assumes that Windows classpaths use only backslash, but the jdk supports forward slash as well.
The classpath reported in #10761 -classpath 'c:/opt/uejlib2.13/*', has forward slash.

In Windows, the additional test can prevent the wildcard expansion from failing.

An alternative might be something like the following:

  /** Expand single path entry */
  private def expandS(pattern: String): List[String] = {
    val wildSuffix = "/*"

    /* Get all subdirectories, jars, zips out of a directory. */
    def lsDir(dir: Directory, filt: String => Boolean = _ => true) =
      dir.list.filter(x => filt(x.name) && (x.isDirectory || isJarOrZip(x))).map(_.path).toList

    if (pattern == "*") lsDir(Directory("."))
    else if (pattern.replace('\\','/').endsWith(wildSuffix)) lsDir(Directory(pattern dropRight 2))
    else if (pattern.contains('*')) {
      try {
        val regexp = ("^" + pattern.replace("""\*""", """.*""") + "$").r
        lsDir(Directory(pattern).parent, regexp.findFirstIn(_).isDefined)
      }
      catch { case _: PatternSyntaxException => List(pattern) }
    }
    else List(pattern)
  }

michelou · 2021-03-09T21:13:02Z

@michelou
The existing wildSuffix test assumes that Windows classpaths use only backslash, but the jdk supports forward slash as well.
The classpath reported in #10761 -classpath 'c:/opt/uejlib2.13/*', has forward slash.

In Windows, the additional test can prevent the wildcard expansion from failing.

An alternative might be something like the following:

  /** Expand single path entry */
  private def expandS(pattern: String): List[String] = {
    val wildSuffix = "/*"

    /* Get all subdirectories, jars, zips out of a directory. */
    def lsDir(dir: Directory, filt: String => Boolean = _ => true) =
      dir.list.filter(x => filt(x.name) && (x.isDirectory || isJarOrZip(x))).map(_.path).toList

    if (pattern == "*") lsDir(Directory("."))
    else if (pattern.replace('\\','/').endsWith(wildSuffix)) lsDir(Directory(pattern dropRight 2))
    else if (pattern.contains('*')) {
      try {
        val regexp = ("^" + pattern.replace("""\*""", """.*""") + "$").r
        lsDir(Directory(pattern).parent, regexp.findFirstIn(_).isDefined)
      }
      catch { case _: PatternSyntaxException => List(pattern) }
    }
    else List(pattern)
  }

So I would add the following comment befote the modified line :

// On Windows the JDK supports forward slash as well.

philwalk · 2021-03-09T21:25:19Z

@michelou - comment was added to line 135 of compiler/src/dotty/tools/io/ClassPath.scala as per your suggestion

philwalk · 2021-03-11T13:17:46Z

After the change to leverage ClassPath#expandPath, a stack dump similar to the original error reported in #10761 has recurred in the -classpath lib/* use case.

I'm investigating,

philwalk · 2021-03-11T14:11:23Z

Added code to expand wildcards in compiler/src/dotty/tools/dotc/core/MacroClassLoader.scala.

I will improve the coverage in the classpath test compiler/test/dotty/tools/io/ClasspathTest.scala

philwalk · 2021-03-11T14:23:31Z

It turns out that Windows jdk 11 (and maybe earlier versions too?) expand all command line arguments that look like wildcard classpath entries.

As a result, -classpath args on the command line are expanded (Windows only), even if escaped and quoted, etc..

If a classpath is specified in the hashbang header, it doesn't get expanded. The most recent commit covers this use case.

michelou · 2021-03-11T15:22:28Z

@philwalk In file ClasspathTest.scala duplicate import on line 22.

philwalk · 2021-03-11T15:44:23Z

@michelou

@philwalk In file ClasspathTest.scala duplicate import on line 22.

I deleted the extra import, and will commit after I add some more tests (hopefully soon).

…hTest.sc

philwalk · 2021-03-16T14:41:08Z

While testing this PR, I discovered an unrelated problem with dist/bin/scalac
Here's the simple fix:

         -repl) PROG_NAME="$ReplMain" && shift ;;
-      -script) PROG_NAME="$ScriptingMain" && target_script="$2" && in_scripting_args=true && shift && shift ;;
+      -script) PROG_NAME="$ScriptingMain" && target_script="$2" && in_scripting_args=true && shift && shift
+               while [[ $# -gt 0 ]]; do addScripting "$1" && shift ; done ;;
      -compile) PROG_NAME="$CompilerMain" && shift ;;

The bug manifests itself if the target_script also has -script or -repl arguments, etc.

What's the recommended process for submitting a fix?
Should I file a bug report first?
Should I add the fix to this PR, or create a new PR?

smarter · 2021-03-16T14:55:54Z

Since this PR is complicated enough as it is, I suggest making another PR for that unrelated change.

liufengyun · 2021-03-17T16:51:19Z

@michelou Is this one ready to go?

philwalk · 2021-03-17T20:13:48Z

@michelou I'm working on tests to thoroughly verify this PR, but they aren't ready yet.
I have done lots of manual testing, so I believe it's safe to merge.

philwalk · 2021-03-17T21:02:38Z

added tests to verify windows wildcard classpath in [dist/bin/scala
added ability for tests to verify [dist/bin/scalac] generated java command line

philwalk · 2021-03-17T23:09:28Z

@michelou -

@philwalk Your addition on lines 122-126 in file bin/scalac is temporary right ?!

It can be temporary if there are objections to it. We possibly can generate an edited copy of scalac and call that at test time.

The reason I added it was to be able to test scala, scalac and scaladoc scripts. I originally tried to diagnose the same thing from inside of a running script, but there are challenges and I haven't got that approach to work yet.

In the test file ClasspathTest.scala on line 38:

      val cmd = Array(bashExe,"-c",s"SCALAC_ECHO_TEST=1 dist/target/pack/bin/scala -classpath 'lib/* $relpath")

To see what happens, you can remove the SCALAC_ECHO_TEST from the test:

      val cmd = Array(bashExe,"-c",s"dist/target/pack/bin/scala -classpath 'lib/*' $relpath")

~~It fails in Windows with a complaint about mismatched class file versions.~~ (unable to duplicate)

philwalk · 2021-03-18T02:51:18Z

When I run sbt tests from a Windows command shell, the new classpath test fails, so I'm working on how to handle it.

…cript

michelou · 2021-03-18T07:59:23Z

@philwalk Note about style : please add a space after commas, e.g.
val cmd = Array(bashExe, "-c", bashCmdline)
instead of
val cmd = Array(bashExe,"-c",bashCmdline)

philwalk · 2021-03-18T07:59:59Z

Classpath tests now cover the two most ~~difficult~~ problematic use cases:
A classpath consisting of a single wildcard entry, specified on the command line:

$ scala -classpath 'lib/*' script.sc

A classpath consisting of a single wildcard entry, specified in a hashbang:

#!/opt/scala3/bin/scala -classpath 'lib/*'
def main(args: Array[String]): Unit =
  val psep = sys.props("path.separator")
  for entry <- sys.props("java.class.path").split(psep) do
    printf("%s\n",entry)

One version of the single wildcard classpath use cases (in Windows) results in all the jar files in the the wildcard directory being expanded and passed as arguments to the compiler. The compiler seems to treat them as if they're source files and produces thousands of error messages.

Unfortunately, I checked in a bug in [dist/bin/scala], I'll push the fix after the current checks complete.
BTW, is there a way to cancel the current checks to avoid wasting resources?

philwalk · 2021-03-18T08:02:00Z

@michelou

@philwalk Note about style : please add a space after commas, e.g.
val cmd = Array(bashExe, "-c", bashCmdline)
instead of
val cmd = Array(bashExe,"-c",bashCmdline)

I'll go through and check for style errors.

…various style errors

philwalk · 2021-03-18T08:11:28Z

I'm ready to push a bug fix and corrected style errors, should I wait until the current checks are completed?
It would be nice if we could cancel them.

philwalk · 2021-03-18T08:54:04Z

The comment on the most recent commit should have been:
'added missing shift in new -classpath* case in dist/bin/scala; fixed various style errors'

michelou · 2021-03-19T11:48:12Z

The comment on the most recent commit should have been:
'added missing shift in new -classpath* case in dist/bin/scala; fixed various style errors'

Strange. Job 0dc35d8 seems to be still running ?!

philwalk · 2021-03-19T15:23:07Z

The comment on the most recent commit should have been:
'added missing shift in new -classpath* case in dist/bin/scala; fixed various style errors'

Strange. Job 0dc35d8 seems to be still running ?!

@michelou It seems to be finished now (4 hours after your comment).

philwalk · 2021-03-26T16:56:26Z

close and re-open to trigger a rebuild ... there does seem to be something odd about the community build status reporting.

philwalk · 2021-03-28T15:21:01Z

@philwalk Your addition on lines 122-126 in file bin/scalac is temporary right ?!

@michelou I'm ready to push an updated version of compiler/test/dotty/tools/scripting/ClasspathTests.scala that eliminates the SCALAC_ECHO_TEST change to dist/bin/scalac, based on testable modified copies of [dist/bin/scala] and [dist/bin/scalac].

Are you aware of any other changes that are needed for this PR?

michelou · 2021-03-28T17:02:04Z

@philwalk Your file dist/bin/scalac differs from current head (e.g. lines 92-93). Please check it again.

philwalk · 2021-03-28T17:16:11Z

@philwalk Your file dist/bin/scalac differs from current head (e.g. lines 92-93). Please check it again.

@michelou - I will push my changes now, the link above is to the original proposal which has the objectionable lines.

…calac

liufengyun

LGTM, thanks @philwalk 🎉

liufengyun · 2021-04-21T08:27:49Z

@michelou I'm merging this now. Feel free to open a new issue if you spot some problems. Thank you.

philwalk added 2 commits March 6, 2021 15:34

expand wildcard classpath entries; not os-dependent, required by jar …

121d746

…manifest; fixes #10761

removed obsolete comments

54a2dc9

philwalk mentioned this pull request Mar 8, 2021

Script startup latency improvement #11660

Closed

leverage dotty.tools.io.ClassPath.expandPath ; allow windows */

f5daaa2

errors populating temp directory with jars not treated as a test failure

5e90176

add comment to modification in io/ClassPath.scala

15f9965

expand wildcards in MacroClassLoader

c58106c

remove redundant import; extend io/ClasspathTest.scala ; add classpat…

bbd0cca

…hTest.sc

philwalk mentioned this pull request Mar 11, 2021

broken support for use of wildcards in the compiler classpath on Windows #10761

Closed

remove wildcard classpath test script

4bfa802

cat mods.txt

8c72495

philwalk added 3 commits March 17, 2021 22:08

updated scripting/ClasspathTests.scala for sbt.bat environment

8efca1d

update java.class.path with wildcard-expanded classpath, for called s…

bd06fad

…cript

tests for script internal and external wildcard classpath use cases

721390d

added missing shift in new -classpath* case in dist/bin/scala; fixed …

d02fc05

…various style errors

code style fixes

0dc35d8

philwalk closed this Mar 26, 2021

philwalk reopened this Mar 26, 2021

replace SCALAC_ECHO_TEST scalac modification; test modified copy of s…

cb64423

…calac

liufengyun approved these changes Apr 21, 2021

View reviewed changes

liufengyun merged commit f7b5594 into scala:master Apr 21, 2021

philwalk deleted the scripting-classpath-wildcard-fix-10761 branch April 21, 2021 15:12

philwalk mentioned this pull request Sep 20, 2021

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

Merged

Kordyjan added this to the 3.0.1 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for #10761 ; expand wildcard classpath entries #11633

fix for #10761 ; expand wildcard classpath entries #11633

philwalk commented Mar 6, 2021 •

edited

Loading

michelou commented Mar 8, 2021

philwalk commented Mar 8, 2021

smarter commented Mar 8, 2021

philwalk commented Mar 9, 2021

michelou commented Mar 9, 2021

philwalk commented Mar 9, 2021 •

edited

Loading

michelou commented Mar 9, 2021

philwalk commented Mar 9, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 11, 2021

michelou commented Mar 11, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 16, 2021 •

edited

Loading

smarter commented Mar 16, 2021

liufengyun commented Mar 17, 2021

philwalk commented Mar 17, 2021

philwalk commented Mar 17, 2021

philwalk commented Mar 17, 2021 •

edited

Loading

philwalk commented Mar 18, 2021

michelou commented Mar 18, 2021

philwalk commented Mar 18, 2021 •

edited

Loading

philwalk commented Mar 18, 2021

philwalk commented Mar 18, 2021

philwalk commented Mar 18, 2021

michelou commented Mar 19, 2021

philwalk commented Mar 19, 2021

philwalk commented Mar 26, 2021

philwalk commented Mar 28, 2021 •

edited

Loading

michelou commented Mar 28, 2021

philwalk commented Mar 28, 2021

liufengyun left a comment

liufengyun commented Apr 21, 2021

fix for #10761 ; expand wildcard classpath entries #11633

fix for #10761 ; expand wildcard classpath entries #11633

Conversation

philwalk commented Mar 6, 2021 • edited Loading

michelou commented Mar 8, 2021

philwalk commented Mar 8, 2021

smarter commented Mar 8, 2021

philwalk commented Mar 9, 2021

michelou commented Mar 9, 2021

philwalk commented Mar 9, 2021 • edited Loading

michelou commented Mar 9, 2021

philwalk commented Mar 9, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 11, 2021

michelou commented Mar 11, 2021

philwalk commented Mar 11, 2021

philwalk commented Mar 16, 2021 • edited Loading

smarter commented Mar 16, 2021

liufengyun commented Mar 17, 2021

philwalk commented Mar 17, 2021

philwalk commented Mar 17, 2021

philwalk commented Mar 17, 2021 • edited Loading

philwalk commented Mar 18, 2021

michelou commented Mar 18, 2021

philwalk commented Mar 18, 2021 • edited Loading

philwalk commented Mar 18, 2021

philwalk commented Mar 18, 2021

philwalk commented Mar 18, 2021

michelou commented Mar 19, 2021

philwalk commented Mar 19, 2021

philwalk commented Mar 26, 2021

philwalk commented Mar 28, 2021 • edited Loading

michelou commented Mar 28, 2021

philwalk commented Mar 28, 2021

liufengyun left a comment

Choose a reason for hiding this comment

liufengyun commented Apr 21, 2021

philwalk commented Mar 6, 2021 •

edited

Loading

philwalk commented Mar 9, 2021 •

edited

Loading

philwalk commented Mar 16, 2021 •

edited

Loading

philwalk commented Mar 17, 2021 •

edited

Loading

philwalk commented Mar 18, 2021 •

edited

Loading

philwalk commented Mar 28, 2021 •

edited

Loading