Lock for the duration of tool commands (#4720)

Feedback from
https://github.com/astral-sh/uv/pull/4501#discussion_r1655391958
This commit is contained in:
Zanie Blue 2024-07-10 12:16:13 -04:00 committed by GitHub
parent 067b3ee666
commit f3c5d26417
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 37 additions and 20 deletions

View file

@ -86,8 +86,9 @@ impl InstalledTools {
} }
/// Return the metadata for all installed tools. /// Return the metadata for all installed tools.
///
/// Note it is generally incorrect to use this without [`Self::acquire_lock`].
pub fn tools(&self) -> Result<Vec<(PackageName, Tool)>, Error> { pub fn tools(&self) -> Result<Vec<(PackageName, Tool)>, Error> {
let _lock = self.acquire_lock();
let mut tools = Vec::new(); let mut tools = Vec::new();
for directory in uv_fs::directories(self.root()) { for directory in uv_fs::directories(self.root()) {
let name = directory.file_name().unwrap().to_string_lossy().to_string(); let name = directory.file_name().unwrap().to_string_lossy().to_string();
@ -109,6 +110,8 @@ impl InstalledTools {
} }
/// Get the receipt for the given tool. /// Get the receipt for the given tool.
///
/// 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: &PackageName) -> Result<Option<Tool>, Error> {
let path = self.root.join(name.to_string()).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) {
@ -119,28 +122,19 @@ impl InstalledTools {
} }
/// Lock the tools directory. /// Lock the tools directory.
fn acquire_lock(&self) -> Result<LockedFile, Error> { pub fn acquire_lock(&self) -> Result<LockedFile, Error> {
Ok(LockedFile::acquire( Ok(LockedFile::acquire(
self.root.join(".lock"), self.root.join(".lock"),
self.root.user_display(), self.root.user_display(),
)?) )?)
} }
/// Lock a tool directory.
fn acquire_tool_lock(&self, name: &PackageName) -> Result<LockedFile, Error> {
let path = self.root.join(name.to_string());
Ok(LockedFile::acquire(
path.join(".lock"),
path.user_display(),
)?)
}
/// Add a receipt for a tool. /// Add a receipt for a tool.
/// ///
/// Any existing receipt will be replaced. /// 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: &PackageName, tool: Tool) -> Result<(), Error> {
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.to_string()).join("uv-receipt.toml"); let path = self.root.join(name.to_string()).join("uv-receipt.toml");
@ -161,11 +155,12 @@ impl InstalledTools {
/// ///
/// Does not remove the tool's entrypoints. /// Does not remove the tool's entrypoints.
/// ///
/// Note it is generally incorrect to use this without [`Self::acquire_lock`].
///
/// # Errors /// # Errors
/// ///
/// If no such environment exists for the tool. /// If no such environment exists for the tool.
pub fn remove_environment(&self, name: &PackageName) -> Result<(), Error> { pub fn remove_environment(&self, name: &PackageName) -> Result<(), Error> {
let _lock = self.acquire_lock();
let environment_path = self.root.join(name.to_string()); let environment_path = self.root.join(name.to_string());
debug!( debug!(
@ -182,12 +177,13 @@ impl InstalledTools {
/// ///
/// Returns `Ok(None)` if the environment does not exist or is linked to a non-existent /// Returns `Ok(None)` if the environment does not exist or is linked to a non-existent
/// interpreter. /// interpreter.
///
/// Note it is generally incorrect to use this without [`Self::acquire_lock`].
pub fn get_environment( pub fn get_environment(
&self, &self,
name: &PackageName, name: &PackageName,
cache: &Cache, cache: &Cache,
) -> Result<Option<PythonEnvironment>, Error> { ) -> Result<Option<PythonEnvironment>, Error> {
let _lock = self.acquire_lock();
let environment_path = self.root.join(name.to_string()); let environment_path = self.root.join(name.to_string());
match PythonEnvironment::from_root(&environment_path, cache) { match PythonEnvironment::from_root(&environment_path, cache) {
@ -213,12 +209,13 @@ impl InstalledTools {
} }
/// Create the [`PythonEnvironment`] for a given tool, removing any existing environments. /// Create the [`PythonEnvironment`] for a given tool, removing any existing environments.
///
/// Note it is generally incorrect to use this without [`Self::acquire_lock`].
pub fn create_environment( pub fn create_environment(
&self, &self,
name: &PackageName, name: &PackageName,
interpreter: Interpreter, interpreter: Interpreter,
) -> Result<PythonEnvironment, Error> { ) -> Result<PythonEnvironment, Error> {
let _lock = self.acquire_lock();
let environment_path = self.root.join(name.to_string()); let environment_path = self.root.join(name.to_string());
// Remove any existing environment. // Remove any existing environment.

View file

@ -154,7 +154,8 @@ pub(crate) async fn install(
requirements requirements
}; };
let installed_tools = InstalledTools::from_settings()?; let installed_tools = InstalledTools::from_settings()?.init()?;
let _lock = installed_tools.acquire_lock()?;
let existing_tool_receipt = installed_tools.get_tool_receipt(&from.name)?; let existing_tool_receipt = installed_tools.get_tool_receipt(&from.name)?;
let existing_environment = let existing_environment =
installed_tools installed_tools
@ -361,7 +362,6 @@ pub(crate) async fn install(
)?; )?;
debug!("Adding receipt for tool `{}`", from.name); debug!("Adding receipt for tool `{}`", from.name);
let installed_tools = installed_tools.init()?;
let tool = Tool::new( let tool = Tool::new(
requirements requirements
.into_iter() .into_iter()

View file

@ -22,6 +22,14 @@ pub(crate) async fn list(
} }
let installed_tools = InstalledTools::from_settings()?; let installed_tools = InstalledTools::from_settings()?;
let _lock = match installed_tools.acquire_lock() {
Ok(lock) => lock,
Err(uv_tool::Error::IO(err)) if err.kind() == std::io::ErrorKind::NotFound => {
writeln!(printer.stderr(), "No tools installed")?;
return Ok(ExitStatus::Success);
}
Err(err) => return Err(err.into()),
};
let mut tools = installed_tools.tools()?.into_iter().collect::<Vec<_>>(); let mut tools = installed_tools.tools()?.into_iter().collect::<Vec<_>>();
tools.sort_by_key(|(name, _)| name.clone()); tools.sort_by_key(|(name, _)| name.clone());

View file

@ -218,7 +218,8 @@ async fn get_or_create_environment(
// Check if the tool is already installed in a compatible environment. // Check if the tool is already installed in a compatible environment.
if !isolated { if !isolated {
let installed_tools = InstalledTools::from_settings()?; let installed_tools = InstalledTools::from_settings()?.init()?;
let _lock = installed_tools.acquire_lock()?;
let existing_environment = let existing_environment =
installed_tools installed_tools

View file

@ -24,7 +24,18 @@ pub(crate) async fn uninstall(
warn_user_once!("`uv tool uninstall` is experimental and may change without warning."); warn_user_once!("`uv tool uninstall` is experimental and may change without warning.");
} }
let installed_tools = InstalledTools::from_settings()?; let installed_tools = InstalledTools::from_settings()?.init()?;
let _lock = match installed_tools.acquire_lock() {
Ok(lock) => lock,
Err(uv_tool::Error::IO(err)) if err.kind() == std::io::ErrorKind::NotFound => {
if let Some(name) = name {
bail!("`{name}` is not installed");
}
writeln!(printer.stderr(), "Nothing to uninstall")?;
return Ok(ExitStatus::Success);
}
Err(err) => return Err(err.into()),
};
let mut entrypoints = if let Some(name) = name { let mut entrypoints = if let Some(name) = name {
let Some(receipt) = installed_tools.get_tool_receipt(&name)? else { let Some(receipt) = installed_tools.get_tool_receipt(&name)? else {