Skip to content
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

Eliminate dependencies on directores and dirs-sys #8048

Merged
merged 4 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
46 changes: 3 additions & 43 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ csv = { version = "1.3.0" }
ctrlc = { version = "3.4.5" }
dashmap = { version = "6.1.0" }
data-encoding = { version = "2.6.0" }
directories = { version = "5.0.1" }
dirs-sys = { version = "0.4.1" }
dunce = { version = "1.0.5" }
either = { version = "1.13.0" }
encoding_rs_io = { version = "0.1.7" }
Expand Down
1 change: 0 additions & 1 deletion crates/uv-cache/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ uv-pypi-types = { workspace = true }
uv-static = { workspace = true }

clap = { workspace = true, features = ["derive", "env"], optional = true }
directories = { workspace = true }
etcetera = { workspace = true }
fs-err = { workspace = true, features = ["tokio"] }
nanoid = { workspace = true }
Expand Down
19 changes: 14 additions & 5 deletions crates/uv-cache/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use uv_static::EnvVars;

use crate::Cache;
use clap::Parser;
use directories::ProjectDirs;
use etcetera::BaseStrategy;
use tracing::{debug, warn};

Expand All @@ -31,6 +30,19 @@ pub struct CacheArgs {
pub cache_dir: Option<PathBuf>,
}

fn legacy_cache_dir() -> Option<PathBuf> {
etcetera::base_strategy::choose_native_strategy()
.ok()
.map(|dirs| dirs.cache_dir().join("uv"))
.map(|dir| {
if cfg!(windows) {
dir.join("cache")
} else {
dir
}
})
}

