-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Support creating lock files on ExFAT on MacOS #17115
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
9f1640a to
7c90004
Compare
crates/uv-cache/src/lib.rs
Outdated
|
|
||
| /// Error initialising the cache | ||
| #[derive(Debug, Error)] | ||
| pub enum InitCacheError { |
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.
nit: We usually have one top level error enum per module or crate. It's a developer convenience that allows the functions and methods in the module or crate to all return the same error type, where it's easier to add new variants to a single type.
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.
I'm not sure I agree with the justification, but I will adjust to fit the style for the time being.
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.
This isn't a rule or anything fwiw, it's more a pattern that seems common, and there's definitely exceptions.
crates/uv-fs/src/locked_file.rs
Outdated
| }, | ||
| #[error("Could not open existing file")] | ||
| OpenExisting(#[source] io::Error), | ||
| #[error("Could not create or open the file")] |
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.
Does fs_err already say which one it is? If so, we can do [error(transparent)] to avoid repeating the message.
| use std::{fs::File, os::unix::fs::PermissionsExt}; | ||
| use tempfile::NamedTempFile; | ||
|
|
||
| #[allow(clippy::disallowed_types)] |
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.
Should we use fs_err::set_permissions instead? This will include the failed path in the warning.
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.
Ah, we can't easily. NamedTempFile::as_file() returns a &File and the only way to make an fs_err::File is using fs_err::File::from_parts(file: File, path: P). I will just pass the path as a parameter and print it.
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.
I see, I had only looked at the second callsite
| } else if matches!( | ||
| Errno::from_io_error(&err.error), | ||
| Some(Errno::NOTSUP | Errno::INVAL) |
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.
You mentioned something about io::ErrorKind not capturing these, can you add a comment about this and potentially file an issue with rust-lang/rust? I looked for both NOTSUP and INVAL but found no results.
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.
I will add the comment, I was on the fence but you persuaded me :)
Regarding upstream, the problem is complicated and this topic seems to have been discussed at length already: rust-lang/rust#78880
The fundamental issues are:
-
The underlying OS APIs are wildly inconsistent in how they use error values. For example, on Linux:
setxattr1 usesENOTSUPfor "The namespace prefix of name is not valid", a role traditionally taken byEINVAL, and also uses it for "Extended attributes are not supported by the filesystem, or are disabled", which is a more conventional mapping.- Meanwhile
renameat22 on Linux usesEINVALfor "An invalid flag was specified in flags" and for "The filesystem does not support one of the flags in flags."
-
Different operating systems assign different error values to different conditions. As you can see here
renameatx_np3 on MacOS usesENOTSUPfor "flags has a value that is not supported by the file system." -
Lastly,
ErrorKind::Unsupportedcould be mapped to by multiple different errno values:ENOTSUPOperation not supported.EOPNOTSUPPOperation not supported on socket (Which on Linux, but not on MacOS, has the same value asENOTSUP.EPFNOSUPPORTProtocol family not supported.EPROTONOSUPPORTProtocol not supported.ESOCKTNOSUPPORTSocket type not supported.EAFNOSUPPORTAddress family not supported.
But it's not clear if it would still be meaningful if you mapped all of these to one kind.
-
There is a fundamental ambiguity here between
EINVAL- you just passed something nonseniscal, and the concept of "unsupported". For example, the APIs are unified, despite there being fundamental differences e.g. between stream and datagram sockets, so you can attempt to perform operations that don't make sense with the current socket type, these get marked asEOPNOTSUPPeven though it could be argued they should beEINVAL.
So I am not certain there is anything meaningful we can add for upstream, the situation isn't great and there aren't many good solutions which don't involve going further upstream, and breaking userspace APIs is taboo. I think we are kind of stuck here.
Footnotes
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.
Thanks for write-up!
crates/uv-fs/src/locked_file.rs
Outdated
| // warning. | ||
| if file | ||
| .metadata() | ||
| .is_ok_and(|metadata| metadata.permissions().mode() != 0o666) |
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.
Can you move the permissions to a constant, to avoid them getting out of sync when we may change the permission set again?
Additionally, fs_err's error reporting already covers adding information about the origin of the error for many cases.
For the cases where `fs_err` already provides the necessary information.
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.9.17` -> `0.9.18` | 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.9.18`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0918) [Compare Source](astral-sh/uv@0.9.17...0.9.18) Released on 2025-12-16. ##### Enhancements - Add value hints to command line arguments to improve shell completion accuracy ([#​17080](astral-sh/uv#17080)) - Improve error handling in `uv publish` ([#​17096](astral-sh/uv#17096)) - Improve rendering of multiline error messages ([#​17132](astral-sh/uv#17132)) - Support redirects in `uv publish` ([#​17130](astral-sh/uv#17130)) - Include Docker images with the alpine version, e.g., `python3.x-alpine3.23` ([#​17100](astral-sh/uv#17100)) ##### Configuration - Accept `--torch-backend` in `[tool.uv]` ([#​17116](astral-sh/uv#17116)) ##### Performance - Speed up `uv cache size` ([#​17015](astral-sh/uv#17015)) - Initialize S3 signer once ([#​17092](astral-sh/uv#17092)) ##### Bug fixes - Avoid panics due to reads on failed requests ([#​17098](astral-sh/uv#17098)) - Enforce latest-version in `@latest` requests ([#​17114](astral-sh/uv#17114)) - Explicitly set `EntryType` for file entries in tar ([#​17043](astral-sh/uv#17043)) - Ignore `pyproject.toml` index username in lockfile comparison ([#​16995](astral-sh/uv#16995)) - Relax error when using `uv add` with `UV_GIT_LFS` set ([#​17127](astral-sh/uv#17127)) - Support file locks on ExFAT on macOS ([#​17115](astral-sh/uv#17115)) - Change schema for `exclude-newer` into optional string ([#​17121](astral-sh/uv#17121)) ##### Documentation - Drop arm musl caveat from Docker documentation ([#​17111](astral-sh/uv#17111)) - Fix version reference in resolver example ([#​17085](astral-sh/uv#17085)) - Better documentation for `exclude-newer*` ([#​17079](astral-sh/uv#17079)) </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:eyJjcmVhdGVkSW5WZXIiOiI0Mi41Ny4xIiwidXBkYXRlZEluVmVyIjoiNDIuNTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Summary
Partially address #16859 by falling back to simply creating the lock file and then attempting to apply permissions in cases where the temporary lockfile cannot be renamed without overwriting (persist_noclobber) due to lack of underlying support from the filesystem.
I've also improved the error handling, but I think the
as_io_errorthing isn't a good idea1. Something for another PR though.Assuming the error handling PR came first, the error from the issue would look like:
I think this is more helpful.
Test Plan
Manually on MacOS with an ExFAT partition.
$ hdiutil create -size 1g -fs ExFAT -volname EXFATDISK exfat.dmg $ hdiutil attach exfat.dmg $ cd /Volumes/EXFATDISK $ uv init --bare --cache-dir build/uv/cache -vFootnotes
In summary, I noticed that all the
as_io_errorimplementations seem to combine disparate underlying sources of errors and are used to check for cases where there is only one appropriate source for the error case. For example, incrates/uv-cache/src/lib.rsas_io_erroris being used to check if shared locking is unsupported, which only has one source:LockedFileError::Lock, butas_io_errorwill also include other sources of errors causing the check to actually be broader than appropriate. ↩