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

fix 13618 - undo windows wildcard classpath globbing #13619

merged 4 commits into from Sep 30, 2021

Conversation

philwalk
Copy link
Contributor

@philwalk philwalk commented Sep 27, 2021

@BarkingBad
This is the fix referred to in #13618, and discussed at the tail end of #13562.

The error output generated by the compiler by this problem can be simulated before this fix by the following command line (notice that there are no quotes around the wildcard classpath entry):

bin/scala -classpath dist/target/pack/lib/* compiler/test-resources/scripting/unglobClasspath.sc >test.out 2>&1

To verify the fix, a new test is added to ClasspathTests.

@philwalk philwalk changed the title undo windows wildcard classpath globbing - fix for #13618 fix 13618 - undo windows wildcard classpath globbing Sep 27, 2021
@philwalk
Copy link
Contributor Author

I have an improved version of MainGenericRunner, with improved platform portability, better comments and simplified logic. I'll push a commit after running tests.

@BarkingBad
Copy link
Member

BarkingBad commented Sep 28, 2021

Thank you, I will review the changes tomorrow. Will you be pushing some more improvements or it is the final state?

@philwalk
Copy link
Contributor Author

Thank you, I will review the changes tomorrow. Will you be pushing some more improvements or it is the final stat?

That's all I have, it's now ready to merge.

Copy link
Member

@BarkingBad BarkingBad left a comment

Choose a reason for hiding this comment

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

For now, looks good. I'll review it one more time when you fix the tests.

@philwalk
Copy link
Contributor Author

working on it ...

@philwalk
Copy link
Contributor Author

philwalk commented Sep 29, 2021

Tests were successful, although this is what I get when I run coursier tests on my Windows workstation, FWIW:

sbt:scala3> scala3-compiler-bootstrapped/Scala3CompilerCoursierTest/test
[info] Wrote C:\Users\philwalk\workspace\newPR-dotty\library\..\out\bootstrap\scala3-library-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\scala3-library_3-3.1.1-RC1-bin-SNAPSHOT.pom
[info] Wrote C:\Users\philwalk\workspace\newPR-dotty\compiler\..\out\bootstrap\scala3-compiler-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\scala3-compiler_3-3.1.1-RC1-bin-SNAPSHOT.pom
[info] Wrote C:\Users\philwalk\workspace\newPR-dotty\interfaces\target\scala3-interfaces-3.1.1-RC1-bin-SNAPSHOT.pom
[info] Wrote C:\Users\philwalk\workspace\newPR-dotty\tasty\..\out\bootstrap\tasty-core-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\tasty-core_3-3.1.1-RC1-bin-SNAPSHOT.pom
[info] :: delivering :: org.scala-lang#scala3-interfaces;3.1.1-RC1-bin-SNAPSHOT :: 3.1.1-RC1-bin-SNAPSHOT :: integration :: Wed Sep 29 16:15:13 MDT 2021
[info]  delivering ivy file to C:\Users\philwalk\workspace\newPR-dotty\interfaces\target\ivy-3.1.1-RC1-bin-SNAPSHOT.xml
[info]  published scala3-interfaces to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-interfaces\3.1.1-RC1-bin-SNAPSHOT\poms\scala3-interfaces.pom
[info]  published scala3-interfaces to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-interfaces\3.1.1-RC1-bin-SNAPSHOT\jars\scala3-interfaces.jar
[info]  published scala3-interfaces to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-interfaces\3.1.1-RC1-bin-SNAPSHOT\srcs\scala3-interfaces-sources.jar
[info]  published scala3-interfaces to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-interfaces\3.1.1-RC1-bin-SNAPSHOT\docs\scala3-interfaces-javadoc.jar
[info]  published ivy to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-interfaces\3.1.1-RC1-bin-SNAPSHOT\ivys\ivy.xml
[info] :: delivering :: org.scala-lang#tasty-core_3;3.1.1-RC1-bin-SNAPSHOT :: 3.1.1-RC1-bin-SNAPSHOT :: integration :: Wed Sep 29 16:15:15 MDT 2021
[info]  delivering ivy file to C:\Users\philwalk\workspace\newPR-dotty\out\bootstrap\tasty-core-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\ivy-3.1.1-RC1-bin-SNAPSHOT.xml
[info] Main Scala API documentation to C:\Users\philwalk\workspace\newPR-dotty\library\..\out\bootstrap\scala3-library-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\api...
[info]  published tasty-core_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\tasty-core_3\3.1.1-RC1-bin-SNAPSHOT\poms\tasty-core_3.pom
[info]  published tasty-core_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\tasty-core_3\3.1.1-RC1-bin-SNAPSHOT\jars\tasty-core_3.jar
[info]  published tasty-core_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\tasty-core_3\3.1.1-RC1-bin-SNAPSHOT\srcs\tasty-core_3-sources.jar
[info]  published tasty-core_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\tasty-core_3\3.1.1-RC1-bin-SNAPSHOT\docs\tasty-core_3-javadoc.jar
[info]  published ivy to C:\Users\philwalk\.ivy2\local\org.scala-lang\tasty-core_3\3.1.1-RC1-bin-SNAPSHOT\ivys\ivy.xml
[info] Skipping unused scalacOptions: -Werror, -Yno-experimental, -deprecation, -feature, -language
[warn] Setting -encoding is currently not supported.
[warn] Setting -sourcepath is currently not supported.
[warn] -- Warning: C:\Users\philwalk\workspace\newPR-dotty\library\src\scala\compiletime\package.scala:58:11
[warn] 58 |inline def error(inline msg: String): Nothing = ???
[warn]    |^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]    |java.util.NoSuchElementException: None.get
[warn]    |     at scala.None$.get(Option.scala:627)
[warn]    |     at scala.None$.get(Option.scala:626)
[warn]    |     at dotty.tools.dotc.typer.PrepareInlineable$.makeInlineable(PrepareInlineable.scala:35)
[warn]    |     at dotty.tools.dotc.typer.PrepareInlineable$.registerInlineInfo$$anonfun$2$$anonfun$1(PrepareInlineable.scala:267)
[error] -- [E069] Syntax Error: C:\Users\philwalk\workspace\newPR-dotty\library\src\scala\IArray.scala:13:7
[error] 13 |object IArray:
[error]    |       ^
[error]    |object IArray cannot have the same name as object IArray in package scala -- cannot define object member with the same name as a object member in self reference _.
[error]    |(Note: this can be resolved by using another name)
[warn] three warnings found
[error] one error found
[info] :: delivering :: org.scala-lang#scala3-library_3;3.1.1-RC1-bin-SNAPSHOT :: 3.1.1-RC1-bin-SNAPSHOT :: integration :: Wed Sep 29 16:15:24 MDT 2021
[info]  delivering ivy file to C:\Users\philwalk\workspace\newPR-dotty\out\bootstrap\scala3-library-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\ivy-3.1.1-RC1-bin-SNAPSHOT.xml
[info]  published scala3-library_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-library_3\3.1.1-RC1-bin-SNAPSHOT\poms\scala3-library_3.pom
[info]  published scala3-library_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-library_3\3.1.1-RC1-bin-SNAPSHOT\jars\scala3-library_3.jar
[info]  published scala3-library_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-library_3\3.1.1-RC1-bin-SNAPSHOT\srcs\scala3-library_3-sources.jar
[info]  published scala3-library_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-library_3\3.1.1-RC1-bin-SNAPSHOT\docs\scala3-library_3-javadoc.jar
[info]  published ivy to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-library_3\3.1.1-RC1-bin-SNAPSHOT\ivys\ivy.xml
[info] :: delivering :: org.scala-lang#scala3-compiler_3;3.1.1-RC1-bin-SNAPSHOT :: 3.1.1-RC1-bin-SNAPSHOT :: integration :: Wed Sep 29 16:15:30 MDT 2021
[info]  delivering ivy file to C:\Users\philwalk\workspace\newPR-dotty\out\bootstrap\scala3-compiler-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\ivy-3.1.1-RC1-bin-SNAPSHOT.xml
[info]  published scala3-compiler_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-compiler_3\3.1.1-RC1-bin-SNAPSHOT\poms\scala3-compiler_3.pom
[info]  published scala3-compiler_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-compiler_3\3.1.1-RC1-bin-SNAPSHOT\jars\scala3-compiler_3.jar
[info]  published scala3-compiler_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-compiler_3\3.1.1-RC1-bin-SNAPSHOT\srcs\scala3-compiler_3-sources.jar
[info]  published scala3-compiler_3 to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-compiler_3\3.1.1-RC1-bin-SNAPSHOT\docs\scala3-compiler_3-javadoc.jar
[info]  published ivy to C:\Users\philwalk\.ivy2\local\org.scala-lang\scala3-compiler_3\3.1.1-RC1-bin-SNAPSHOT\ivys\ivy.xml
[info] compiling 2 Scala sources to C:\Users\philwalk\workspace\newPR-dotty\out\bootstrap\scala3-compiler-bootstrapped\scala-3.1.1-RC1-bin-SNAPSHOT-nonbootstrapped\test-classes ...
[info] Test run started
[info] Test dotty.tools.coursier.CoursierScalaTests.allTests started
expected: arg  0:[a]
actual  : arg  0:[a]er-bootstrapped / Scala3CompilerCoursierTest / executeTests 3s
expected: arg  1:[b]
actual  : arg  1:[b]
expected: arg  2:[c]
actual  : arg  2:[c]
expected: arg  3:[-repl]
actual  : arg  3:[-repl]
expected: arg  4:[-run]
actual  : arg  4:[-run]
expected: arg  5:[-script]
actual  : arg  5:[-script]
expected: arg  6:[-debug]
actual  : arg  6:[-debug]
actual  : arg  6:[-debug][info] Test run finished: 0 failed, 0 ignored, 1 total, 22.628s
[info] Passed: Total 1, Failed 0, Errors 0, Passed 1
[success] Total time: 43 s, completed Sep 29, 2021, 4:15:56 PM

@BarkingBad
Copy link
Member

Do you mean that there are some errors etc? They all come from publishing artifacts - coursier tests depend on Couriser, which is built on to of some scala jars. I don't see anything particularly wrong with the MainGenericRunner itself, the warnings are not related.

Copy link
Member

@BarkingBad BarkingBad left a comment

Choose a reason for hiding this comment

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

Just 2 notes

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.

@@ -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.

@@ -106,7 +106,7 @@ object MainGenericRunner {
process(tail, settings.withExecuteMode(ExecuteMode.Run).withTargetToRun(fqName))
case ("-cp" | "-classpath" | "--class-path") :: cp :: tail =>
val cpEntries = cp.split(classpathSeparator).toList
val singleEntryClasspath: Boolean = cpEntries.nonEmpty && cpEntries.drop(1).isEmpty
val singleEntryClasspath: Boolean = cpEntries.take(2).size == 1
Copy link
Member

Choose a reason for hiding this comment

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

Clever

@BarkingBad BarkingBad merged commit 0e3507e into scala:master Sep 30, 2021
@michelou
Copy link
Collaborator

@philwalk 🎊

@philwalk philwalk deleted the unglob-windows-wildcard-classpath branch September 30, 2021 16:22
@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
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.

None yet

4 participants