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.

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

Fix normalized URI generation for UNC path #374

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Fix normalized URI generation for UNC path #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ ThisBuild / version := {
(sys.env.get("BUILD_VERSION") orElse sys.props.get("sbt.build.version")) match {
case Some(v) => v
case _ =>
if ((ThisBuild / isSnapshot).value) "1.6.0-SNAPSHOT"
if ((ThisBuild / isSnapshot).value) "1.10.2-SNAPSHOT"
else old
}
}
Expand Down
4 changes: 2 additions & 2 deletions io/src/main/scala/sbt/io/IO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ object IO {
*/
def directoryURI(dir: File): URI = {
assertAbsolute(dir)
directoryURI(dir.toURI.normalize)
directoryURI(dir.toPath.normalize.toUri)
Copy link
Member

Choose a reason for hiding this comment

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

Could we add unit test for this plz?

}

/**
Expand All @@ -1177,7 +1177,7 @@ object IO {
else
new URI(str + "/")

dirURI.normalize
new File(dirURI).toPath.normalize.toUri
Copy link
Member

Choose a reason for hiding this comment

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

The creation of the first URI object seems wasteful here. See also #132, which states that URI object can lead to hundreds of MB in heap.

Copy link
Member Author

@Friendseeker Friendseeker Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shall investigate this once I regain access to a Windows Machine.

Indeed there is something fishy there. If dirURI follows u2 notation, then calling dirURI.normalize should be correct. While I think the other changes in the PR makes sense, this particular change is just a bandaid that somehow fixes the sbt crash without fixing the underlying issue.

I now actually suspect the hidden issue might be with IO.toURL, which is called by ProjectRef.apply on sbt side. This is just a hypothesis though.

Copy link
Member Author

@Friendseeker Friendseeker Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw going slightly off topic but your blogpost about file URI schema is so helpful! Without it I probably need to spend a lot of time researching about file URI.

}

private[sbt] val isWindows: Boolean =
Expand Down
2 changes: 1 addition & 1 deletion io/src/main/scala/sbt/io/Path.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ object Path extends Mapper {
def fileProperty(name: String): File = new File(System.getProperty(name))
def userHome: File = fileProperty("user.home")

def absolute(file: File): File = new File(file.toURI.normalize).getAbsoluteFile
def absolute(file: File): File = new File(file.toPath.normalize.toUri).getAbsoluteFile
def makeString(paths: Seq[File]): String = makeString(paths, File.pathSeparator)
def makeString(paths: Seq[File], sep: String): String = {
val separated = paths.map(_.getAbsolutePath)
Expand Down