Migrate from std::fs::canonicalise to dunce
#5538
Replies: 4 comments
-
|
People were investigating this on discord and from what I recall this was actually an issue with Node. Are there other places where this canonicalisation resulted in problems? |
Beta Was this translation helpful? Give feedback.
-
|
I don't know about other problems, but at least from my tests using dunce fixes tests |
Beta Was this translation helpful? Give feedback.
-
|
I think changing something in the compiler to work around a bug in Node seems not great. Could we not upgrade the Node version, or use a different runtime? |
Beta Was this translation helpful? Give feedback.
-
We can stay on bug-free version for now, but on some point we will need to update anyway. Is this bug reported to Node?
At least those failing tests run on all targets and runtimes, so that's not an option |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
The problem
Paths on Windows are hard. Currently we canonicalise paths with
std::fs::canonicalise, which has a drawback of making paths UNC paths, so they look like\\\\?\\C:\\Users\\user\\hello.txt(or\\?\C:\Users\user\hello.txtif remove escapes). This allows long paths, but many programs don't play with them well, so we get issues with them. The most recent problem are failingechointegration tests on Windows in the CI (they failed on my Windows machine even before).Possible solution
We can use
duncecrate, which is widely used in the Rust ecosystem. On Windows it tries to canonicalise paths without UNC, and does exactly whatstd::fs::canonicalisedoes on other platforms, so there are no problems with it.Another possible solution
I didn't check, but I've read that
std::path::absolutedoesn't create UNC paths.And another solution
We can just manually remove this prefix, like here:
gleam/language-server/src/compiler.rs
Lines 142 to 149 in 2a56ee9
If this is desired thing, I will work on it
Beta Was this translation helpful? Give feedback.
All reactions