workaround unc path on windows #38
Conversation
ncloudioj
force-pushed
the
fix-windows-unc-path
branch
from
16326c0 to
c79cde7
Compare
src/manager.rs
Outdated
| fn canonicalize_path<'p, P>(path: P) -> io::Result<PathBuf> | ||
| where P: Into<&'p Path> { | ||
| let canonical = path.into().canonicalize()?; | ||
| { |
There was a problem hiding this comment.
It seems like we should preface this block to compile it only on Windows:
#[cfg(windows)]Then other platforms won't pay the cost of the workaround.
There was a problem hiding this comment.
Good point, will tweak it a bit based on all other review feedback.
| @@ -44,6 +46,24 @@ lazy_static! { | |||
| }; | |||
| } | |||
|
|
|||
| // Workaround the UNC path on Windows, see https://github.com/rust-lang/rust/issues/42869. | |||
| // Otherwise, `Env::from_env()` will panic with error_no(123). | |||
There was a problem hiding this comment.
It'd be good to file this upstream, in lmdb-rs, or perhaps farther upstream, in LMDB itself, although the latter requires grokking the OpenLDAP Issue Tracking System, so I'd probably start with the former. 😉
There was a problem hiding this comment.
I'd recommend to do them both, i.e. having a workaround in rkv for the time being so that it works on Windows at least to get CI rolling. On the other hand, pop this back to upstream(s) in hope that this would get fixed in the right place. Yep, let me try it from lmdb-rs first, cuz I suspect that the windows mmap syscall won't play nice with UNC path, since it returned a error_no(123), which means it received an invalid directory according to its document.
Heh, I used to be on the lmdb's mailing list a while ago, and filed various bugs there, will try to grok their new issue tracking system if necessary :)
src/manager.rs
Outdated
| let mut canonical_path = canonical.to_str().unwrap(); | ||
| if canonical_path.starts_with(r"\\?\") { | ||
| canonical_path = &canonical_path[r"\\?\".len()..]; | ||
| return Ok(PathBuf::from(canonical_path)); |
There was a problem hiding this comment.
Hmm, I wonder if it would be more robust to unconditionally strip the UNC prefix from paths via the Url crate, i.e. something like:
Url::from_file_path(canonical).expect("URL").to_file_path().expect("path")I don't know if this would cover more cases or fail less often than the current implementation, however. What do you think?
There was a problem hiding this comment.
Sure, will check it out.
| @@ -44,6 +46,24 @@ lazy_static! { | |||
| }; | |||
| } | |||
|
|
|||
| // Workaround the UNC path on Windows, see https://github.com/rust-lang/rust/issues/42869. | |||
There was a problem hiding this comment.
Ah, rust-lang/rust#42869, I know it well (unfortunately).
There was a problem hiding this comment.
Haha, so you've also been there before. Such a small world! :)
ncloudioj
force-pushed
the
fix-windows-unc-path
branch
from
c79cde7 to
e469367
Compare
|
Refactored it by using the url crate. Also, filed an issue for this in lmdb-rs. |
Workaround the UNC path on Windows, all the tests passed in the current stable toolchain "stable-x86_64-pc-windows-msvc"
Note that:
@mykmelez