Skip to content
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
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ toml_edit = { version = "0.25.1" }
tracing = { version = "0.1.40" }
tracing-subscriber = { version = "0.3.20", features = ["env-filter"] }
unicode-width = { version = "0.2.0", default-features = false }
url = { version = "2.5.8" }
walkdir = { version = "2.5.0" }
webpki-root-certs = { version = "1.0.6" }
which = { version = "8.0.0" }
Expand Down
1 change: 1 addition & 0 deletions crates/prek/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ toml_edit = { workspace = true }
tracing = { workspace = true }
tracing-subscriber = { workspace = true }
unicode-width = { workspace = true }
url = { workspace = true }
walkdir = { workspace = true }
webpki-root-certs = { workspace = true }
which = { workspace = true }
Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/cli/try_repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ pub(crate) async fn try_repo(
let store = Store::from_settings()?;
let tmp_dir = TempDir::with_prefix_in("try-repo-", store.scratch_path())?;

let store = Store::from_path(tmp_dir.path()).init()?;
let store = Store::from_path(tmp_dir.path())?.init()?;
let selectors = Selectors::load(&run_args.includes, &run_args.skips, GIT_ROOT.as_ref()?)?;

let predefined_hooks = match repo.as_str() {
Expand Down
23 changes: 0 additions & 23 deletions crates/prek/src/hook.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::fmt::{Display, Formatter};
use std::ops::Deref;
Expand Down Expand Up @@ -596,21 +595,6 @@ impl Hook {
language_request: &self.language_request,
})
}

