-
Notifications
You must be signed in to change notification settings - Fork 52
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 normalized URI generation for UNC path #374
base: develop
Are you sure you want to change the base?
Conversation
@@ -1160,7 +1160,7 @@ object IO { | |||
*/ | |||
def directoryURI(dir: File): URI = { | |||
assertAbsolute(dir) | |||
directoryURI(dir.toURI.normalize) | |||
directoryURI(dir.toPath.normalize.toUri) |
There was a problem hiding this comment.
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?
@@ -1177,7 +1177,7 @@ object IO { | |||
else | |||
new URI(str + "/") | |||
|
|||
dirURI.normalize | |||
new File(dirURI).toPath.normalize.toUri |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I must say this is more complex than I anticipated... The PR, as of its current state, is inadequate. The issue is that, the dollar sign actually matters when parsing a URI. Probably since $ is a reserved character.
The u4 URI is fragile. After calling So here's the current state:
|
Issue
IO.directoryURI
andPath.absolute
both callstoURI.normalize
to convert ajava.io.File
to a normalized URI, which generates invalid URI for UNC path. As a result, sbt crashes when we try to open a project in a WSL directory.Example: say we have a
dir: java.io.File
representingUbuntu-24.04/home/jerrytan/testwsl/
in WSL, then callingdir.toPath.toUri.normalize
yieldsfile:/wsl$/Ubuntu-24.04/home/jerrytan/testwsl/
, which then leads toFix
Call
toPath.normalize.toUri
instead. For above example, it yieldsfile:////wsl$/Ubuntu-24.04/home/jerrytan/testwsl/
which can be loaded by sbt. Note that the new file URL still do not follow the best practice as documented in Eugene's blog post, but it indeed do not crash.Closes sbt/sbt#7135
Credit to reporter of sbt/sbt#7135 for detailed investigation of the issue.