Skip to content
Draft
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
20 changes: 9 additions & 11 deletions crates/prek/src/cli/cache_gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ fn truncate_end(s: &str, max_chars: usize) -> String {
out
}

fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<String>) {
fn split_repo_dependency(deps: &[String]) -> (Option<String>, Vec<String>) {
// Best-effort: the remote repo dependency is typically `repo@rev`.
// Prefer URL-like values to avoid accidentally treating PEP508 deps as repo identifiers.
let mut repo_dep: Option<String> = None;
Expand All @@ -714,7 +714,6 @@ fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<Strin
}
}

rest.sort_unstable();
(repo_dep, rest)
}

Expand Down Expand Up @@ -757,29 +756,28 @@ mod tests {

#[test]
fn split_repo_dependency_prefers_url_like_repo_at_rev() {
let mut deps = FxHashSet::default();
deps.insert("requests==2.32.0".to_string());
deps.insert("black==24.1.0".to_string());
deps.insert("https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string());
let deps = vec![
"requests==2.32.0".to_string(),
"black==24.1.0".to_string(),
"https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string(),
];

let (repo_dep, rest) = split_repo_dependency(&deps);

assert_eq!(
repo_dep.as_deref(),
Some("https://github.com/pre-commit/pre-commit-hooks@v1.0.0")
);
assert_eq!(rest, vec!["black==24.1.0", "requests==2.32.0"]);
assert_eq!(rest, vec!["requests==2.32.0", "black==24.1.0"]);
}

#[test]
fn split_repo_dependency_returns_none_when_no_repo_like_dep() {
let mut deps = FxHashSet::default();
deps.insert("requests==2.32.0".to_string());
deps.insert("black==24.1.0".to_string());
let deps = vec!["requests==2.32.0".to_string(), "black==24.1.0".to_string()];

let (repo_dep, rest) = split_repo_dependency(&deps);
assert!(repo_dep.is_none());
assert_eq!(rest, vec!["black==24.1.0", "requests==2.32.0"]);
assert_eq!(rest, vec!["requests==2.32.0", "black==24.1.0"]);
}

#[test]
Expand Down
129 changes: 102 additions & 27 deletions crates/prek/src/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::sync::{Arc, OnceLock};
use anyhow::{Context, Result};
use clap::ValueEnum;
use prek_consts::PRE_COMMIT_HOOKS_YAML;
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
use rustc_hash::FxHashMap;
use serde::{Deserialize, Serialize};
use tempfile::TempDir;
use thiserror::Error;
Expand Down Expand Up @@ -330,11 +330,7 @@ impl HookBuilder {
let pass_filenames = options.pass_filenames.unwrap_or(true);
let require_serial = options.require_serial.unwrap_or(false);
let verbose = options.verbose.unwrap_or(false);
let additional_dependencies = options
.additional_dependencies
.unwrap_or_default()
.into_iter()
.collect::<FxHashSet<_>>();
let additional_dependencies = options.additional_dependencies.unwrap_or_default();

let language_request = LanguageRequest::parse(self.hook_spec.language, &language_version)
.map_err(|e| Error::Hook {
Expand Down Expand Up @@ -482,7 +478,7 @@ pub(crate) struct Hook {
project: Arc<Project>,
repo: Arc<Repo>,
// Cached computed dependencies.
dependencies: OnceLock<FxHashSet<String>>,
dependencies: OnceLock<Vec<String>>,

/// The index of the hook defined in the configuration file.
pub idx: usize,
Expand All @@ -496,7 +492,7 @@ pub(crate) struct Hook {
pub types: Vec<String>,
pub types_or: Vec<String>,
pub exclude_types: Vec<String>,
pub additional_dependencies: FxHashSet<String>,
pub additional_dependencies: Vec<String>,
pub args: Vec<String>,
pub env: FxHashMap<String, String>,
pub always_run: bool,
Expand Down Expand Up @@ -558,7 +554,7 @@ impl Hook {
///
/// For remote hooks, the repo URL is included to avoid reusing an environment created
/// from a different remote repository.
pub(crate) fn env_key_dependencies(&self) -> &FxHashSet<String> {
pub(crate) fn env_key_dependencies(&self) -> &[String] {
if !self.is_remote() {
return &self.additional_dependencies;
}
Expand Down Expand Up @@ -586,10 +582,11 @@ impl Hook {
///
/// For remote hooks, this includes the local path to the cloned repository so that
/// installers can install the hook's package/project itself.
pub(crate) fn install_dependencies(&self) -> Cow<'_, FxHashSet<String>> {
pub(crate) fn install_dependencies(&self) -> Cow<'_, [String]> {
if let Some(repo_path) = self.repo_path() {
let mut deps = self.additional_dependencies.clone();
deps.insert(repo_path.to_string_lossy().to_string());
let mut deps = Vec::with_capacity(self.additional_dependencies.len() + 1);
deps.push(repo_path.to_string_lossy().to_string());
deps.extend(self.additional_dependencies.iter().cloned());
Cow::Owned(deps)
} else {
Cow::Borrowed(&self.additional_dependencies)
Expand All @@ -600,7 +597,7 @@ impl Hook {
#[derive(Debug, Clone)]
pub(crate) struct HookEnvKey {
pub(crate) language: Language,
pub(crate) dependencies: FxHashSet<String>,
pub(crate) dependencies: Vec<String>,
pub(crate) language_request: LanguageRequest,
}

Expand All @@ -609,25 +606,24 @@ pub(crate) struct HookEnvKey {
#[derive(Debug, Clone, Copy)]
pub(crate) struct HookEnvKeyRef<'a> {
pub(crate) language: Language,
pub(crate) dependencies: &'a FxHashSet<String>,
pub(crate) dependencies: &'a [String],
pub(crate) language_request: &'a LanguageRequest,
}

/// Builds the dependency set used to identify a hook environment.
/// Builds the dependency list used to identify a hook environment.
///
/// For remote hooks, `remote_repo_dependency` is included so environments from different
/// repositories are not reused accidentally.
fn env_key_dependencies(
additional_dependencies: &FxHashSet<String>,
additional_dependencies: &[String],
remote_repo_dependency: Option<&str>,
) -> FxHashSet<String> {
let mut deps = FxHashSet::with_capacity_and_hasher(
) -> Vec<String> {
let mut deps = Vec::with_capacity(
additional_dependencies.len() + usize::from(remote_repo_dependency.is_some()),
FxBuildHasher,
);
deps.extend(additional_dependencies.iter().cloned());
if let Some(dep) = remote_repo_dependency {
deps.insert(dep.to_string());
deps.push(dep.to_string());
}
deps
}
Expand All @@ -636,7 +632,7 @@ fn env_key_dependencies(
/// environment described by [`InstallInfo`].
fn matches_install_info(
language: Language,
dependencies: &FxHashSet<String>,
dependencies: &[String],
language_request: &LanguageRequest,
info: &InstallInfo,
) -> bool {
Expand Down Expand Up @@ -674,11 +670,11 @@ impl HookEnvKey {
)
})?;

let additional_dependencies: FxHashSet<String> = hook_spec
let additional_dependencies: Vec<String> = hook_spec
.options
.additional_dependencies
.as_ref()
.map_or_else(FxHashSet::default, |deps| deps.iter().cloned().collect());
.clone()
.unwrap_or_default();

let dependencies = env_key_dependencies(&additional_dependencies, remote_repo_dependency);

Expand Down Expand Up @@ -786,7 +782,7 @@ impl InstalledHook {
pub(crate) struct InstallInfo {
pub(crate) language: Language,
pub(crate) language_version: semver::Version,
pub(crate) dependencies: FxHashSet<String>,
pub(crate) dependencies: Vec<String>,
pub(crate) env_path: PathBuf,
pub(crate) toolchain: PathBuf,
extra: FxHashMap<String, String>,
Expand All @@ -811,7 +807,7 @@ impl Clone for InstallInfo {
impl InstallInfo {
pub(crate) fn new(
language: Language,
dependencies: FxHashSet<String>,
dependencies: Vec<String>,
hooks_dir: &Path,
) -> Result<Self, Error> {
let env_path = tempfile::Builder::new()
Expand Down Expand Up @@ -1004,7 +1000,7 @@ mod tests {
],
types_or: [],
exclude_types: [],
additional_dependencies: {},
additional_dependencies: [],
args: [
"--flag",
],
Expand Down Expand Up @@ -1039,4 +1035,83 @@ mod tests {

Ok(())
}

#[tokio::test]
async fn hook_builder_preserves_additional_dependency_order_and_duplicates() -> Result<()> {
let temp = tempfile::tempdir()?;
let config_path = temp.path().join(PRE_COMMIT_CONFIG_YAML);
fs_err::write(&config_path, "repos: []\n")?;

let project = Arc::new(Project::from_config_file(
Cow::Borrowed(&config_path),
None,
)?);
let repo = Arc::new(Repo::Local { hooks: vec![] });

let expected = vec![
"--index-url".to_string(),
"https://pypi.org/simple".to_string(),
"pyecho-cli".to_string(),
"pyecho-cli".to_string(),
];

let hook_spec = HookSpec {
id: "ordered-deps".to_string(),
name: "ordered-deps".to_string(),
entry: "python -c 'print(1)'".to_string(),
language: Language::Python,
priority: None,
options: HookOptions {
additional_dependencies: Some(expected.clone()),
..Default::default()
},
};

let hook = HookBuilder::new(project.clone(), repo.clone(), hook_spec, 0)
.build()
.await?;
assert_eq!(hook.additional_dependencies, expected);

let hook_a = HookBuilder::new(
project.clone(),
repo.clone(),
HookSpec {
id: "a".to_string(),
name: "a".to_string(),
entry: "python -c 'print(1)'".to_string(),
language: Language::Python,
priority: None,
options: HookOptions {
additional_dependencies: Some(vec!["foo".to_string(), "bar".to_string()]),
..Default::default()
},
},
1,
)
.build()
.await?;

let hook_b = HookBuilder::new(
project,
repo,
HookSpec {
id: "b".to_string(),
name: "b".to_string(),
entry: "python -c 'print(1)'".to_string(),
language: Language::Python,
priority: None,
options: HookOptions {
additional_dependencies: Some(vec!["bar".to_string(), "foo".to_string()]),
..Default::default()
},
},
2,
)
.build()
.await?;

assert_ne!(hook_a.env_key_dependencies(), hook_b.env_key_dependencies());

Ok(())
}
}
2 changes: 1 addition & 1 deletion crates/prek/src/languages/bun/bun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl LanguageImpl for Bun {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
6 changes: 2 additions & 4 deletions crates/prek/src/languages/docker.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::collections::hash_map::DefaultHasher;
use std::fs;
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -323,8 +322,7 @@ impl Docker {

info.language.hash(&mut hasher);
info.language_version.hash(&mut hasher);
let deps = info.dependencies.iter().collect::<BTreeSet<&String>>();
deps.hash(&mut hasher);
info.dependencies.hash(&mut hasher);

let digest = hex::encode(hasher.finish().to_le_bytes());
format!("prek-{digest}")
Expand Down Expand Up @@ -436,7 +434,7 @@ impl LanguageImpl for Docker {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/golang/golang.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl LanguageImpl for Golang {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;
info.with_toolchain(go.bin().to_path_buf())
Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/haskell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl LanguageImpl for Haskell {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/julia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl LanguageImpl for Julia {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/lua.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl LanguageImpl for Lua {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/node/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl LanguageImpl for Node {

let mut info = InstallInfo::new(
hook.language,
hook.env_key_dependencies().clone(),
hook.env_key_dependencies().to_vec(),
&store.hooks_dir(),
)?;

Expand Down
4 changes: 1 addition & 3 deletions crates/prek/src/languages/node/version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ mod tests {
use super::{EXTRA_KEY_LTS, NodeRequest};
use crate::config::Language;
use crate::hook::InstallInfo;
use rustc_hash::FxHashSet;
use std::path::PathBuf;
use std::str::FromStr;

Expand Down Expand Up @@ -310,8 +309,7 @@ mod tests {
#[test]
fn test_node_request_satisfied_by() -> anyhow::Result<()> {
let temp_dir = tempfile::tempdir()?;
let mut install_info =
InstallInfo::new(Language::Node, FxHashSet::default(), temp_dir.path())?;
let mut install_info = InstallInfo::new(Language::Node, Vec::new(), temp_dir.path())?;
install_info
.with_language_version(semver::Version::new(12, 18, 3))
.with_toolchain(PathBuf::from("/usr/bin/node"))
Expand Down
Loading