Accept package names in the tool API (#4737)

## Summary

It seems helpful that these _not_ accept arbitrary strings.
This commit is contained in:
Charlie Marsh 2024-07-02 15:30:40 -04:00 committed by GitHub
parent c8987269ff
commit 21187e1f36
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 33 additions and 24 deletions

View file

@ -2014,7 +2014,8 @@ pub struct ToolListArgs;
#[derive(Args)] #[derive(Args)]
#[allow(clippy::struct_excessive_bools)] #[allow(clippy::struct_excessive_bools)]
pub struct ToolUninstallArgs { pub struct ToolUninstallArgs {
pub name: String, /// The name of the tool to uninstall.
pub name: PackageName,
} }
#[derive(Args)] #[derive(Args)]

View file

@ -96,8 +96,8 @@ impl InstalledTools {
} }
/// Get the receipt for the given tool. /// Get the receipt for the given tool.
pub fn get_tool_receipt(&self, name: &str) -> Result<Option<Tool>, Error> { pub fn get_tool_receipt(&self, name: &PackageName) -> Result<Option<Tool>, Error> {
let path = self.root.join(name).join("uv-receipt.toml"); let path = self.root.join(name.to_string()).join("uv-receipt.toml");
match ToolReceipt::from_path(&path) { match ToolReceipt::from_path(&path) {
Ok(tool_receipt) => Ok(Some(tool_receipt.tool)), Ok(tool_receipt) => Ok(Some(tool_receipt.tool)),
Err(Error::IO(err)) if err.kind() == io::ErrorKind::NotFound => Ok(None), Err(Error::IO(err)) if err.kind() == io::ErrorKind::NotFound => Ok(None),
@ -114,8 +114,8 @@ impl InstalledTools {
} }
/// Lock a tool directory. /// Lock a tool directory.
fn acquire_tool_lock(&self, name: &str) -> Result<LockedFile, Error> { fn acquire_tool_lock(&self, name: &PackageName) -> Result<LockedFile, Error> {
let path = self.root.join(name); let path = self.root.join(name.to_string());
Ok(LockedFile::acquire( Ok(LockedFile::acquire(
path.join(".lock"), path.join(".lock"),
path.user_display(), path.user_display(),
@ -125,11 +125,11 @@ impl InstalledTools {
/// Add a receipt for a tool. /// Add a receipt for a tool.
/// ///
/// Any existing receipt will be replaced. /// Any existing receipt will be replaced.
pub fn add_tool_receipt(&self, name: &str, tool: Tool) -> Result<(), Error> { pub fn add_tool_receipt(&self, name: &PackageName, tool: Tool) -> Result<(), Error> {
let _lock = self.acquire_tool_lock(name); let _lock = self.acquire_tool_lock(name);
let tool_receipt = ToolReceipt::from(tool); let tool_receipt = ToolReceipt::from(tool);
let path = self.root.join(name).join("uv-receipt.toml"); let path = self.root.join(name.to_string()).join("uv-receipt.toml");
debug!( debug!(
"Adding metadata entry for tool `{name}` at {}", "Adding metadata entry for tool `{name}` at {}",
@ -147,9 +147,9 @@ impl InstalledTools {
/// Remove the environment for a tool. /// Remove the environment for a tool.
/// ///
/// Does not remove the tool's entrypoints. /// Does not remove the tool's entrypoints.
pub fn remove_environment(&self, name: &str) -> Result<(), Error> { pub fn remove_environment(&self, name: &PackageName) -> Result<(), Error> {
let _lock = self.acquire_lock(); let _lock = self.acquire_lock();
let environment_path = self.root.join(name); let environment_path = self.root.join(name.to_string());
debug!( debug!(
"Deleting environment for tool `{name}` at {}", "Deleting environment for tool `{name}` at {}",
@ -161,15 +161,16 @@ impl InstalledTools {
Ok(()) Ok(())
} }
/// Return the [`PythonEnvironment`] for a given tool.
pub fn environment( pub fn environment(
&self, &self,
name: &str, name: &PackageName,
remove_existing: bool, remove_existing: bool,
interpreter: Interpreter, interpreter: Interpreter,
cache: &Cache, cache: &Cache,
) -> Result<PythonEnvironment, Error> { ) -> Result<PythonEnvironment, Error> {
let _lock = self.acquire_lock(); let _lock = self.acquire_lock();
let environment_path = self.root.join(name); let environment_path = self.root.join(name.to_string());
if !remove_existing && environment_path.exists() { if !remove_existing && environment_path.exists() {
debug!( debug!(

View file

@ -122,11 +122,9 @@ pub(crate) async fn install(
.unwrap() .unwrap()
}; };
let name = from.name.to_string();
let installed_tools = InstalledTools::from_settings()?; let installed_tools = InstalledTools::from_settings()?;
let existing_tool_receipt = installed_tools.get_tool_receipt(&name)?; let existing_tool_receipt = installed_tools.get_tool_receipt(&from.name)?;
// TODO(zanieb): Automatically replace an existing tool if the request differs // TODO(zanieb): Automatically replace an existing tool if the request differs
let reinstall_entry_points = if existing_tool_receipt.is_some() { let reinstall_entry_points = if existing_tool_receipt.is_some() {
if force { if force {
@ -142,7 +140,11 @@ pub(crate) async fn install(
Reinstall::Packages(ref packages) => packages.contains(&from.name), Reinstall::Packages(ref packages) => packages.contains(&from.name),
// If not reinstalling... then we're done // If not reinstalling... then we're done
Reinstall::None => { Reinstall::None => {
writeln!(printer.stderr(), "Tool `{name}` is already installed")?; writeln!(
printer.stderr(),
"Tool `{}` is already installed",
from.name
)?;
return Ok(ExitStatus::Failure); return Ok(ExitStatus::Failure);
} }
} }
@ -176,7 +178,7 @@ pub(crate) async fn install(
// TODO(zanieb): Build the environment in the cache directory then copy into the tool directory // TODO(zanieb): Build the environment in the cache directory then copy into the tool directory
// This lets us confirm the environment is valid before removing an existing install // This lets us confirm the environment is valid before removing an existing install
let environment = installed_tools.environment( let environment = installed_tools.environment(
&name, &from.name,
// Do not remove the existing environment if we're reinstalling a subset of packages // Do not remove the existing environment if we're reinstalling a subset of packages
!matches!(settings.reinstall, Reinstall::Packages(_)), !matches!(settings.reinstall, Reinstall::Packages(_)),
interpreter, interpreter,
@ -208,7 +210,11 @@ pub(crate) async fn install(
// Exit early if we're not supposed to be reinstalling entry points // Exit early if we're not supposed to be reinstalling entry points
// e.g. `--reinstall-package` was used for some dependency // e.g. `--reinstall-package` was used for some dependency
if existing_tool_receipt.is_some() && !reinstall_entry_points { if existing_tool_receipt.is_some() && !reinstall_entry_points {
writeln!(printer.stderr(), "Updated environment for tool `{name}`")?; writeln!(
printer.stderr(),
"Updated environment for tool `{}`",
from.name
)?;
return Ok(ExitStatus::Success); return Ok(ExitStatus::Success);
} }
@ -246,9 +252,9 @@ pub(crate) async fn install(
if target_entry_points.is_empty() { if target_entry_points.is_empty() {
// Clean up the environment we just created // Clean up the environment we just created
installed_tools.remove_environment(&name)?; installed_tools.remove_environment(&from.name)?;
bail!("No entry points found for tool `{name}`"); bail!("No entry points found for tool `{}`", from.name);
} }
// Check if they exist, before installing // Check if they exist, before installing
@ -266,7 +272,7 @@ pub(crate) async fn install(
} }
} else if existing_entry_points.peek().is_some() { } else if existing_entry_points.peek().is_some() {
// Clean up the environment we just created // Clean up the environment we just created
installed_tools.remove_environment(&name)?; installed_tools.remove_environment(&from.name)?;
let existing_entry_points = existing_entry_points let existing_entry_points = existing_entry_points
// SAFETY: We know the target has a filename because we just constructed it above // SAFETY: We know the target has a filename because we just constructed it above
@ -300,7 +306,7 @@ pub(crate) async fn install(
.join(", ") .join(", ")
)?; )?;
debug!("Adding receipt for tool `{name}`"); debug!("Adding receipt for tool `{}`", from.name);
let installed_tools = installed_tools.init()?; let installed_tools = installed_tools.init()?;
let tool = Tool::new( let tool = Tool::new(
requirements requirements
@ -312,7 +318,7 @@ pub(crate) async fn install(
.into_iter() .into_iter()
.map(|(name, _, target_path)| ToolEntrypoint::new(name, target_path)), .map(|(name, _, target_path)| ToolEntrypoint::new(name, target_path)),
); );
installed_tools.add_tool_receipt(&name, tool)?; installed_tools.add_tool_receipt(&from.name, tool)?;
Ok(ExitStatus::Success) Ok(ExitStatus::Success)
} }

View file

@ -6,6 +6,7 @@ use itertools::Itertools;
use tracing::debug; use tracing::debug;
use uv_configuration::PreviewMode; use uv_configuration::PreviewMode;
use uv_fs::Simplified; use uv_fs::Simplified;
use uv_normalize::PackageName;
use uv_tool::InstalledTools; use uv_tool::InstalledTools;
use uv_warnings::warn_user_once; use uv_warnings::warn_user_once;
@ -14,7 +15,7 @@ use crate::printer::Printer;
/// Uninstall a tool. /// Uninstall a tool.
pub(crate) async fn uninstall( pub(crate) async fn uninstall(
name: String, name: PackageName,
preview: PreviewMode, preview: PreviewMode,
printer: Printer, printer: Printer,
) -> Result<ExitStatus> { ) -> Result<ExitStatus> {

View file

@ -300,7 +300,7 @@ impl ToolListSettings {
#[allow(clippy::struct_excessive_bools)] #[allow(clippy::struct_excessive_bools)]
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
pub(crate) struct ToolUninstallSettings { pub(crate) struct ToolUninstallSettings {
pub(crate) name: String, pub(crate) name: PackageName,
} }
impl ToolUninstallSettings { impl ToolUninstallSettings {