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.

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

Use files instead of junctions on Windows #11269

Merged
merged 4 commits into from
Feb 8, 2025
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Feb 6, 2025

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_symlink should now be on atomic on Windows.

Closes #11263.

@charliermarsh charliermarsh added the no-build Disable building binaries in CI label Feb 6, 2025
@charliermarsh charliermarsh force-pushed the charlie/sym branch 7 times, most recently from a4bc912 to 4e21181 Compare February 6, 2025 01:54
@charliermarsh charliermarsh added the windows Specific to the Windows platform label Feb 6, 2025
@charliermarsh charliermarsh marked this pull request as ready for review February 6, 2025 02:02
@charliermarsh charliermarsh force-pushed the charlie/sym branch 2 times, most recently from 7a01a26 to 4a3c71c Compare February 6, 2025 02:08
@charliermarsh charliermarsh requested a review from zanieb February 6, 2025 02:18
@charliermarsh
Copy link
Member Author

We might be able to remove some of the extra flock usages we have on Windows after this, but I want to test and evaluate that separately. The proximate motivation here is that Ofek / Datadog are seeing a really strange behavior in their Windows container whereby we had a junction that... didn't point to anything? Like, it's not that the target was gone, the target was empty.

),
));
}
// First, attempt to create a file at the location, but fail if it doesn't exist.
Copy link
Member

@konstin konstin Feb 6, 2025

Choose a reason for hiding this comment

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

Does this intend to say "fail if it does exist"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

Comment on lines 66 to 67
let temp_dir = tempfile::tempdir_in(dst.as_ref().parent().unwrap())?;
let temp_file = temp_dir.path().join("link");
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a tempdir or can we directly create a tempfile with a random name in the directory?

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an error? Or perhaps a WARN log or something?

