-
Notifications
You must be signed in to change notification settings - Fork 2.2k
default the test buckets dir to the canonicalized temp dir #14290
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
Conversation
|
Don't you need to unset |
|
https://github.com/astral-sh/uv/actions/runs/15908534044 is a CI run on a scratch branch that uses If you think this is a reasonable change, I could go ahead and remove |
|
Ah no, I'm probably breaking something on Windows. I got lazy and assumed that Windows failures were timeouts after the macOS runner passed. Incorrect! |
|
Ok, "canonicalize -> simple_canonicalize" fixed it. |
zanieb
left a comment
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.
Fine with me
Previously we were using the XDG data dir to avoid symlinks, but there's no particular guarantee that that's not going to be a symlink too. Using the canonicalized temp dir by default is also slightly nicer for a couple reasons: It's sometimes faster (an in-memory tempfs on e.g. Arch), and it makes overriding `$TMPDIR` or `%TMP%` sufficient to control where tests put temp files, without needing to override `UV_INTERNAL__TEST_DIR` too.
6686bc1 to
33fa9d9
Compare
## Summary This came up on [Discord](https://discord.com/channels/1039017663004942429/1343692072921731082/1395447082520678440) and also in #19387, but on macOS the tmp directory is a symlink to `/private/tmp`, which breaks this filter. I'm still not quite sure why only these tests are affected when we use the `tempdir_filter` elsewhere, but hopefully this fixes the immediate issue. Just `tempdir.path().canonicalize()` also worked, but I used `dunce` since that's what I saw in other tests (I guess it's not _just_ these tests). Some related links from uv: - https://github.com/astral-sh/uv/blob/1b2f212e8b2f91069b858cb7f5905589c9d15add/crates/uv/tests/it/common/mod.rs#L1161-L1178 - https://github.com/astral-sh/uv/blob/1b2f212e8b2f91069b858cb7f5905589c9d15add/crates/uv/tests/it/common/mod.rs#L424-L438 - astral-sh/uv#14290 Thanks to @zanieb for those! ## Test Plan I tested the `main` branch on my MacBook and reproduced the test failure, then confirmed that the tests pass after the change. Now to make sure it passes on Windows, which caused most of the trouble in the first PR!
Previously we were using the XDG data dir to avoid symlinks, but there's no particular guarantee that that's not going to be a symlink too. Using the (canonicalized) temp dir by default is also slightly nicer for a couple reasons: It's sometimes faster (an in-memory tempfs on e.g. Arch), and it makes overriding
$TMPDIRor%TMP%sufficient to control where tests put temp files, without needing to overrideUV_INTERNAL__TEST_DIRtoo.