Skip to content

Fix window path #5560

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Dec 10, 2018
Merged

Fix window path #5560

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1c721d8
Fix window path
liufengyun Dec 3, 2018
a5e60dd
Fix window path in ClassPath
liufengyun Dec 3, 2018
769233a
More fix for window path
liufengyun Dec 3, 2018
7727d51
Refactor without using JFile.separator (thanks @michelou)
liufengyun Dec 4, 2018
0ff65a5
Address review
liufengyun Dec 5, 2018
0a7ffbc
More path fixes
liufengyun Dec 6, 2018
0c3e55b
Path fix for doc-tool and QuoteDriver
liufengyun Dec 9, 2018
4b3340f
Address review
liufengyun Dec 10, 2018
23764fd
Keep the original logic: count names in absolute path
liufengyun Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions bench/src/main/scala/Benchmarks.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ object Bench {
val iterations = if (intArgs.length > 1) intArgs(1).toInt else 20
val forks = if (intArgs.length > 2) intArgs(2).toInt else 1


import File.{ separator => sep }

val args2 = args1.map { arg =>
if ((arg.endsWith(".scala") || arg.endsWith(".java")) && arg.head != '/') "../" + arg
if ((arg.endsWith(".scala") || arg.endsWith(".java")) && !(new File(arg)).isAbsolute) ".." + sep + arg
else arg
}
storeCompileOptions(args2)
Expand Down Expand Up @@ -61,9 +64,10 @@ object Bench {
val libs = if (args.contains("-with-compiler")) compiler_libs else standard_libs
var argsNorm = args.filter(_ != "-with-compiler")

import File.{ pathSeparator => sep }
var cpIndex = argsNorm.indexOf("-classpath")
if (cpIndex == -1) cpIndex = argsNorm.indexOf("-cp")
if (cpIndex != -1) argsNorm(cpIndex + 1) = argsNorm(cpIndex + 1) + java.io.File.pathSeparator + libs
if (cpIndex != -1) argsNorm(cpIndex + 1) = argsNorm(cpIndex + 1) + sep + libs
else argsNorm = argsNorm :+ "-classpath" :+ libs

val file = new File(COMPILE_OPTS_FILE)
Expand Down
8 changes: 4 additions & 4 deletions compiler/src/dotty/tools/dotc/classpath/DirectoryClassPath.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

ClassFileEntryImpl(new PlainFile(new dotty.tools.io.File(x)))).toVector
}
}
Expand All @@ -193,7 +193,7 @@ final class JrtClassPath(fs: java.nio.file.FileSystem) extends ClassPath with No
else {
val inPackage = packageOf(className)
packageToModuleBases.getOrElse(inPackage, Nil).iterator.flatMap{x =>
val file = x.resolve(className.replace('.', '/') + ".class")
val file = x.resolve(FileUtils.dirPath(className) + ".class")
if (Files.exists(file)) new PlainFile(new dotty.tools.io.File(file)) :: Nil else Nil
}.take(1).toList.headOption
}
Expand All @@ -207,7 +207,7 @@ case class DirectoryClassPath(dir: JFile) extends JFileDirectoryLookup[ClassFile

def findClassFile(className: String): Option[AbstractFile] = {
val relativePath = FileUtils.dirPath(className)
val classFile = new JFile(s"$dir/$relativePath.class")
val classFile = new JFile(dir, relativePath + ".class")
if (classFile.exists) {
val wrappedClassFile = new dotty.tools.io.File(classFile.toPath)
val abstractClassFile = new PlainFile(wrappedClassFile)
Expand All @@ -232,7 +232,7 @@ case class DirectorySourcePath(dir: JFile) extends JFileDirectoryLookup[SourceFi
private def findSourceFile(className: String): Option[AbstractFile] = {
val relativePath = FileUtils.dirPath(className)
val sourceFile = Stream("scala", "java")
.map(ext => new JFile(s"$dir/$relativePath.$ext"))
.map(ext => new JFile(dir, relativePath + "." + ext))
.collectFirst { case file if file.exists() => file }

sourceFile.map { file =>
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/classpath/FileUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ object FileUtils {
else throw new FatalError("Unexpected source file ending: " + fileName)
}

def dirPath(forPackage: String): String = forPackage.replace('.', '/')
def dirPath(forPackage: String): String = forPackage.replace('.', JFile.separatorChar)

def endsClass(fileName: String): Boolean =
fileName.length > 6 && fileName.substring(fileName.length - 6) == ".class"
Expand Down
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/classpath/VirtualDirectoryClassPath.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ case class VirtualDirectoryClassPath(dir: VirtualDirectory) extends ClassPath wi

protected def emptyFiles: Array[AbstractFile] = Array.empty
protected def getSubDir(packageDirName: String): Option[AbstractFile] =
Option(lookupPath(dir)(packageDirName.split('/'), directory = true))
Option(lookupPath(dir)(packageDirName.split(java.io.File.separator), directory = true))
protected def listChildren(dir: AbstractFile, filter: Option[AbstractFile => Boolean] = None): Array[F] = filter match {
case Some(f) => dir.iterator.filter(f).toArray
case _ => dir.toArray
Expand All @@ -41,7 +41,7 @@ case class VirtualDirectoryClassPath(dir: VirtualDirectory) extends ClassPath wi

def findClassFile(className: String): Option[AbstractFile] = {
val relativePath = FileUtils.dirPath(className) + ".class"
Option(lookupPath(dir)(relativePath split '/', directory = false))
Option(lookupPath(dir)(relativePath.split(java.io.File.separator), directory = false))
}

private[dotty] def classes(inPackage: String): Seq[ClassFileEntry] = files(inPackage)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/classpath/ZipArchiveFileLookup.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ trait ZipArchiveFileLookup[FileEntryType <: ClassRepresentation] extends ClassPa
}

private def findDirEntry(pkg: String): Option[archive.DirEntry] = {
val dirName = s"${FileUtils.dirPath(pkg)}/"
val dirName = pkg.replace('.', '/') + "/"
archive.allDirs.get(dirName)
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/consumetasty/ConsumeTasty.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ object ConsumeTasty {
}

val currentClasspath = QuoteDriver.currentClasspath
val args = "-from-tasty" +: "-classpath" +: s"$classpath:$currentClasspath" +: classes
import java.io.File.{ pathSeparator => sep }
val args = "-from-tasty" +: "-classpath" +: s"$classpath$sep$currentClasspath" +: classes
(new Consume).process(args.toArray)
}
}
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/quoted/QuoteDriver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, toString is better and I just tested it works on windows.

Copy link
Contributor

@michelou michelou Dec 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@michelou michelou Dec 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

instead of

  val cp =
    classOf[ChildJVMMain].getProtectionDomain.getCodeSource.getLocation.getFile + JFile.pathSeparator +
        Properties.scalaLibrary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

newClasspath.mkString("", java.io.File.pathSeparator, if (classpath0 == "") "" else java.io.File.pathSeparator + classpath0)
case _ => classpath0
}
Expand Down
19 changes: 10 additions & 9 deletions doc-tool/src/dotty/tools/dottydoc/staticsite/Site.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import java.io.{ File => JFile, OutputStreamWriter, BufferedWriter, ByteArrayInp
import java.util.{ List => JList, Arrays }
import java.nio.file.Path
import java.nio.charset.StandardCharsets
import java.io.File.{ separator => sep }

import com.vladsch.flexmark.parser.ParserEmulationProfile
import com.vladsch.flexmark.parser.Parser
Expand Down Expand Up @@ -166,9 +167,9 @@ case class Site(
private def defaultParams(pageLocation: JFile, additionalDepth: Int = 0): DefaultParams = {
val pathFromRoot = stripRoot(pageLocation)
val baseUrl: String = {
val rootLen = root.getAbsolutePath.split('/').length
val assetLen = pageLocation.getAbsolutePath.split('/').length
"../" * (assetLen - rootLen - 1 + additionalDepth) + "."
val rootLen = root.toPath.normalize.getNameCount
val assetLen = pageLocation.toPath.normalize.getNameCount
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to convert to absolute path as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usage of absolute path info in URL seems problematic. For now, I added toAbstractPath to keep the logic as before.

"../" * (assetLen - rootLen + additionalDepth) + "."
}

DefaultParams(
Expand Down Expand Up @@ -197,16 +198,16 @@ case class Site(
// Suffix is index.html for packages and therefore the additional depth
// is increased by 1
val (suffix, offset) =
if (e.kind == "package") ("/index.html", -1)
if (e.kind == "package") (sep + "index.html", -1)
else (".html", 0)

val path = if (scala.util.Properties.isWin)
e.path.map(_.replace("<", "_").replace(">", "_"))
else
else
e.path
val target = mkdirs(fs.getPath(outDir.getAbsolutePath + "/api/" + path.mkString("/") + suffix))
val target = mkdirs(fs.getPath(outDir.getAbsolutePath + sep + "api" + sep + path.mkString(sep) + suffix))
val params = defaultParams(target.toFile, -1).withPosts(blogInfo).withEntity(Some(e)).toMap
val page = new HtmlPage("_layouts/api-page.html", layouts("api-page").content, params, includes)
val page = new HtmlPage("_layouts" + sep + "api-page.html", layouts("api-page").content, params, includes)

render(page).foreach { rendered =>
val source = new ByteArrayInputStream(rendered.getBytes(StandardCharsets.UTF_8))
Expand All @@ -223,9 +224,9 @@ case class Site(
}

// generate search page:
val target = mkdirs(fs.getPath(outDir.getAbsolutePath + "/api/search.html"))
val target = mkdirs(fs.getPath(outDir.getAbsolutePath + sep + "api" + sep + "search.html"))
val searchPageParams = defaultParams(target.toFile, -1).withPosts(blogInfo).toMap
val searchPage = new HtmlPage("_layouts/search.html", layouts("search").content, searchPageParams, includes)
val searchPage = new HtmlPage("_layouts" + sep + "search.html", layouts("search").content, searchPageParams, includes)
render(searchPage).foreach { rendered =>
Files.copy(
new ByteArrayInputStream(rendered.getBytes(StandardCharsets.UTF_8)),
Expand Down
9 changes: 4 additions & 5 deletions project/Build.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,8 +628,7 @@ object Build {
val dottyLib = jars("dotty-library")

def run(args: List[String]): Unit = {
val sep = File.pathSeparator
val fullArgs = insertClasspathInArgs(args, s".$sep$dottyLib$sep$scalaLib")
val fullArgs = insertClasspathInArgs(args, List(".", dottyLib, scalaLib).mkString(File.pathSeparator))
runProcess("java" :: fullArgs, wait = true)
}

Expand All @@ -645,7 +644,7 @@ object Build {
val asm = findLib(attList, "scala-asm")
val dottyCompiler = jars("dotty-compiler")
val dottyInterfaces = jars("dotty-interfaces")
run(insertClasspathInArgs(args1, s"$dottyCompiler:$dottyInterfaces:$asm"))
run(insertClasspathInArgs(args1, List(dottyCompiler, dottyInterfaces, asm).mkString(File.pathSeparator)))
} else run(args)
},

Expand Down Expand Up @@ -1092,8 +1091,8 @@ object Build {
Developer(
id = "liufengyun",
name = "Liu Fengyun",
email = "[email protected]",
url = url("http://chaos-lab.com")
email = "[email protected]",
url = url("https://fengy.me")
),
Developer(
id = "nicolasstucki",
Expand Down