else {
continue;
};
if WheelFilename::from_stem(filename).is_err() {
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

if WheelFilename::from_stem(filename).is_err() {
continue;
}
if let Ok(target) = uv_fs::resolve_symlink(entry.path()) {
Copy link
Member

Choose a reason for hiding this comment

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

And here.

Although I guess we weren't emitting logs before. I wonder if it might help debugging if something goes wrong here.

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())?;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thank you!

@charliermarsh charliermarsh requested a review from Gankra February 6, 2025 17:14
@charliermarsh charliermarsh changed the base branch from main to charlie/version February 7, 2025 02:34
@charliermarsh
Copy link
Member Author

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.

@charliermarsh
Copy link
Member Author

(I didn't mean to close, accidental click.)

@ofek
Copy link
Contributor

ofek commented Feb 7, 2025

In the future, would it be possible to detect symlink capability on Windows and use this new approach as a fallback?

@charliermarsh charliermarsh added this to the v0.6.0 milestone Feb 7, 2025
@zanieb zanieb mentioned this pull request Feb 7, 2025
Base automatically changed from charlie/version to tracking/060 February 7, 2025 22:20
@charliermarsh charliermarsh force-pushed the charlie/sym branch 3 times, most recently from ccc1326 to 439e70f Compare February 7, 2025 23:12
@charliermarsh charliermarsh merged commit 238bc29 into tracking/060 Feb 8, 2025
83 checks passed
@charliermarsh charliermarsh deleted the charlie/sym branch February 8, 2025 00:13
charliermarsh added a commit that referenced this pull request Feb 10, 2025
## 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_symlink` should now be on atomic
on Windows.

Closes #11263.
Gankra pushed a commit that referenced this pull request Feb 12, 2025
## 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_symlink` should now be on atomic
on Windows.

Closes #11263.
zanieb pushed a commit that referenced this pull request Feb 13, 2025
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_symlink` should now be on atomic
on Windows.

Closes #11263.
zanieb pushed a commit that referenced this pull request Feb 13, 2025
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_symlink` should now be on atomic
on Windows.

Closes #11263.
zanieb pushed a commit that referenced this pull request Feb 13, 2025
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_symlink` should now be on atomic
on Windows.

Closes #11263.
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 17, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | minor | `0.5.31` -> `0.6.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.6.0`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#060)

[Compare Source](astral-sh/uv@0.5.31...0.6.0)

There have been 31 releases and 1135 pull requests since [0.5.0](https://github.com/astral-sh/uv/releases/tag/0.5.0), our last release with breaking changes. As before, we've accumulated various changes that improve correctness and user experience, but could break some workflows. This release contains those changes; many have been marked as breaking out of an abundance of caution. We expect most users to be able to upgrade without making changes.

##### Breaking changes

-   **Create `main.py` instead of `hello.py` in `uv init`** ([#&#8203;10369](astral-sh/uv#10369))

    Previously, `uv init` created a `hello.py` sample file. Now, `uv init` will create `main.py` instead — which aligns with expectations from user feedback. The `--bare` option can be used to avoid creating the file altogether.

-   **Respect `UV_PYTHON` in `uv python install`** ([#&#8203;11487](astral-sh/uv#11487))

    Previously, `uv python install` did not read this environment variable; now it does. We believe this matches user expectations, however, this will take priority over `.python-version` files which could be considered breaking.

-   **Set `UV` to the uv executable path** ([#&#8203;11326](astral-sh/uv#11326))

    When uv spawns a subprocess, it will now have the `UV` environment variable set to the `uv` binary path. This change is breaking if you are setting the `UV` environment variable yourself, as we will overwrite its value.

    Additionally, this change requires marking the uv Rust entrypoint (`uv::main`) as `unsafe` to avoid unsoundness — this is only relevant if you are invoking uv using Rust. See the [Rust documentation](https://doc.rust-lang.org/std/env/fn.set_var.html#safety) for details about the safety of updating a process' environment.

-   **Error on non-existent extras, e.g., in `uv sync`** ([#&#8203;11426](astral-sh/uv#11426))

    Previously, uv would silently ignore non-existent extras requested on the command-line (e.g., via `uv sync --extra foo`). This is *generally* correct behavior when resolving requests for package extras, because an extra may be present on one compatible version of a package but not another. However, this flexibility doesn't need to apply to the local project and it's less surprising to error here.

-   **Error on missing dependency groups when `--frozen` is provided** ([#&#8203;11499](astral-sh/uv#11499))

    Previously, uv would not validate that the requested dependency groups were present in the lockfile when the `--frozen` flag was used. Now, an error will be raised if a requested dependency group is not present.

-   **Change `-p` to a `--python` alias in `uv pip compile`** ([#&#8203;11486](astral-sh/uv#11486))

    In `uv pip compile`, `-p` was an alias for `--python-version` while everywhere else in uv's interface it is an alias for `--python`. Additionally, `uv pip compile` did not respect the `UV_PYTHON` environment variable. Now, the semantics of this flag have been updated for parity with the rest of the CLI.

    However, `--python-version` is unique: if we cannot find an interpreter with the given version, we will not fail. Instead, we'll use an alternative interpreter and override its version tags with the requested version during package resolution. This behavior is retained here for backwards compatibility, `--python <version>` / `-p <version>` will not fail if the version cannot be found. However, if a specific interpreter is requested, e.g., with `--python <path>` or `--python pypy`, and cannot be found — uv will exit with an error.

    The breaking changes here are that `UV_PYTHON` is respected and `--python <version>` will no longer fail if the version cannot be found.

-   **Bump `alpine` default tag to 3.21 for derived Docker images** ([#&#8203;11157](astral-sh/uv#11157))

    Alpine 3.21 was released in Dec 2024 and is used in the official Alpine-based Python images. Our `uv:python3.x-alpine` images have been using 3.21 since uv v0.5.8. However, now the the `uv:alpine` image will use 3.21 instead of 3.20 and `uv:alpine3.20` will no longer be updated.

-   **Use files instead of junctions on Windows** ([#&#8203;11269](astral-sh/uv#11269))

    Previously, we used junctions for atomic replacement of cache entries on Windows. Now, we use a file with a pointer to the cache entry instead. This resolves various edge-case behaviors with junctions. These files are only intended to be consumed by uv and the cache version has been bumped. We do not think this change will affect workflows.

##### Stabilizations

-   **`uv publish` is no longer in preview** ([#&#8203;11032](astral-sh/uv#11032))

    This does not come with any behavior changes. You will no longer see an experimental warning when using `uv publish`. See the linked pull request for a report on the stabilization.

##### Enhancements

-   Support `--active` for PEP 723 script environments ([#&#8203;11433](astral-sh/uv#11433))
-   Add `revision` to the lockfile to allow backwards-compatible metadata changes ([#&#8203;11500](astral-sh/uv#11500))

##### Bug fixes

-   Avoid reading metadata from `.egg-info` files ([#&#8203;11395](astral-sh/uv#11395))
-   Include archive bucket version in archive pointers ([#&#8203;11306](astral-sh/uv#11306))
-   Omit lockfile version when additional fields are dynamic ([#&#8203;11468](astral-sh/uv#11468))
-   Respect executable name in `uvx --from tool@latest` ([#&#8203;11465](astral-sh/uv#11465))

##### Documentation

-   The `CHANGELOG.md` is now split into separate files for each "major" version to fix rendering ([#&#8203;11510](astral-sh/uv#11510))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNzAuMSIsInVwZGF0ZWRJblZlciI6IjM5LjE3MC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
loic-lescoat pushed a commit to loic-lescoat/uv that referenced this pull request Mar 2, 2025
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_symlink` should now be on atomic
on Windows.

Closes astral-sh#11263.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-build Disable building binaries in CI windows Specific to the Windows platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace junctions with "fake" symlinks on Windows
4 participants