-
Notifications
You must be signed in to change notification settings - Fork 900
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
Implement Toolchain::find_or_fetch
and use in uv venv --preview
#4138
Conversation
// Order matters here, as we overwrite previous links | ||
info!("Installing to `{}`...", toolchain_dir.user_display()); | ||
|
||
// On Windows, linking the executable generally results in broken installations | ||
// and each toolchain path will need to be added to the PATH separately in the | ||
// desired order | ||
#[cfg(unix)] | ||
{ | ||
let mut links: HashMap<PathBuf, PathBuf> = HashMap::new(); | ||
for (version, path) in results { | ||
// TODO(zanieb): This path should be a part of the download metadata | ||
let executable = path.join("install").join("bin").join("python3"); | ||
for target in [ | ||
toolchain_dir.join(format!("python{}", version.python_full_version())), | ||
toolchain_dir.join(format!("python{}.{}", version.major(), version.minor())), | ||
toolchain_dir.join(format!("python{}", version.major())), | ||
toolchain_dir.join("python"), | ||
] { | ||
// Attempt to remove it, we'll fail on link if we couldn't remove it for some reason | ||
// but if it's missing we don't want to error | ||
let _ = fs::remove_file(&target); | ||
symlink(&executable, &target).await?; | ||
links.insert(target, executable.clone()); | ||
} | ||
} | ||
for (target, executable) in links.iter().sorted() { | ||
info!( | ||
"Linked `{}` to `{}`", | ||
target.user_display(), | ||
executable.user_display() | ||
); | ||
} | ||
}; |
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 whole thing is no longer necessary, we've been discovering managed toolchains via their directories instead of executables for a while now
62c4cad
to
b476a30
Compare
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.
Will review, but while it's on my mind: should there be a way to force a project to use managed toolchains? Like, fetch and install even if you have some non-managed thing on your PATH?
There's a hidden environment variable right now ( |
Ok(Self { root: root.into() }) | ||
} | ||
|
||
/// Prefer, in order: | ||
/// 1. The specific toolchain directory specified by the user, i.e., `UV_TOOLCHAIN_DIR` | ||
/// 2. A directory in the system-appropriate user-level data directory, e.g., `~/.local/uv/toolchains` | ||
/// 3. A directory in the local data directory, e.g., `./.uv/toolchains` | ||
pub fn from_settings() -> Result<Self, io::Error> { | ||
pub fn from_settings() -> Result<Self, Error> { |
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.
With the #[error(transparent)]
on Error
, i think the callsites of from_settings
should have context that the error comes from toolchain discovery.
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 what the action item is here.
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 think the suggestion is that if you're using transparent IO errors, there will be no indication of why the IO error occurred. Like, if we fail to read or write a file, the entire error trace would just be "Failed to write the file". Using context, you can say that we failed during toolchain discovery.
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 interesting, thanks. I'll look into that.
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.
There's two kinds of adding context to errors: One would be replacing something like
#[error(transparent)]
ManagedToolchain(#[from] crate::managed::Error),
with
#[error("Something about what this error is, maybe a {python version}")]
ManagedToolchain(#[from] crate::managed::Error),
The other is replacing InstalledToolchains::from_settings()?.init()?
with
let toolchains = InstalledToolchains::from_settings()?.init()?;
let toolchains = InstalledToolchains::from_settings()
.context("Failed to load installed toolchains")?
.init()
.context("Failed to initialize toolchain discovery")?;
This requires some manual auditing of who adds the context, e.g. if you're asking for a specific python version, either the caller or the callee should be responsible for reporting the python version.
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 again!
# Conflicts: # crates/uv-toolchain/src/toolchain.rs
Extends #4121
Part of #2607
Adds support for managed toolchain fetching to
uv venv
, e.g.The preview flag is required. The fetch is performed if we can't find an interpreter that satisfies the request. Once fetched, the toolchain will be available for later invocations that include the
--preview
flag. There will be follow-ups to improve toolchain management in general, there is still outstanding work from the initial implementation.