/// Dependencies to pass to language dependency installers.
///
/// 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<'_, [String]> {
if let Some(repo_path) = self.repo_path() {
let mut deps = Vec::with_capacity(self.additional_dependencies.len() + 1);
deps.push(repo_path.to_string_lossy().into_owned());
deps.extend(self.additional_dependencies.iter().cloned());
Cow::Owned(deps)
} else {
Cow::Borrowed(&self.additional_dependencies)
}
}
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -1254,18 +1238,11 @@ mod tests {
let hook = HookBuilder::new(project, repo, hook_spec, 0)
.build()
.await?;
let install_dependencies = hook.install_dependencies();
let mut expected_install_dependencies = vec![repo_path.to_string_lossy().into_owned()];
expected_install_dependencies.extend(additional_dependencies.iter().cloned());
let requirement = hook
.environment_requirement()
.expect("Python hook installs an environment");

assert_eq!(hook.additional_dependencies, additional_dependencies);
assert_eq!(
install_dependencies.as_ref(),
expected_install_dependencies.as_slice()
);
assert_eq!(requirement.repo, Some(repo_identity));
assert_eq!(requirement.dependencies, additional_dependencies);

Expand Down
10 changes: 5 additions & 5 deletions crates/prek/src/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ YyRIHN8wfdVoOw==
#[tokio::test]
async fn downloads_file_without_checksum() -> Result<()> {
let temp = tempfile::tempdir()?;
let store = Store::from_path(temp.path()).init()?;
let store = Store::from_path(temp.path())?.init()?;
let (url, server) = serve_once(b"data").await?;

let (download, _computed_digest) =
Expand All @@ -448,7 +448,7 @@ YyRIHN8wfdVoOw==
#[tokio::test]
async fn download_artifact_keeps_plain_file() -> Result<()> {
let temp = tempfile::tempdir()?;
let store = Store::from_path(temp.path()).init()?;
let store = Store::from_path(temp.path())?.init()?;
let (url, server) = serve_once(b"data").await?;

let download = super::download_artifact(&url, "rustup-init", &store, async || {
Expand All @@ -468,7 +468,7 @@ YyRIHN8wfdVoOw==
#[tokio::test]
async fn download_artifact_rejects_mismatched_checksum() -> Result<()> {
let temp = tempfile::tempdir()?;
let store = Store::from_path(temp.path()).init()?;
let store = Store::from_path(temp.path())?.init()?;
let (url, server) = serve_once(b"data").await?;

let result = super::download_artifact(&url, "archive.tar.gz", &store, async || {
Expand All @@ -487,7 +487,7 @@ YyRIHN8wfdVoOw==
#[tokio::test]
async fn downloads_file_and_computes_checksum() -> Result<()> {
let temp = tempfile::tempdir()?;
let store = Store::from_path(temp.path()).init()?;
let store = Store::from_path(temp.path())?.init()?;
let (url, server) = serve_once(b"data").await?;

let (download, sha256_digest) =
Expand All @@ -502,7 +502,7 @@ YyRIHN8wfdVoOw==
#[tokio::test]
async fn downloads_chunked_file_and_computes_checksum() -> Result<()> {
let temp = tempfile::tempdir()?;
let store = Store::from_path(temp.path()).init()?;
let store = Store::from_path(temp.path())?.init()?;
let (url, server) = serve_chunked(&[b"da", b"ta"]).await?;

let (download, sha256_digest) =
Expand Down
14 changes: 12 additions & 2 deletions crates/prek/src/languages/bun/bun.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::ffi::OsStr;
use std::path::Path;
use std::process::Stdio;
use std::sync::Arc;
Expand Down Expand Up @@ -66,7 +67,16 @@ impl LanguageBackend for Bun {
fs_err::tokio::create_dir_all(&lib_dir).await?;

// 3. Install dependencies
let deps = hook.install_dependencies();
let mut deps: Vec<&OsStr> = Vec::with_capacity(hook.additional_dependencies.len() + 1);
if let Some(repo_path) = hook.repo_path() {
deps.push(repo_path.as_os_str());
}
deps.extend(
hook.additional_dependencies
.iter()
.map(|dependency| OsStr::new(dependency.as_str())),
);

if deps.is_empty() {
debug!("No dependencies to install");
} else {
Expand All @@ -79,7 +89,7 @@ impl LanguageBackend for Bun {
Cmd::new(bun.bun())
.arg("install")
.arg("-g")
.args(&*deps)
.args(deps)
.env(EnvVars::PATH, new_path)
.env(EnvVars::BUN_INSTALL, &info.env_path)
.check(true)
Expand Down
128 changes: 115 additions & 13 deletions crates/prek/src/languages/node/node.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use std::path::Path;
use std::process::Stdio;
use std::str;
use std::sync::Arc;

use anyhow::{Context, Result};
use anyhow::{Context, Result, anyhow};
use prek_consts::env_vars::EnvVars;
use prek_consts::prepend_paths;
use semver::Version;
use tracing::debug;
use url::Url;

use crate::cli::reporter::HookInstallReporter;
use crate::cli::run::HookRunReporter;
Expand Down Expand Up @@ -85,18 +88,85 @@ impl LanguageBackend for Node {
fs_err::tokio::create_dir_all(&lib_dir).await?;

// 3. Install dependencies
let deps = hook.install_dependencies();
let (deps, includes_git_hook_repo) = node_install_dependencies(&hook)?;
if deps.is_empty() {
debug!("No dependencies to install");
} else {
// npm install <folder>:
// If <folder> sits inside the root of your project, its dependencies will be installed
// and may be hoisted to the top-level node_modules as they would for other types of dependencies.
// If <folder> sits outside the root of your project, npm will not install the package dependencies
// in the directory <folder>, but it will create a symlink to <folder>.
// Why remote hook repositories are installed as `git+file://` rather than as folders
// ------------------------------------------------------------------------------------
//
// NOTE: If you want to install the content of a directory like a package from the registry
// instead of creating a link, you would need to use the --install-links option.
// npm delegates package acquisition to `pacote`. The type of the package spec selects
// a fetcher, and folder and Git specs have importantly different preparation semantics:
//
// * `<folder>` (including `<folder>` with `--install-links`) selects `DirFetcher`.
// `DirFetcher` runs the source package's `prepare` script and then packs the directory.
// It does *not* first run a nested install in that source directory. Although Arborist
// has resolved the package's dependency tree, those dependencies have not yet been
// reified into `<folder>/node_modules` when `DirFetcher` needs to prepare and pack it.
// Consequently, a conventional source package such as
//
// devDependencies: { "typescript": "..." }
// scripts: { "prepare": "tsc" }
//
// fails with `tsc: not found`. `--install-links` only changes whether directory
// content is packed instead of linked; it does not add the missing install-before-
// prepare step.
//
// * `git+file://<repo>` selects `GitFetcher`. It clones the already-pinned local
// checkout into npm's temporary cache. In npm 12, when the package needs
// preparation, `GitFetcher` runs a nested, non-global install roughly equivalent to:
//
// npm install --force --include=dev --include=peer --include=optional \
// --global=false
//
// The nested install makes build-time dependencies available and runs the root
// package's `prepare`; `DirFetcher` then packs that prepared temporary clone, and
// the outer global install installs the packed result into the hook environment.
// This is npm's documented behavior for Git dependencies and matches what package
// authors expect when publishing source that must be compiled before use.
//
// Besides fixing lifecycle ordering, the temporary Git clone keeps `node_modules` and
// generated build output out of prek's shared repository cache. The extra local clone
// and pack are deliberate costs in exchange for correct, isolated package preparation.
//
// npm 12 defaults `allow-git` to `none`, so prek must explicitly opt this top-level
// Git package into fetching. `root` is intentionally narrower than `all`: it permits
// Git dependencies introduced by this npm command's project root, while transitive
// Git dependencies remain blocked. Because all specs below share one npm command, a
// Git URL explicitly supplied through `additional_dependencies` is also a root
// dependency and is therefore allowed. We intentionally do not enable `allow-remote`,
// `allow-scripts`, or unrestricted `allow-git=all`; npm's other safety defaults remain
// in effect.
//
// npm < 12 does not need the allow flag and older GitFetcher implementations also
// lack the explicit `--global=false` on their nested install. That means a build
// which requires devDependencies during `prepare` is only fixed by this path on npm
// 12 or newer; it already failed with prek's previous folder installation on older
// npm.
//
// In particular, do not pass `--allow-git=root` to npm 11.9 through 11.12. Those
// releases have an npm bug, not a different definition of a root dependency. The
// first manifest fetch used to discover an unnamed CLI Git spec correctly receives
// `_isRoot=true`, and Arborist creates an edge from the project root. A later
// manifest fetch and the reify/extract path, however, fail to forward that context
// to pacote. Pacote defaults a missing `_isRoot` to false and consequently rejects
// the same root dependency as "non-root" with EALLOWGIT. npm 11 defaults
// `allow-git` to `all`, which masks the bug unless `root` is explicitly requested.
// The bug was fixed upstream and backported in npm 11.13:
//
// - https://github.com/npm/cli/issues/9189
// - https://github.com/npm/cli/pull/9206
//
// Since npm 11 already defaults to allowing Git, prek omits the flag for all npm 11
// versions to remain compatible with the affected releases. npm 12 both contains the
// fix and defaults `allow-git` to `none`, so that is where prek starts passing
// `--allow-git=root`. Querying npm itself instead of inferring from the Node version
// also covers custom and independently upgraded npm installations correctly.
//
// Relevant npm implementation:
// - pacote/lib/dir.js (`DirFetcher`)
// - pacote/lib/git.js (`GitFetcher`, especially `#prepareDir`)
// - @npmcli/arborist/lib/arborist/build-ideal-tree.js (`allow-git` root checks)

// `npm` is a script that uses `/usr/bin/env node`, so we need to add the
// node toolchain directory to PATH so that `npm` can find `node`.
Expand All @@ -105,14 +175,17 @@ impl LanguageBackend for Node {
let npm_cache = store.cache_path(CacheBucket::Npm);

let mut cmd = Cmd::new(node.npm());
cmd.arg("install")
.arg("-g")
cmd.arg("install");
if includes_git_hook_repo && query_npm_version(node.npm(), &new_path).await?.major >= 12
{
cmd.arg("--allow-git=root");
Comment thread
j178 marked this conversation as resolved.
}
cmd.arg("-g")
.arg("--no-progress")
.arg("--no-save")
.arg("--no-fund")
.arg("--no-audit")
.arg("--install-links")
.args(&*deps)
.args(&deps)
Comment thread
j178 marked this conversation as resolved.
.env(EnvVars::PATH, new_path)
.env(EnvVars::NODE_PATH, &lib_dir);
apply_npm_config_env(&mut cmd, &info.env_path, &npm_cache);
Expand Down Expand Up @@ -191,6 +264,35 @@ impl LanguageBackend for Node {
}
}

fn node_install_dependencies(hook: &Hook) -> Result<(Vec<String>, bool)> {
let mut deps = Vec::with_capacity(hook.additional_dependencies.len() + 1);
let mut includes_git_hook_repo = false;

if let Some(repo_path) = hook.repo_path() {
let file_url = Url::from_file_path(repo_path).map_err(|()| {
Comment thread
j178 marked this conversation as resolved.
anyhow!(
"Failed to convert Node hook repository path to a file URL: {}",
repo_path.display()
)
})?;
deps.push(format!("git+{file_url}"));
includes_git_hook_repo = true;
}

deps.extend(hook.additional_dependencies.iter().cloned());
Ok((deps, includes_git_hook_repo))
}

async fn query_npm_version(npm: &Path, path: &std::ffi::OsStr) -> Result<Version> {
let output = Cmd::new(npm)
.arg("--version")
.env(EnvVars::PATH, path)
.check(true)
.output()
.await?;
Version::parse(str::from_utf8(&output.stdout)?.trim()).context("Failed to parse npm version")
}

fn apply_npm_config_env(cmd: &mut Cmd, prefix: &Path, cache: &Path) {
for key in NPM_CONFIG_ENVS_TO_REMOVE {
cmd.env_remove(key);
Expand Down
2 changes: 1 addition & 1 deletion crates/prek/src/languages/python/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ mod tests {

let info = InstallInfo::create(Language::Python, None, Vec::new(), &hooks_dir)
.expect("create install info");
let store = Store::from_path(temp.path().join("store"));
let store = Store::from_path(temp.path().join("store")).expect("create store");
let uv = Uv::new(PathBuf::from("uv"));

(temp, uv, store, info)
Expand Down
8 changes: 5 additions & 3 deletions crates/prek/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@ pub struct Store {
}

impl Store {
pub(crate) fn from_path(path: impl Into<PathBuf>) -> Self {
Self { path: path.into() }
pub(crate) fn from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
Ok(Self {
path: std::path::absolute(path)?,
})
}

/// Create a store from environment variables or default paths.
Expand All @@ -83,7 +85,7 @@ impl Store {
let Some(path) = path else {
return Err(Error::HomeNotFound);
};
let store = Store::from_path(path).init()?;
let store = Store::from_path(path)?.init()?;

Ok(store)
}
Expand Down
Loading
Loading