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 1 commit
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
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
8 changes: 4 additions & 4 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 @@ -167,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(sep).length
val assetLen = pageLocation.getAbsolutePath.split(sep).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 @@ -203,7 +203,7 @@ case class Site(

val path = if (scala.util.Properties.isWin)
e.path.map(_.replace("<", "_").replace(">", "_"))
else
else
e.path
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
Expand Down