Skip to content

[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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/uv-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3156,6 +3156,10 @@ pub struct ToolInstallArgs {
help_heading = "Python options"
)]
pub python: Option<String>,

/// The suffix to install the package and executables with
#[arg(long, help_heading = "Installer options")]
pub suffix: Option<String>,
}

#[derive(Args)]
Expand Down
46 changes: 31 additions & 15 deletions crates/uv-tool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Copy link
Member

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?

Copy link
Author

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

}

/// A collection of uv-managed tools installed on the current system.
#[derive(Debug, Clone)]
pub struct InstalledTools {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)),
Expand All @@ -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()
);

Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit curious why you did this instead of implementing Display for ToolName

Copy link
Author

Choose a reason for hiding this comment

The 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.
Of course, I complicated myself by not implementing Display, I blame my late little use of Rust, will change to that

Copy link
Author

Choose a reason for hiding this comment

The 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()
);

Expand All @@ -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))
Expand All @@ -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);
Expand All @@ -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()
);
}
Expand All @@ -246,7 +261,8 @@ impl InstalledTools {
}

debug!(
"Creating environment for tool `{name}`: {}",
"Creating environment for tool `{}`: {}",
name.name,
environment_path.user_display()
);

Expand All @@ -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())
}

Expand Down
31 changes: 21 additions & 10 deletions crates/uv/src/commands/tool/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle file extensions on Windows?

Copy link
Author

Choose a reason for hiding this comment

The 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 . in their name)


let target_path = executable_directory.join(file_name);
(name, source_path, target_path)
})
.collect::<BTreeSet<_>>();
Expand All @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
53 changes: 30 additions & 23 deletions crates/uv/src/commands/tool/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use uv_python::{
};
use uv_requirements::{RequirementsSource, RequirementsSpecification};
use uv_settings::{ResolverInstallerOptions, ToolOptions};
use uv_tool::InstalledTools;
use uv_tool::{InstalledTools, ToolName};
use uv_warnings::warn_user;

use crate::commands::pip::loggers::{DefaultInstallLogger, DefaultResolveLogger};
Expand All @@ -42,6 +42,7 @@ pub(crate) async fn install(
force: bool,
options: ResolverInstallerOptions,
settings: ResolverInstallerSettings,
suffix: Option<String>,
python_preference: PythonPreference,
python_downloads: PythonDownloads,
connectivity: Connectivity,
Expand Down Expand Up @@ -241,6 +242,11 @@ pub(crate) async fn install(
let installed_tools = InstalledTools::from_settings()?.init()?;
let _lock = installed_tools.lock().await?;

let pkg = ToolName {
name: from.name.clone(),
suffix: suffix.clone(),
};

// Find the existing receipt, if it exists. If the receipt is present but malformed, we'll
// remove the environment and continue with the install.
//
Expand All @@ -249,31 +255,31 @@ pub(crate) async fn install(
//
// (If we find existing entrypoints later on, and the tool _doesn't_ exist, we'll avoid removing
// the external tool's entrypoints (without `--force`).)
let (existing_tool_receipt, invalid_tool_receipt) =
match installed_tools.get_tool_receipt(&from.name) {
Ok(None) => (None, false),
Ok(Some(receipt)) => (Some(receipt), false),
Err(_) => {
// If the tool is not installed properly, remove the environment and continue.
match installed_tools.remove_environment(&from.name) {
Ok(()) => {
warn_user!(
"Removed existing `{from}` with invalid receipt",
from = from.name.cyan()
);
}
Err(uv_tool::Error::Io(err)) if err.kind() == std::io::ErrorKind::NotFound => {}
Err(err) => {
return Err(err.into());
}
let (existing_tool_receipt, invalid_tool_receipt) = match installed_tools.get_tool_receipt(&pkg)
{
Ok(None) => (None, false),
Ok(Some(receipt)) => (Some(receipt), false),
Err(_) => {
// If the tool is not installed properly, remove the environment and continue.
match installed_tools.remove_environment(&pkg) {
Ok(()) => {
warn_user!(
"Removed existing `{from}` with invalid receipt",
from = from.name.cyan()
);
}
Err(uv_tool::Error::Io(err)) if err.kind() == std::io::ErrorKind::NotFound => {}
Err(err) => {
return Err(err.into());
}
(None, true)
}
};
(None, true)
}
};

let existing_environment =
installed_tools
.get_environment(&from.name, &cache)?
.get_environment(&pkg, &cache)?
.filter(|environment| {
python_request.as_ref().map_or(true, |python_request| {
if python_request.satisfied(environment.interpreter(), &cache) {
Expand Down Expand Up @@ -308,7 +314,7 @@ pub(crate) async fn install(
if *tool_receipt.options() != options {
// ...but the options differ, we need to update the receipt.
installed_tools
.add_tool_receipt(&from.name, tool_receipt.clone().with_options(options))?;
.add_tool_receipt(&pkg, tool_receipt.clone().with_options(options))?;
}

// We're done, though we might need to update the receipt.
Expand Down Expand Up @@ -378,7 +384,7 @@ pub(crate) async fn install(
)
.await?;

let environment = installed_tools.create_environment(&from.name, interpreter)?;
let environment = installed_tools.create_environment(&pkg, interpreter)?;

// At this point, we removed any existing environment, so we should remove any of its
// executables.
Expand Down Expand Up @@ -411,5 +417,6 @@ pub(crate) async fn install(
python,
requirements,
printer,
&suffix,
)
}
10 changes: 7 additions & 3 deletions crates/uv/src/commands/tool/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use owo_colors::OwoColorize;

use uv_cache::Cache;
use uv_fs::Simplified;
use uv_tool::InstalledTools;
use uv_tool::{InstalledTools, ToolName};
use uv_warnings::warn_user;

use crate::commands::ExitStatus;
Expand Down Expand Up @@ -46,9 +46,13 @@ pub(crate) async fn list(
);
continue;
};
let pkg = ToolName {
name: name.clone(),
suffix: None, // TODO Add suffix option
};

// Output tool name and version
let version = match installed_tools.version(&name, cache) {
let version = match installed_tools.version(&pkg, cache) {
Ok(version) => version,
Err(e) => {
writeln!(printer.stderr(), "{e}")?;
Expand All @@ -74,7 +78,7 @@ pub(crate) async fn list(
printer.stdout(),
"{} ({})",
format!("{name} v{version}{version_specifier}").bold(),
installed_tools.tool_dir(&name).simplified_display().cyan(),
installed_tools.tool_dir(&pkg).simplified_display().cyan(),
)?;
} else {
writeln!(
Expand Down
8 changes: 7 additions & 1 deletion crates/uv/src/commands/tool/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use uv_python::{
PythonPreference, PythonRequest,
};
use uv_requirements::{RequirementsSource, RequirementsSpecification};
use uv_tool::ToolName;
use uv_tool::{entrypoint_paths, InstalledTools};
use uv_warnings::warn_user;

Expand Down Expand Up @@ -436,9 +437,14 @@ async fn get_or_create_environment(
let installed_tools = InstalledTools::from_settings()?.init()?;
let _lock = installed_tools.lock().await?;

let pkg = ToolName {
name: from.name.clone(),
suffix: None, // TODO add suffix support
};

let existing_environment =
installed_tools
.get_environment(&from.name, cache)?
.get_environment(&pkg, cache)?
.filter(|environment| {
python_request.as_ref().map_or(true, |python_request| {
python_request.satisfied(environment.interpreter(), cache)
Expand Down
Loading
Loading