impl Cache {
/// Prefer, in order:
///
Expand All @@ -45,10 +57,7 @@ impl Cache {
Self::temp()
} else if let Some(cache_dir) = cache_dir {
Ok(Self::from_path(cache_dir))
} else if let Some(cache_dir) = ProjectDirs::from("", "", "uv")
.map(|dirs| dirs.cache_dir().to_path_buf())
.filter(|dir| dir.exists())
{
} else if let Some(cache_dir) = legacy_cache_dir().filter(|dir| dir.exists()) {
// If the user has an existing directory at (e.g.) `/Users/user/Library/Caches/uv`,
// respect it for backwards compatibility. Otherwise, prefer the XDG strategy, even on
// macOS.
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-settings/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ uv-static = { workspace = true }
uv-warnings = { workspace = true }

clap = { workspace = true }
dirs-sys = { workspace = true }
etcetera = { workspace = true }
fs-err = { workspace = true }
schemars = { workspace = true, optional = true }
serde = { workspace = true }
Expand Down
24 changes: 6 additions & 18 deletions crates/uv-settings/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use std::ops::Deref;
use std::path::{Path, PathBuf};

use etcetera::BaseStrategy;
use tracing::debug;

use uv_fs::Simplified;
#[cfg(not(windows))]
use uv_static::EnvVars;
use uv_warnings::warn_user;

pub use crate::combine::*;
Expand Down Expand Up @@ -168,23 +167,12 @@ impl From<Options> for FilesystemOptions {

/// Returns the path to the user configuration directory.
///
/// This is similar to the `config_dir()` returned by the `dirs` crate, but it uses the
/// `XDG_CONFIG_HOME` environment variable on both Linux _and_ macOS, rather than the
/// `Application Support` directory on macOS.
/// On Windows, use, e.g., C:\Users\Alice\AppData\Roaming
/// On Linux and macOS, use `XDG_CONFIG_HOME` or $HOME/.config, e.g., /home/alice/.config.
fn config_dir() -> Option<PathBuf> {
// On Windows, use, e.g., C:\Users\Alice\AppData\Roaming
#[cfg(windows)]
{
dirs_sys::known_folder_roaming_app_data()
}

// On Linux and macOS, use, e.g., /home/alice/.config.
#[cfg(not(windows))]
{
std::env::var_os(EnvVars::XDG_CONFIG_HOME)
.and_then(dirs_sys::is_absolute_path)
.or_else(|| dirs_sys::home_dir().map(|path| path.join(".config")))
}
etcetera::choose_base_strategy()
Copy link
Member

Choose a reason for hiding this comment

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

I think this isn't quite the same. It looks like etcetera will use local AppData and then falls back to Roaming AppData, whereas the above always uses the Roaming version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dirs-sys uses the KnownFolderID FOLDERID_RoamingAppData, while etcetera uses the CSIDL CSIDL_APPDATA. And FOLDERID_RoamingAppData is equivalent to CSIDL_APPDATA in CSIDL.

If the SHGetFolderPathW call fails, etcetera will fall back to use self.home_dir.join("AppData").join("Roaming"). So it seems etcetera always uses the Roaming APPDATA too, they should be the same.

.map(|dirs| dirs.config_dir())
.ok()
}

/// Load [`Options`] from a `uv.toml` file.
Expand Down
1 change: 0 additions & 1 deletion crates/uv-state/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ doctest = false
workspace = true

[dependencies]
directories = { workspace = true }
etcetera = { workspace = true }
tempfile = { workspace = true }
fs-err = { workspace = true }
13 changes: 8 additions & 5 deletions crates/uv-state/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use std::{
sync::Arc,
};

use directories::ProjectDirs;
use etcetera::BaseStrategy;
use fs_err as fs;
use tempfile::{tempdir, TempDir};
Expand Down Expand Up @@ -85,10 +84,7 @@ impl StateStore {
pub fn from_settings(state_dir: Option<PathBuf>) -> Result<Self, io::Error> {
if let Some(state_dir) = state_dir {
StateStore::from_path(state_dir)
} else if let Some(data_dir) = ProjectDirs::from("", "", "uv")
.map(|dirs| dirs.data_dir().to_path_buf())
.filter(|dir| dir.exists())
{
} else if let Some(data_dir) = legacy_data_dir().filter(|dir| dir.exists()) {
// If the user has an existing directory at (e.g.) `/Users/user/Library/Application Support/uv`,
// respect it for backwards compatibility. Otherwise, prefer the XDG strategy, even on
// macOS.
Expand All @@ -104,6 +100,13 @@ impl StateStore {
}
}

fn legacy_data_dir() -> Option<PathBuf> {
etcetera::base_strategy::choose_native_strategy()
.ok()
.map(|dirs| dirs.data_dir().join("uv"))
.map(|dir| if cfg!(windows) { dir.join("data") } else { dir })
}

/// The different kinds of data in the state store are stored in different bucket, which in our case
/// are subdirectories of the state store root.
#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-tool/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ uv-state = { workspace = true }
uv-static = { workspace = true }
uv-virtualenv = { workspace = true }

dirs-sys = { workspace = true }
etcetera = { workspace = true }
fs-err = { workspace = true }
pathdiff = { workspace = true }
serde = { workspace = true }
Expand Down
25 changes: 15 additions & 10 deletions crates/uv-tool/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use core::fmt;

use fs_err as fs;
use std::ffi::OsString;

use uv_pep440::Version;
use uv_pep508::{InvalidNameError, PackageName};
Expand Down Expand Up @@ -354,6 +354,15 @@ impl fmt::Display for InstalledTool {
}
}

fn is_absolute_path(path: OsString) -> Option<PathBuf> {
let path = PathBuf::from(path);
if path.is_absolute() {
Some(path)
} else {
None
}
}

/// Find a directory to place executables in.
///
/// This follows, in order:
Expand All @@ -368,20 +377,16 @@ impl fmt::Display for InstalledTool {
/// Errors if a directory cannot be found.
pub fn find_executable_directory() -> Result<PathBuf, Error> {
std::env::var_os(EnvVars::UV_TOOL_BIN_DIR)
.and_then(dirs_sys::is_absolute_path)
.or_else(|| std::env::var_os(EnvVars::XDG_BIN_HOME).and_then(dirs_sys::is_absolute_path))
.and_then(is_absolute_path)
.or_else(|| std::env::var_os(EnvVars::XDG_BIN_HOME).and_then(is_absolute_path))
.or_else(|| {
std::env::var_os(EnvVars::XDG_DATA_HOME)
.and_then(dirs_sys::is_absolute_path)
.and_then(is_absolute_path)
.map(|path| path.join("../bin"))
})
.or_else(|| {
// See https://github.com/dirs-dev/dirs-rs/blob/50b50f31f3363b7656e5e63b3fa1060217cbc844/src/win.rs#L5C58-L5C78
#[cfg(windows)]
let home_dir = dirs_sys::known_folder_profile();
#[cfg(not(windows))]
let home_dir = dirs_sys::home_dir();
home_dir.map(|path| path.join(".local").join("bin"))
let home_dir = etcetera::home_dir();
home_dir.map(|path| path.join(".local").join("bin")).ok()
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be mostly the same except that it defers to USERPROFILE first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's unfortunate. What's your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we already depend on homedirs via axoupdater... So in theory we could call homedirs::my_home here at least on Windows, which does seem to use FOLDERID_Profile. Or we could just inline our own home_dir impl for Windows here, and add a TODO to move to etcetera on the next breaking release (0.5.0). Or we could just ship this whole thing in 0.5.0.

Copy link
Member

Choose a reason for hiding this comment

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

@zanieb -- I think my preference here is just to ship this as part of 0.5.0 in the near-ish future, and accept that we now respect non-empty USERPROFILE on Windows (which seems fine).

})
.ok_or(Error::NoExecutableDirectory)
}
Expand Down