Use files instead of junctions on Windows #11269
Conversation
52b556d to
cfc11eb
Compare
a4bc912 to
4e21181
Compare
7a01a26 to
4a3c71c
Compare
|
We might be able to remove some of the extra |
crates/uv-fs/src/lib.rs
Outdated
| ), | ||
| )); | ||
| } | ||
| // First, attempt to create a file at the location, but fail if it doesn't exist. |
There was a problem hiding this comment.
Does this intend to say "fail if it does exist"?
crates/uv-fs/src/lib.rs
Outdated
| let temp_dir = tempfile::tempdir_in(dst.as_ref().parent().unwrap())?; | ||
| let temp_file = temp_dir.path().join("link"); |
There was a problem hiding this comment.
Do we need a tempdir or can we directly create a tempfile with a random name in the directory?
There was a problem hiding this comment.
Rolling your own symlinks because Windows-native symlinks are so bad... It's just crazy enough to work! Haha.
I made a suggestion about how to avoid the lossy call here. Sorry about being unclear last night.
I also wonder if perhaps we can emit some more logs in places, but I defer to you on that. I don't have a ton of context on this area of the code.
| .path() | ||
| .file_name() | ||
| .and_then(|file_name| file_name.to_str()) | ||
| else { |
There was a problem hiding this comment.
Should this be an error? Or perhaps a WARN log or something?
| else { | ||
| continue; | ||
| }; | ||
| if WheelFilename::from_stem(filename).is_err() { |
crates/uv-cache/src/lib.rs
Outdated
| if WheelFilename::from_stem(filename).is_err() { | ||
| continue; | ||
| } | ||
| if let Ok(target) = uv_fs::resolve_symlink(entry.path()) { |
There was a problem hiding this comment.
And here.
Although I guess we weren't emitting logs before. I wonder if it might help debugging if something goes wrong here.
crates/uv-fs/src/lib.rs
Outdated
| Ok(mut file) => { | ||
| // Write the target path to the file. | ||
| use std::io::Write; | ||
| file.write_all(src.as_ref().to_string_lossy().as_bytes())?; |
There was a problem hiding this comment.
Oh, okay, now that I know what you are doing here, I think I can suggest something more bullet proof that isn't too much work. Basically, on Windows, file paths are just arbitrary sequences of u16 (with some restrictions), just like on Unix, file paths are just arbitrary sequences of u8. So the way to avoid the lossy conversion here is to just store the u16 sequence. You can get the raw u16 sequence from this API in std: https://doc.rust-lang.org/std/os/windows/ffi/trait.OsStrExt.html
Once you have a Vec<u16> build in memory, create a Vec<u8> with twice the length and then use byteorder to (safely) write the &[u16] into a &mut [u8]: https://docs.rs/byteorder/latest/byteorder/trait.ByteOrder.html#tymethod.write_u16_into
Then save that to the file.
To go in the other direction, use ByteOrder::read_u16_into and std::os::windows::ffi::OsStringExt.
When using byteorder, I'd suggest picking a specific byteorder (byteorder::BigEndian) and always using it. Regardless of the endianness of the host platform. That way, the file format is portable.
There was a problem hiding this comment.
Awesome, thank you!
4a3c71c to
42af524
Compare
|
Ok, I simplified things a bit (I think). Instead of "generically" using these routines to read and write symlinks, they're now methods on the cache specifically framed at writing links to archive entries and reading links to archive entries. |
|
(I didn't mean to close, accidental click.) |
|
In the future, would it be possible to detect symlink capability on Windows and use this new approach as a fallback? |
Summary
Instead of using junctions, we can just write files that contain (as the file contents) the target path. This requires a little more finesse in that, as readers, we need to know where to expect these. But it also means we get to avoid junctions, which have led to a variety of confusing behaviors. Further,
replace_symlinkshould now be on atomic on Windows.Closes #11263.