-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[WIP] Add a --suffix
option to uv tool install
#7095
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,13 @@ pub enum Error { | |
Serialization(#[from] toml_edit::ser::Error), | ||
} | ||
|
||
/// A Package identitifier | ||
#[derive(Debug, Clone)] | ||
pub struct ToolName { | ||
pub name: PackageName, | ||
pub suffix: Option<String>, | ||
} | ||
|
||
/// A collection of uv-managed tools installed on the current system. | ||
#[derive(Debug, Clone)] | ||
pub struct InstalledTools { | ||
|
@@ -87,8 +94,12 @@ impl InstalledTools { | |
} | ||
|
||
/// Return the expected directory for a tool with the given [`PackageName`]. | ||
pub fn tool_dir(&self, name: &PackageName) -> PathBuf { | ||
self.root.join(name.to_string()) | ||
pub fn tool_dir(&self, name: &ToolName) -> PathBuf { | ||
if let Some(suffix) = &name.suffix { | ||
self.root.join(name.name.to_string() + suffix) | ||
} else { | ||
self.root.join(name.name.to_string()) | ||
} | ||
} | ||
|
||
/// Return the metadata for all installed tools. | ||
|
@@ -130,7 +141,7 @@ impl InstalledTools { | |
/// error. | ||
/// | ||
/// Note it is generally incorrect to use this without [`Self::acquire_lock`]. | ||
pub fn get_tool_receipt(&self, name: &PackageName) -> Result<Option<Tool>, Error> { | ||
pub fn get_tool_receipt(&self, name: &ToolName) -> Result<Option<Tool>, Error> { | ||
let path = self.tool_dir(name).join("uv-receipt.toml"); | ||
match ToolReceipt::from_path(&path) { | ||
Ok(tool_receipt) => Ok(Some(tool_receipt.tool)), | ||
|
@@ -149,12 +160,13 @@ impl InstalledTools { | |
/// Any existing receipt will be replaced. | ||
/// | ||
/// Note it is generally incorrect to use this without [`Self::acquire_lock`]. | ||
pub fn add_tool_receipt(&self, name: &PackageName, tool: Tool) -> Result<(), Error> { | ||
pub fn add_tool_receipt(&self, name: &ToolName, tool: Tool) -> Result<(), Error> { | ||
let tool_receipt = ToolReceipt::from(tool); | ||
let path = self.tool_dir(name).join("uv-receipt.toml"); | ||
|
||
debug!( | ||
"Adding metadata entry for tool `{name}` at {}", | ||
"Adding metadata entry for tool `{}` at {}", | ||
name.name, | ||
path.user_display() | ||
); | ||
|
||
|
@@ -175,11 +187,12 @@ impl InstalledTools { | |
/// # Errors | ||
/// | ||
/// If no such environment exists for the tool. | ||
pub fn remove_environment(&self, name: &PackageName) -> Result<(), Error> { | ||
pub fn remove_environment(&self, name: &ToolName) -> Result<(), Error> { | ||
let environment_path = self.tool_dir(name); | ||
|
||
debug!( | ||
"Deleting environment for tool `{name}` at {}", | ||
"Deleting environment for tool `{}` at {}", | ||
name.name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll generally probably want to see the both the name and suffix that we're performing operations on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit curious why you did this instead of implementing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, didn't change to showing the whole tool name to simplify for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a TODO for changing how ToolNames are displayed, since it needs discussion |
||
environment_path.user_display() | ||
); | ||
|
||
|
@@ -196,15 +209,16 @@ impl InstalledTools { | |
/// Note it is generally incorrect to use this without [`Self::acquire_lock`]. | ||
pub fn get_environment( | ||
&self, | ||
name: &PackageName, | ||
name: &ToolName, | ||
cache: &Cache, | ||
) -> Result<Option<PythonEnvironment>, Error> { | ||
let environment_path = self.tool_dir(name); | ||
|
||
match PythonEnvironment::from_root(&environment_path, cache) { | ||
Ok(venv) => { | ||
debug!( | ||
"Using existing environment for tool `{name}`: {}", | ||
"Using existing environment for tool `{}`: {}", | ||
name.name, | ||
environment_path.user_display() | ||
); | ||
Ok(Some(venv)) | ||
|
@@ -228,7 +242,7 @@ impl InstalledTools { | |
/// Note it is generally incorrect to use this without [`Self::acquire_lock`]. | ||
pub fn create_environment( | ||
&self, | ||
name: &PackageName, | ||
name: &ToolName, | ||
interpreter: Interpreter, | ||
) -> Result<PythonEnvironment, Error> { | ||
let environment_path = self.tool_dir(name); | ||
|
@@ -237,7 +251,8 @@ impl InstalledTools { | |
match fs_err::remove_dir_all(&environment_path) { | ||
Ok(()) => { | ||
debug!( | ||
"Removed existing environment for tool `{name}`: {}", | ||
"Removed existing environment for tool `{}`: {}", | ||
name.name, | ||
environment_path.user_display() | ||
); | ||
} | ||
|
@@ -246,7 +261,8 @@ impl InstalledTools { | |
} | ||
|
||
debug!( | ||
"Creating environment for tool `{name}`: {}", | ||
"Creating environment for tool `{}`: {}", | ||
name.name, | ||
environment_path.user_display() | ||
); | ||
|
||
|
@@ -271,15 +287,15 @@ impl InstalledTools { | |
} | ||
|
||
/// Return the [`Version`] of an installed tool. | ||
pub fn version(&self, name: &PackageName, cache: &Cache) -> Result<Version, Error> { | ||
pub fn version(&self, name: &ToolName, cache: &Cache) -> Result<Version, Error> { | ||
let environment_path = self.tool_dir(name); | ||
let environment = PythonEnvironment::from_root(&environment_path, cache)?; | ||
let site_packages = SitePackages::from_environment(&environment) | ||
.map_err(|err| Error::EnvironmentRead(environment_path.clone(), err.to_string()))?; | ||
let packages = site_packages.get_packages(name); | ||
let packages = site_packages.get_packages(&name.name); | ||
let package = packages | ||
.first() | ||
.ok_or_else(|| Error::MissingToolPackage(name.clone()))?; | ||
.ok_or_else(|| Error::MissingToolPackage(name.name.clone()))?; | ||
Ok(package.version().clone()) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,9 @@ use uv_installer::SitePackages; | |
use uv_python::PythonEnvironment; | ||
use uv_settings::ToolOptions; | ||
use uv_shell::Shell; | ||
use uv_tool::{entrypoint_paths, find_executable_directory, InstalledTools, Tool, ToolEntrypoint}; | ||
use uv_tool::{ | ||
entrypoint_paths, find_executable_directory, InstalledTools, Tool, ToolEntrypoint, ToolName, | ||
}; | ||
use uv_warnings::warn_user; | ||
|
||
use crate::commands::ExitStatus; | ||
|
@@ -71,13 +73,19 @@ pub(crate) fn install_executables( | |
python: Option<String>, | ||
requirements: Vec<Requirement>, | ||
printer: Printer, | ||
suffix: &Option<String>, | ||
) -> anyhow::Result<ExitStatus> { | ||
let site_packages = SitePackages::from_environment(environment)?; | ||
let installed = site_packages.get_packages(name); | ||
let Some(installed_dist) = installed.first().copied() else { | ||
bail!("Expected at least one requirement") | ||
}; | ||
|
||
let pkg = ToolName { | ||
name: name.clone(), | ||
suffix: suffix.clone(), | ||
}; | ||
|
||
// Find a suitable path to install into | ||
let executable_directory = find_executable_directory()?; | ||
fs_err::create_dir_all(&executable_directory) | ||
|
@@ -98,12 +106,15 @@ pub(crate) fn install_executables( | |
let target_entry_points = entry_points | ||
.into_iter() | ||
.map(|(name, source_path)| { | ||
let target_path = executable_directory.join( | ||
source_path | ||
.file_name() | ||
.map(std::borrow::ToOwned::to_owned) | ||
.unwrap_or_else(|| OsString::from(name.clone())), | ||
); | ||
let mut file_name = source_path | ||
.file_name() | ||
.map(std::borrow::ToOwned::to_owned) | ||
.unwrap_or_else(|| OsString::from(name.clone())); | ||
if let Some(suffix) = suffix { | ||
file_name.push(suffix); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this handle file extensions on Windows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the suffix between the stem and the extension, thanks for noticing More thought and testing is needed for this bit (as in, what happens with tools that have a |
||
|
||
let target_path = executable_directory.join(file_name); | ||
(name, source_path, target_path) | ||
}) | ||
.collect::<BTreeSet<_>>(); | ||
|
@@ -118,7 +129,7 @@ pub(crate) fn install_executables( | |
hint_executable_from_dependency(name, &site_packages, printer)?; | ||
|
||
// Clean up the environment we just created. | ||
installed_tools.remove_environment(name)?; | ||
installed_tools.remove_environment(&pkg)?; | ||
|
||
return Ok(ExitStatus::Failure); | ||
} | ||
|
@@ -138,7 +149,7 @@ pub(crate) fn install_executables( | |
} | ||
} else if existing_entry_points.peek().is_some() { | ||
// Clean up the environment we just created | ||
installed_tools.remove_environment(name)?; | ||
installed_tools.remove_environment(&pkg)?; | ||
|
||
let existing_entry_points = existing_entry_points | ||
// SAFETY: We know the target has a filename because we just constructed it above | ||
|
@@ -190,7 +201,7 @@ pub(crate) fn install_executables( | |
.map(|(name, _, target_path)| ToolEntrypoint::new(name, target_path)), | ||
options, | ||
); | ||
installed_tools.add_tool_receipt(name, tool)?; | ||
installed_tools.add_tool_receipt(&pkg, tool)?; | ||
|
||
// If the executable directory isn't on the user's PATH, warn. | ||
if !Shell::contains_path(&executable_directory) { | ||
|
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 may be a problematic structure for
uv tool upgrade
etc., e.g., if the user specifies a tool name via the command line we won't know what's the package name and what's a suffix until sometime later?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.
Agree, thanks for pointing it out