Fix window path #5560
Fix window path #5560
Conversation
| @@ -174,7 +174,7 @@ final class JrtClassPath(fs: java.nio.file.FileSystem) extends ClassPath with No | |||
| if (inPackage == "") Nil | |||
| else { | |||
| packageToModuleBases.getOrElse(inPackage, Nil).flatMap(x => | |||
| Files.list(x.resolve(inPackage.replace('.', '/'))).iterator().asScala.filter(_.getFileName.toString.endsWith(".class"))).map(x => | |||
| Files.list(x.resolve(FileUtils.dirPath(inPackage))).iterator().asScala.filter(_.getFileName.toString.endsWith(".class"))).map(x => | |||
There was a problem hiding this comment.
Are you sure this is actually broken ? This code comes straight from scalac (https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/classpath/DirectoryClassPath.scala#L207) which I assume has received adequate testing under Windows /cc @retronym
There was a problem hiding this comment.
I'm not sure if it causes a problem, just get to this by a search. I'm wondering if there is any reason to use / instead of File.separator.
|
Please don't merge, until we test this on windows. |
In QuoteDriver, previous we have path like the following on windows: /C:/Users/dotty/My%20Programs/Java/Test/bin/myJar.jar Credit: https://stackoverflow.com/questions/6164448/convert-url-to-normal-windows-filename-java
|
@allanrenucci @nicolasstucki Could you please have a look at the last commit? |
| @@ -96,7 +96,8 @@ object QuoteDriver { | |||
| case cl: URLClassLoader => | |||
| // Loads the classes loaded by this class loader | |||
| // When executing `run` or `test` in sbt the classpath is not in the property java.class.path | |||
| val newClasspath = cl.getURLs.map(_.getFile()) | |||
| import java.nio.file.Paths | |||
| val newClasspath = cl.getURLs.map(url => Paths.get(url.toURI).toFile) | |||
There was a problem hiding this comment.
Why do we need a conversion to File here? It seems we are only interested in the String representation of these paths. I would make the conversion to String explicit. E.g.
val newClasspath = cl.getURLs.map(url => Paths.get(url.toURI).toString)There was a problem hiding this comment.
Indeed, toString is better and I just tested it works on windows.
There was a problem hiding this comment.
I like Allan's suggestion and I will test it ASAP:
val newClasspath = cl.getURLs.map(url => Paths.get(url.toURI).toString)For comparison here is my local change (pending PR for several weeks now):
val newClasspath = cl.getURLs.map(url => new JFile(url.getFile).getAbsolutePath)There was a problem hiding this comment.
By the way the same issue exists in source file compiler/test/dotty/tools/vulpix/RunnerOrchestration.scala.
My local change in function createProcess is:
val url = classOf[ChildJVMMain].getProtectionDomain.getCodeSource.getLocation
val cp = Paths.get(url.toURI).toString + JFile.pathSeparator + Properties.scalaLibraryinstead of
val cp =
classOf[ChildJVMMain].getProtectionDomain.getCodeSource.getLocation.getFile + JFile.pathSeparator +
Properties.scalaLibraryThere was a problem hiding this comment.
Thanks @michelou . It seems there are a lot of changes needed for the tests. I propose to restrict this PR for the compiler fixes, and make a separate PR for the test fixes.
| val assetLen = pageLocation.getAbsolutePath.split(sep).length | ||
| "../" * (assetLen - rootLen - 1 + additionalDepth) + "." | ||
| val rootLen = root.toPath.normalize.getNameCount | ||
| val assetLen = pageLocation.toPath.normalize.getNameCount |
There was a problem hiding this comment.
Don't you need to convert to absolute path as well?
There was a problem hiding this comment.
Usage of absolute path info in URL seems problematic. For now, I added toAbstractPath to keep the logic as before.
fix windows path (see #5560)
Fix window path