From 533a9b108677f1560fe55882771a0be2bb0b0fd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9E=97=E7=82=B3=E6=9D=83?= <695601626@qq.com> Date: Wed, 16 Oct 2024 00:10:07 +0800 Subject: chore: upgrade to rust 1.81.0 (#26261) --- cli/npm/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cli/npm') diff --git a/cli/npm/common.rs b/cli/npm/common.rs index a3a828e74..de282310a 100644 --- a/cli/npm/common.rs +++ b/cli/npm/common.rs @@ -40,7 +40,7 @@ pub fn maybe_auth_header_for_npm_registry( header::AUTHORIZATION, header::HeaderValue::from_str(&format!( "Basic {}", - BASE64_STANDARD.encode(&format!( + BASE64_STANDARD.encode(format!( "{}:{}", username.unwrap(), password.unwrap() -- cgit v1.2.3 From 7c3c13cecf48219cdcb90dc0b5019686cdd88626 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:46:42 -0700 Subject: fix(install): retry downloads of registry info / tarballs (#26278) Fixes #26085. Adds a basic retry utility with some defaults, starts off with a 100ms wait, then 250ms, then 500ms I've applied the retry in the http client, reusing an existing function, so this also applies to retrying downloads of deno binaries in `upgrade` and `compile`. I can make a separate function that doesn't retry so this doesn't affect `upgrade` and `compile`, but it seemed desirable to have retries there too, so I left it in. --- cli/npm/managed/cache/registry_info.rs | 11 +++++++---- cli/npm/managed/cache/tarball.rs | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/cache/registry_info.rs b/cli/npm/managed/cache/registry_info.rs index 28b19373e..597a2283f 100644 --- a/cli/npm/managed/cache/registry_info.rs +++ b/cli/npm/managed/cache/registry_info.rs @@ -202,10 +202,13 @@ impl RegistryInfoDownloader { let guard = self.progress_bar.update(package_url.as_str()); let name = name.to_string(); async move { - let maybe_bytes = downloader - .http_client_provider - .get_or_create()? - .download_with_progress(package_url, maybe_auth_header, &guard) + let client = downloader.http_client_provider.get_or_create()?; + let maybe_bytes = client + .download_with_progress_and_retries( + package_url, + maybe_auth_header, + &guard, + ) .await?; match maybe_bytes { Some(bytes) => { diff --git a/cli/npm/managed/cache/tarball.rs b/cli/npm/managed/cache/tarball.rs index 4bcee38ea..7cf88d6d6 100644 --- a/cli/npm/managed/cache/tarball.rs +++ b/cli/npm/managed/cache/tarball.rs @@ -172,7 +172,7 @@ impl TarballCache { let guard = tarball_cache.progress_bar.update(&dist.tarball); let result = tarball_cache.http_client_provider .get_or_create()? - .download_with_progress(tarball_uri, maybe_auth_header, &guard) + .download_with_progress_and_retries(tarball_uri, maybe_auth_header, &guard) .await; let maybe_bytes = match result { Ok(maybe_bytes) => maybe_bytes, -- cgit v1.2.3 From 4b99cde504854baabdcf912f2fce3a7448119653 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 18 Oct 2024 20:38:57 +0200 Subject: fix(npm): ensure scoped package name is encoded in URLs (#26390) Fixes https://github.com/denoland/deno/issues/26385 --- cli/npm/managed/cache/registry_info.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/cache/registry_info.rs b/cli/npm/managed/cache/registry_info.rs index 597a2283f..6c4a7503b 100644 --- a/cli/npm/managed/cache/registry_info.rs +++ b/cli/npm/managed/cache/registry_info.rs @@ -242,6 +242,14 @@ impl RegistryInfoDownloader { fn get_package_url(&self, name: &str) -> Url { let registry_url = self.npmrc.get_registry_url(name); + // The '/' character in scoped package names "@scope/name" must be + // encoded for older third party registries. Newer registries and + // npm itself support both ways + // - encoded: https://registry.npmjs.org/@rollup%2fplugin-json + // - non-ecoded: https://registry.npmjs.org/@rollup/plugin-json + // To support as many third party registries as possible we'll + // always encode the '/' character. + // list of all characters used in npm packages: // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~ const ASCII_SET: percent_encoding::AsciiSet = @@ -253,11 +261,14 @@ impl RegistryInfoDownloader { .remove(b'*') .remove(b'-') .remove(b'.') - .remove(b'/') .remove(b'@') .remove(b'_') .remove(b'~'); let name = percent_encoding::utf8_percent_encode(name, &ASCII_SET); - registry_url.join(&name.to_string()).unwrap() + registry_url + // Ensure that scoped package name percent encoding is lower cased + // to match npm. + .join(&name.to_string().replace("%2F", "%2f")) + .unwrap() } } -- cgit v1.2.3 From 5f0bb3c6f4328003012e98ba70ce18e4e2e842de Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Thu, 24 Oct 2024 20:03:56 +0200 Subject: fix: `.npmrc` settings not being passed to install/add command (#26473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We weren't passing the resolved npmrc settings to the install commands. This lead us to always fall back to the default registry url instead of using the one from npmrc. Fixes https://github.com/denoland/deno/issues/26139 Fixes https://github.com/denoland/deno/issues/26033 Fixes https://github.com/denoland/deno/issues/25924 Fixes https://github.com/denoland/deno/issues/25822 Fixes https://github.com/denoland/deno/issues/26152 --------- Co-authored-by: Bartek Iwańczuk --- cli/npm/managed/cache/mod.rs | 2 +- cli/npm/managed/cache/registry_info.rs | 64 +++++++++++++++++----------------- cli/npm/managed/mod.rs | 2 +- cli/npm/mod.rs | 25 ++++++++++--- 4 files changed, 55 insertions(+), 38 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/cache/mod.rs b/cli/npm/managed/cache/mod.rs index fa0e8c8a5..aaec48668 100644 --- a/cli/npm/managed/cache/mod.rs +++ b/cli/npm/managed/cache/mod.rs @@ -26,7 +26,7 @@ use crate::cache::CACHE_PERM; use crate::util::fs::atomic_write_file_with_retries; use crate::util::fs::hard_link_dir_recursive; -mod registry_info; +pub mod registry_info; mod tarball; mod tarball_extract; diff --git a/cli/npm/managed/cache/registry_info.rs b/cli/npm/managed/cache/registry_info.rs index 6c4a7503b..6d39d3c13 100644 --- a/cli/npm/managed/cache/registry_info.rs +++ b/cli/npm/managed/cache/registry_info.rs @@ -84,7 +84,7 @@ impl RegistryInfoDownloader { self.load_package_info_inner(name).await.with_context(|| { format!( "Error getting response at {} for package \"{}\"", - self.get_package_url(name), + get_package_url(&self.npmrc, name), name ) }) @@ -190,7 +190,7 @@ impl RegistryInfoDownloader { fn create_load_future(self: &Arc, name: &str) -> LoadFuture { let downloader = self.clone(); - let package_url = self.get_package_url(name); + let package_url = get_package_url(&self.npmrc, name); let registry_config = self.npmrc.get_registry_config(name); let maybe_auth_header = match maybe_auth_header_for_npm_registry(registry_config) { @@ -239,36 +239,36 @@ impl RegistryInfoDownloader { .map(|r| r.map_err(Arc::new)) .boxed_local() } +} - fn get_package_url(&self, name: &str) -> Url { - let registry_url = self.npmrc.get_registry_url(name); - // The '/' character in scoped package names "@scope/name" must be - // encoded for older third party registries. Newer registries and - // npm itself support both ways - // - encoded: https://registry.npmjs.org/@rollup%2fplugin-json - // - non-ecoded: https://registry.npmjs.org/@rollup/plugin-json - // To support as many third party registries as possible we'll - // always encode the '/' character. +pub fn get_package_url(npmrc: &ResolvedNpmRc, name: &str) -> Url { + let registry_url = npmrc.get_registry_url(name); + // The '/' character in scoped package names "@scope/name" must be + // encoded for older third party registries. Newer registries and + // npm itself support both ways + // - encoded: https://registry.npmjs.org/@rollup%2fplugin-json + // - non-ecoded: https://registry.npmjs.org/@rollup/plugin-json + // To support as many third party registries as possible we'll + // always encode the '/' character. - // list of all characters used in npm packages: - // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~ - const ASCII_SET: percent_encoding::AsciiSet = - percent_encoding::NON_ALPHANUMERIC - .remove(b'!') - .remove(b'\'') - .remove(b'(') - .remove(b')') - .remove(b'*') - .remove(b'-') - .remove(b'.') - .remove(b'@') - .remove(b'_') - .remove(b'~'); - let name = percent_encoding::utf8_percent_encode(name, &ASCII_SET); - registry_url - // Ensure that scoped package name percent encoding is lower cased - // to match npm. - .join(&name.to_string().replace("%2F", "%2f")) - .unwrap() - } + // list of all characters used in npm packages: + // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~ + const ASCII_SET: percent_encoding::AsciiSet = + percent_encoding::NON_ALPHANUMERIC + .remove(b'!') + .remove(b'\'') + .remove(b'(') + .remove(b')') + .remove(b'*') + .remove(b'-') + .remove(b'.') + .remove(b'@') + .remove(b'_') + .remove(b'~'); + let name = percent_encoding::utf8_percent_encode(name, &ASCII_SET); + registry_url + // Ensure that scoped package name percent encoding is lower cased + // to match npm. + .join(&name.to_string().replace("%2F", "%2f")) + .unwrap() } diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index ec50a9c65..d0880557f 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -55,7 +55,7 @@ use super::CliNpmResolver; use super::InnerCliNpmResolverRef; use super::ResolvePkgFolderFromDenoReqError; -mod cache; +pub mod cache; mod registry; mod resolution; mod resolvers; diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 53baaf77b..f48f7a740 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -8,10 +8,12 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use common::maybe_auth_header_for_npm_registry; use dashmap::DashMap; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; use deno_resolver::npm::ByonmNpmResolver; use deno_resolver::npm::ByonmResolvePkgFolderFromDenoReqError; @@ -19,10 +21,10 @@ use deno_runtime::deno_node::NodeRequireResolver; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; +use managed::cache::registry_info::get_package_url; use node_resolver::NpmResolver; use thiserror::Error; -use crate::args::npm_registry_url; use crate::file_fetcher::FileFetcher; pub use self::byonm::CliByonmNpmResolver; @@ -115,14 +117,19 @@ pub struct NpmFetchResolver { nv_by_req: DashMap>, info_by_name: DashMap>>, file_fetcher: Arc, + npmrc: Arc, } impl NpmFetchResolver { - pub fn new(file_fetcher: Arc) -> Self { + pub fn new( + file_fetcher: Arc, + npmrc: Arc, + ) -> Self { Self { nv_by_req: Default::default(), info_by_name: Default::default(), file_fetcher, + npmrc, } } @@ -157,11 +164,21 @@ impl NpmFetchResolver { return info.value().clone(); } let fetch_package_info = || async { - let info_url = npm_registry_url().join(name).ok()?; + let info_url = get_package_url(&self.npmrc, name); let file_fetcher = self.file_fetcher.clone(); + let registry_config = self.npmrc.get_registry_config(name); + // TODO(bartlomieju): this should error out, not use `.ok()`. + let maybe_auth_header = + maybe_auth_header_for_npm_registry(registry_config).ok()?; // spawn due to the lsp's `Send` requirement let file = deno_core::unsync::spawn(async move { - file_fetcher.fetch_bypass_permissions(&info_url).await.ok() + file_fetcher + .fetch_bypass_permissions_with_maybe_auth( + &info_url, + maybe_auth_header, + ) + .await + .ok() }) .await .ok()??; -- cgit v1.2.3 From 5389972ba5037b1ed48da11506f6798deec2b48e Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Mon, 28 Oct 2024 12:08:51 +0100 Subject: fix(npm): match npm bearer token generation (#26544) Spend some time stepping through the npm client code and noticed that the bearer token was different from ours. They do some double encoding and @dsherret helped me in matching the encoding behavior. Fixes https://github.com/denoland/deno/issues/26033 --- cli/npm/common.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/common.rs b/cli/npm/common.rs index de282310a..55f1bc086 100644 --- a/cli/npm/common.rs +++ b/cli/npm/common.rs @@ -3,6 +3,7 @@ use base64::prelude::BASE64_STANDARD; use base64::Engine; use deno_core::anyhow::bail; +use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_npm::npm_rc::RegistryConfig; use http::header; @@ -36,17 +37,21 @@ pub fn maybe_auth_header_for_npm_registry( } if username.is_some() && password.is_some() { + // The npm client does some double encoding when generating the + // bearer token value, see + // https://github.com/npm/cli/blob/780afc50e3a345feb1871a28e33fa48235bc3bd5/workspaces/config/lib/index.js#L846-L851 + let pw_base64 = BASE64_STANDARD + .decode(password.unwrap()) + .with_context(|| "The password in npmrc is an invalid base64 string")?; + let bearer = BASE64_STANDARD.encode(format!( + "{}:{}", + username.unwrap(), + String::from_utf8_lossy(&pw_base64) + )); + return Ok(Some(( header::AUTHORIZATION, - header::HeaderValue::from_str(&format!( - "Basic {}", - BASE64_STANDARD.encode(format!( - "{}:{}", - username.unwrap(), - password.unwrap() - )) - )) - .unwrap(), + header::HeaderValue::from_str(&format!("Basic {}", bearer)).unwrap(), ))); } -- cgit v1.2.3 From 90edca21a26fd2decd0603fea37af10d1e11e454 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 31 Oct 2024 11:35:17 -0400 Subject: fix: surface package.json location on dep parse failure (#26665) Related: https://github.com/denoland/deno/issues/26653 --- cli/npm/managed/mod.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index d0880557f..a0754812b 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -38,6 +38,7 @@ use crate::args::LifecycleScriptsConfig; use crate::args::NpmInstallDepsProvider; use crate::args::NpmProcessState; use crate::args::NpmProcessStateKind; +use crate::args::PackageJsonDepValueParseWithLocationError; use crate::cache::DenoCacheEnvFsAdapter; use crate::cache::FastInsecureHasher; use crate::http_util::HttpClientProvider; @@ -480,19 +481,24 @@ impl ManagedCliNpmResolver { self.resolution.resolve_pkg_id_from_pkg_req(req) } - pub fn ensure_no_pkg_json_dep_errors(&self) -> Result<(), AnyError> { + pub fn ensure_no_pkg_json_dep_errors( + &self, + ) -> Result<(), Box> { for err in self.npm_install_deps_provider.pkg_json_dep_errors() { - match err { + match &err.source { deno_package_json::PackageJsonDepValueParseError::VersionReq(_) => { - return Err( - AnyError::from(err.clone()) - .context("Failed to install from package.json"), - ); + return Err(Box::new(err.clone())); } deno_package_json::PackageJsonDepValueParseError::Unsupported { .. } => { - log::warn!("{} {} in package.json", colors::yellow("Warning"), err) + // only warn for this one + log::warn!( + "{} {}\n at {}", + colors::yellow("Warning"), + err.source, + err.location, + ) } } } -- cgit v1.2.3 From 04ae1a551726dd6e0047a942b459d18e1dcb1935 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 31 Oct 2024 22:19:19 -0700 Subject: fix(cli): set `npm_config_user_agent` when running npm packages or tasks (#26639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #25342. Still not sure on the exact user agent to set (should it include `node`?). After this PR, here's the state of running some `create-*` packages (just ones I could think of off the top of my head): | package | prints/runs/suggests deno install | notes | | ---------------- | ------------- | ------ | | `create-next-app` | ❌ | falls back to npm, needs a PR ([code](https://github.com/vercel/next.js/blob/c32e2802097c03fd9f95b1dae228d6e0257569c0/packages/create-next-app/helpers/get-pkg-manager.ts#L3)) | `sv create` | ❌ | uses `package-manager-detector`, needs a PR ([code](https://github.com/antfu-collective/package-manager-detector/tree/main)) | `create-qwik` | ✅ | runs `deno install` but suggests `deno start` which doesn't work (should be `deno task start` or `deno run start`) | `create-astro` | ✅ | runs `deno install` but suggests `npm run dev` later in output, probably needs a PR | `nuxi init` | ❌ | deno not an option in dialog, needs a PR ([code](https://github.com/nuxt/cli/blob/f04e2e894446f597da9d971b7eb03191d5a0da7e/src/commands/init.ts#L96-L102)) | `create-react-app` | ❌ | uses npm | `ng new` (`@angular/cli`) | ❌ | uses npm | `create-vite` | ✅ | suggests working deno commands 🎉 | `create-solid` | ❌ | suggests npm commands, needs PR It's possible that fixing `package-manager-detector` or other packages might make some of these just work, but haven't looked too carefully at each --- cli/npm/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'cli/npm') diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index f48f7a740..3971b1a0b 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -189,3 +189,15 @@ impl NpmFetchResolver { info } } + +pub const NPM_CONFIG_USER_AGENT_ENV_VAR: &str = "npm_config_user_agent"; + +pub fn get_npm_config_user_agent() -> String { + format!( + "deno/{} npm/? deno/{} {} {}", + env!("CARGO_PKG_VERSION"), + env!("CARGO_PKG_VERSION"), + std::env::consts::OS, + std::env::consts::ARCH + ) +} -- cgit v1.2.3 From 826e42a5b5880c974ae019a7a21aade6a718062c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 1 Nov 2024 12:27:00 -0400 Subject: fix: improved support for cjs and cts modules (#26558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * cts support * better cjs/cts type checking * deno compile cjs/cts support * More efficient detect cjs (going towards stabilization) * Determination of whether .js, .ts, .jsx, or .tsx is cjs or esm is only done after loading * Support `import x = require(...);` Co-authored-by: Bartek Iwańczuk --- cli/npm/byonm.rs | 43 ++++++++---------- cli/npm/managed/cache/mod.rs | 12 ++--- cli/npm/managed/mod.rs | 91 +++++++++++++++++++++++-------------- cli/npm/managed/resolvers/common.rs | 4 -- cli/npm/managed/resolvers/global.rs | 7 +-- cli/npm/managed/resolvers/local.rs | 4 -- cli/npm/mod.rs | 34 ++++++++++++-- 7 files changed, 112 insertions(+), 83 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index fc095ab16..4535b07fc 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -10,8 +10,8 @@ use deno_core::serde_json; use deno_core::url::Url; use deno_resolver::npm::ByonmNpmResolver; use deno_resolver::npm::ByonmNpmResolverCreateOptions; +use deno_runtime::deno_node::DenoFsNodeResolverEnv; use deno_runtime::deno_node::NodePermissions; -use deno_runtime::deno_node::NodeRequireResolver; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageReq; use node_resolver::NpmResolver; @@ -25,30 +25,14 @@ use super::InnerCliNpmResolverRef; use super::ResolvePkgFolderFromDenoReqError; pub type CliByonmNpmResolverCreateOptions = - ByonmNpmResolverCreateOptions; -pub type CliByonmNpmResolver = ByonmNpmResolver; + ByonmNpmResolverCreateOptions; +pub type CliByonmNpmResolver = + ByonmNpmResolver; // todo(dsherret): the services hanging off `CliNpmResolver` doesn't seem ideal. We should probably decouple. #[derive(Debug)] struct CliByonmWrapper(Arc); -impl NodeRequireResolver for CliByonmWrapper { - fn ensure_read_permission<'a>( - &self, - permissions: &mut dyn NodePermissions, - path: &'a Path, - ) -> Result, AnyError> { - if !path - .components() - .any(|c| c.as_os_str().to_ascii_lowercase() == "node_modules") - { - permissions.check_read_path(path) - } else { - Ok(Cow::Borrowed(path)) - } - } -} - impl NpmProcessStateProvider for CliByonmWrapper { fn get_npm_process_state(&self) -> String { serde_json::to_string(&NpmProcessState { @@ -67,10 +51,6 @@ impl CliNpmResolver for CliByonmNpmResolver { self } - fn into_require_resolver(self: Arc) -> Arc { - Arc::new(CliByonmWrapper(self)) - } - fn into_process_state_provider( self: Arc, ) -> Arc { @@ -100,6 +80,21 @@ impl CliNpmResolver for CliByonmNpmResolver { .map_err(ResolvePkgFolderFromDenoReqError::Byonm) } + fn ensure_read_permission<'a>( + &self, + permissions: &mut dyn NodePermissions, + path: &'a Path, + ) -> Result, AnyError> { + if !path + .components() + .any(|c| c.as_os_str().to_ascii_lowercase() == "node_modules") + { + permissions.check_read_path(path) + } else { + Ok(Cow::Borrowed(path)) + } + } + fn check_state_hash(&self) -> Option { // it is very difficult to determine the check state hash for byonm // so we just return None to signify check caching is not supported diff --git a/cli/npm/managed/cache/mod.rs b/cli/npm/managed/cache/mod.rs index aaec48668..8ae99f41e 100644 --- a/cli/npm/managed/cache/mod.rs +++ b/cli/npm/managed/cache/mod.rs @@ -36,7 +36,7 @@ pub use tarball::TarballCache; /// Stores a single copy of npm packages in a cache. #[derive(Debug)] pub struct NpmCache { - cache_dir: NpmCacheDir, + cache_dir: Arc, cache_setting: CacheSetting, npmrc: Arc, /// ensures a package is only downloaded once per run @@ -45,7 +45,7 @@ pub struct NpmCache { impl NpmCache { pub fn new( - cache_dir: NpmCacheDir, + cache_dir: Arc, cache_setting: CacheSetting, npmrc: Arc, ) -> Self { @@ -61,6 +61,10 @@ impl NpmCache { &self.cache_setting } + pub fn root_dir_path(&self) -> &Path { + self.cache_dir.root_dir() + } + pub fn root_dir_url(&self) -> &Url { self.cache_dir.root_dir_url() } @@ -152,10 +156,6 @@ impl NpmCache { self.cache_dir.package_name_folder(name, registry_url) } - pub fn root_folder(&self) -> PathBuf { - self.cache_dir.root_dir().to_owned() - } - pub fn resolve_package_folder_id_from_specifier( &self, specifier: &ModuleSpecifier, diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index a0754812b..4a91bc347 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -12,6 +12,7 @@ use deno_cache_dir::npm::NpmCacheDir; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_core::url::Url; use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; use deno_npm::registry::NpmRegistryApi; @@ -24,12 +25,12 @@ use deno_npm::NpmSystemInfo; use deno_runtime::colors; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; -use deno_runtime::deno_node::NodeRequireResolver; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; use node_resolver::errors::PackageFolderResolveError; use node_resolver::errors::PackageFolderResolveIoError; +use node_resolver::InNpmPackageChecker; use node_resolver::NpmResolver; use resolution::AddPkgReqsResult; @@ -39,7 +40,6 @@ use crate::args::NpmInstallDepsProvider; use crate::args::NpmProcessState; use crate::args::NpmProcessStateKind; use crate::args::PackageJsonDepValueParseWithLocationError; -use crate::cache::DenoCacheEnvFsAdapter; use crate::cache::FastInsecureHasher; use crate::http_util::HttpClientProvider; use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; @@ -66,12 +66,12 @@ pub enum CliNpmResolverManagedSnapshotOption { Specified(Option), } -pub struct CliNpmResolverManagedCreateOptions { +pub struct CliManagedNpmResolverCreateOptions { pub snapshot: CliNpmResolverManagedSnapshotOption, pub maybe_lockfile: Option>, pub fs: Arc, pub http_client_provider: Arc, - pub npm_global_cache_dir: PathBuf, + pub npm_cache_dir: Arc, pub cache_setting: crate::args::CacheSetting, pub text_only_progress_bar: crate::util::progress_bar::ProgressBar, pub maybe_node_modules_path: Option, @@ -82,7 +82,7 @@ pub struct CliNpmResolverManagedCreateOptions { } pub async fn create_managed_npm_resolver_for_lsp( - options: CliNpmResolverManagedCreateOptions, + options: CliManagedNpmResolverCreateOptions, ) -> Arc { let npm_cache = create_cache(&options); let npm_api = create_api(&options, npm_cache.clone()); @@ -115,7 +115,7 @@ pub async fn create_managed_npm_resolver_for_lsp( } pub async fn create_managed_npm_resolver( - options: CliNpmResolverManagedCreateOptions, + options: CliManagedNpmResolverCreateOptions, ) -> Result, AnyError> { let npm_cache = create_cache(&options); let npm_api = create_api(&options, npm_cache.clone()); @@ -189,20 +189,16 @@ fn create_inner( )) } -fn create_cache(options: &CliNpmResolverManagedCreateOptions) -> Arc { +fn create_cache(options: &CliManagedNpmResolverCreateOptions) -> Arc { Arc::new(NpmCache::new( - NpmCacheDir::new( - &DenoCacheEnvFsAdapter(options.fs.as_ref()), - options.npm_global_cache_dir.clone(), - options.npmrc.get_all_known_registries_urls(), - ), + options.npm_cache_dir.clone(), options.cache_setting.clone(), options.npmrc.clone(), )) } fn create_api( - options: &CliNpmResolverManagedCreateOptions, + options: &CliManagedNpmResolverCreateOptions, npm_cache: Arc, ) -> Arc { Arc::new(CliNpmRegistryApi::new( @@ -259,6 +255,35 @@ async fn snapshot_from_lockfile( Ok(snapshot) } +#[derive(Debug)] +struct ManagedInNpmPackageChecker { + root_dir: Url, +} + +impl InNpmPackageChecker for ManagedInNpmPackageChecker { + fn in_npm_package(&self, specifier: &Url) -> bool { + specifier.as_ref().starts_with(self.root_dir.as_str()) + } +} + +pub struct CliManagedInNpmPkgCheckerCreateOptions<'a> { + pub root_cache_dir_url: &'a Url, + pub maybe_node_modules_path: Option<&'a Path>, +} + +pub fn create_managed_in_npm_pkg_checker( + options: CliManagedInNpmPkgCheckerCreateOptions, +) -> Arc { + let root_dir = match options.maybe_node_modules_path { + Some(node_modules_folder) => { + deno_path_util::url_from_directory_path(node_modules_folder).unwrap() + } + None => options.root_cache_dir_url.clone(), + }; + debug_assert!(root_dir.as_str().ends_with('/')); + Arc::new(ManagedInNpmPackageChecker { root_dir }) +} + /// An npm resolver where the resolution is managed by Deno rather than /// the user bringing their own node_modules (BYONM) on the file system. pub struct ManagedCliNpmResolver { @@ -555,8 +580,16 @@ impl ManagedCliNpmResolver { .map_err(|err| err.into()) } - pub fn global_cache_root_folder(&self) -> PathBuf { - self.npm_cache.root_folder() + pub fn maybe_node_modules_path(&self) -> Option<&Path> { + self.fs_resolver.node_modules_path() + } + + pub fn global_cache_root_path(&self) -> &Path { + self.npm_cache.root_dir_path() + } + + pub fn global_cache_root_url(&self) -> &Url { + self.npm_cache.root_dir_url() } } @@ -591,22 +624,6 @@ impl NpmResolver for ManagedCliNpmResolver { log::debug!("Resolved {} from {} to {}", name, referrer, path.display()); Ok(path) } - - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { - let root_dir_url = self.fs_resolver.root_dir_url(); - debug_assert!(root_dir_url.as_str().ends_with('/')); - specifier.as_ref().starts_with(root_dir_url.as_str()) - } -} - -impl NodeRequireResolver for ManagedCliNpmResolver { - fn ensure_read_permission<'a>( - &self, - permissions: &mut dyn NodePermissions, - path: &'a Path, - ) -> Result, AnyError> { - self.fs_resolver.ensure_read_permission(permissions, path) - } } impl NpmProcessStateProvider for ManagedCliNpmResolver { @@ -623,10 +640,6 @@ impl CliNpmResolver for ManagedCliNpmResolver { self } - fn into_require_resolver(self: Arc) -> Arc { - self - } - fn into_process_state_provider( self: Arc, ) -> Arc { @@ -687,6 +700,14 @@ impl CliNpmResolver for ManagedCliNpmResolver { .map_err(ResolvePkgFolderFromDenoReqError::Managed) } + fn ensure_read_permission<'a>( + &self, + permissions: &mut dyn NodePermissions, + path: &'a Path, + ) -> Result, AnyError> { + self.fs_resolver.ensure_read_permission(permissions, path) + } + fn check_state_hash(&self) -> Option { // We could go further and check all the individual // npm packages, but that's probably overkill. diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index 867bb4168..590f8fb25 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -17,7 +17,6 @@ use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::futures; use deno_core::futures::StreamExt; -use deno_core::url::Url; use deno_npm::NpmPackageCacheFolderId; use deno_npm::NpmPackageId; use deno_npm::NpmResolutionPackage; @@ -30,9 +29,6 @@ use crate::npm::managed::cache::TarballCache; /// Part of the resolution that interacts with the file system. #[async_trait(?Send)] pub trait NpmPackageFsResolver: Send + Sync { - /// Specifier for the root directory. - fn root_dir_url(&self) -> &Url; - /// The local node_modules folder if it is applicable to the implementation. fn node_modules_path(&self) -> Option<&Path>; diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs index 5be315e99..f0193e78e 100644 --- a/cli/npm/managed/resolvers/global.rs +++ b/cli/npm/managed/resolvers/global.rs @@ -11,7 +11,6 @@ use crate::colors; use async_trait::async_trait; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; -use deno_core::url::Url; use deno_npm::NpmPackageCacheFolderId; use deno_npm::NpmPackageId; use deno_npm::NpmResolutionPackage; @@ -56,7 +55,7 @@ impl GlobalNpmPackageResolver { Self { registry_read_permission_checker: RegistryReadPermissionChecker::new( fs, - cache.root_folder(), + cache.root_dir_path().to_path_buf(), ), cache, tarball_cache, @@ -69,10 +68,6 @@ impl GlobalNpmPackageResolver { #[async_trait(?Send)] impl NpmPackageFsResolver for GlobalNpmPackageResolver { - fn root_dir_url(&self) -> &Url { - self.cache.root_dir_url() - } - fn node_modules_path(&self) -> Option<&Path> { None } diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 54f7576ad..0968be8a7 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -155,10 +155,6 @@ impl LocalNpmPackageResolver { #[async_trait(?Send)] impl NpmPackageFsResolver for LocalNpmPackageResolver { - fn root_dir_url(&self) -> &Url { - &self.root_node_modules_url - } - fn node_modules_path(&self) -> Option<&Path> { Some(self.root_node_modules_path.as_ref()) } diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 3971b1a0b..0d434ca27 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -4,6 +4,7 @@ mod byonm; mod common; mod managed; +use std::borrow::Cow; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -15,13 +16,16 @@ use deno_core::error::AnyError; use deno_core::serde_json; use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; +use deno_resolver::npm::ByonmInNpmPackageChecker; use deno_resolver::npm::ByonmNpmResolver; use deno_resolver::npm::ByonmResolvePkgFolderFromDenoReqError; -use deno_runtime::deno_node::NodeRequireResolver; +use deno_runtime::deno_node::NodePermissions; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; use managed::cache::registry_info::get_package_url; +use managed::create_managed_in_npm_pkg_checker; +use node_resolver::InNpmPackageChecker; use node_resolver::NpmResolver; use thiserror::Error; @@ -29,7 +33,8 @@ use crate::file_fetcher::FileFetcher; pub use self::byonm::CliByonmNpmResolver; pub use self::byonm::CliByonmNpmResolverCreateOptions; -pub use self::managed::CliNpmResolverManagedCreateOptions; +pub use self::managed::CliManagedInNpmPkgCheckerCreateOptions; +pub use self::managed::CliManagedNpmResolverCreateOptions; pub use self::managed::CliNpmResolverManagedSnapshotOption; pub use self::managed::ManagedCliNpmResolver; @@ -42,7 +47,7 @@ pub enum ResolvePkgFolderFromDenoReqError { } pub enum CliNpmResolverCreateOptions { - Managed(CliNpmResolverManagedCreateOptions), + Managed(CliManagedNpmResolverCreateOptions), Byonm(CliByonmNpmResolverCreateOptions), } @@ -68,6 +73,22 @@ pub async fn create_cli_npm_resolver( } } +pub enum CreateInNpmPkgCheckerOptions<'a> { + Managed(CliManagedInNpmPkgCheckerCreateOptions<'a>), + Byonm, +} + +pub fn create_in_npm_pkg_checker( + options: CreateInNpmPkgCheckerOptions, +) -> Arc { + match options { + CreateInNpmPkgCheckerOptions::Managed(options) => { + create_managed_in_npm_pkg_checker(options) + } + CreateInNpmPkgCheckerOptions::Byonm => Arc::new(ByonmInNpmPackageChecker), + } +} + pub enum InnerCliNpmResolverRef<'a> { Managed(&'a ManagedCliNpmResolver), #[allow(dead_code)] @@ -76,7 +97,6 @@ pub enum InnerCliNpmResolverRef<'a> { pub trait CliNpmResolver: NpmResolver { fn into_npm_resolver(self: Arc) -> Arc; - fn into_require_resolver(self: Arc) -> Arc; fn into_process_state_provider( self: Arc, ) -> Arc; @@ -107,6 +127,12 @@ pub trait CliNpmResolver: NpmResolver { referrer: &ModuleSpecifier, ) -> Result; + fn ensure_read_permission<'a>( + &self, + permissions: &mut dyn NodePermissions, + path: &'a Path, + ) -> Result, AnyError>; + /// Returns a hash returning the state of the npm resolver /// or `None` if the state currently can't be determined. fn check_state_hash(&self) -> Option; -- cgit v1.2.3 From fe9f0ee5934871175758857899fe64e56c397fd5 Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Mon, 4 Nov 2024 09:17:21 -0800 Subject: refactor(runtime/permissions): use concrete error types (#26464) --- cli/npm/byonm.rs | 2 +- cli/npm/managed/resolvers/common.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 4535b07fc..45fa4cfd1 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -89,7 +89,7 @@ impl CliNpmResolver for CliByonmNpmResolver { .components() .any(|c| c.as_os_str().to_ascii_lowercase() == "node_modules") { - permissions.check_read_path(path) + permissions.check_read_path(path).map_err(Into::into) } else { Ok(Cow::Borrowed(path)) } diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index 590f8fb25..eee11c760 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -133,7 +133,7 @@ impl RegistryReadPermissionChecker { } } - permissions.check_read_path(path) + permissions.check_read_path(path).map_err(Into::into) } } -- cgit v1.2.3 From f9a05068d6de247574fb764044a446d1d7ed2e9b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Mon, 4 Nov 2024 20:16:53 -0800 Subject: fix(install): handle invalid function error, and fallback to junctions regardless of the error (#26730) Fixes #26116. Handle the new error and treat is as lacking permission to make symlinks, but also to make this more robust, just always fall back to junctions no matter what the actual error is. Instead, warn if the error isn't one we've handled, but go on to attempt creating the junction --- cli/npm/managed/resolvers/local.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 0968be8a7..eddb0dc9b 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -1035,12 +1035,18 @@ fn junction_or_symlink_dir( if symlink_err.kind() == std::io::ErrorKind::PermissionDenied => { USE_JUNCTIONS.store(true, std::sync::atomic::Ordering::Relaxed); - junction::create(old_path, new_path).map_err(Into::into) + junction::create(old_path, new_path) + .context("Failed creating junction in node_modules folder") + } + Err(symlink_err) => { + log::warn!( + "{} Unexpected error symlinking node_modules: {symlink_err}", + colors::yellow("Warning") + ); + USE_JUNCTIONS.store(true, std::sync::atomic::Ordering::Relaxed); + junction::create(old_path, new_path) + .context("Failed creating junction in node_modules folder") } - Err(symlink_err) => Err( - AnyError::from(symlink_err) - .context("Failed creating symlink in node_modules folder"), - ), } } -- cgit v1.2.3 From c371b2a492c60f47ce6b96b4df129c5d01706e1b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 12 Nov 2024 09:23:39 -0800 Subject: fix(install): re-setup bin entries after running lifecycle scripts (#26752) Fixes #26677 Some packages (like supabase) declare bin entries that don't exist until lifecycle scripts are run. For instance, the lifecycle script downloads a binary file which serves as a bin entrypoint. Unfortunately you can't just defer setting up the bin entries until after lifecycle scripts have run, because the scripts may rely on them. I looked into this, and PNPM just re-links bin entries after running lifecycle scripts. I think that's about the best we can do as well. Note that we'll only re-setup bin entries for packages whose lifecycle scripts we run. This should limit the performance cost, as typically a given project will not have many lifecycle scripts (and of those, many of them probably don't have bin entries to set up). --- cli/npm/managed/resolvers/common/bin_entries.rs | 139 ++++++++++++++++----- .../managed/resolvers/common/lifecycle_scripts.rs | 57 ++++++--- cli/npm/managed/resolvers/local.rs | 29 ++++- 3 files changed, 178 insertions(+), 47 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/managed/resolvers/common/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs index 4524ce832..e4a184568 100644 --- a/cli/npm/managed/resolvers/common/bin_entries.rs +++ b/cli/npm/managed/resolvers/common/bin_entries.rs @@ -18,6 +18,7 @@ pub struct BinEntries<'a> { seen_names: HashMap<&'a str, &'a NpmPackageId>, /// The bin entries entries: Vec<(&'a NpmResolutionPackage, PathBuf)>, + sorted: bool, } /// Returns the name of the default binary for the given package. @@ -31,6 +32,20 @@ fn default_bin_name(package: &NpmResolutionPackage) -> &str { .map_or(package.id.nv.name.as_str(), |(_, name)| name) } +pub fn warn_missing_entrypoint( + bin_name: &str, + package_path: &Path, + entrypoint: &Path, +) { + log::warn!( + "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.", + deno_terminal::colors::yellow("Warning"), + bin_name, + package_path.display(), + entrypoint.display() + ); +} + impl<'a> BinEntries<'a> { pub fn new() -> Self { Self::default() @@ -42,6 +57,7 @@ impl<'a> BinEntries<'a> { package: &'a NpmResolutionPackage, package_path: PathBuf, ) { + self.sorted = false; // check for a new collision, if we haven't already // found one match package.bin.as_ref().unwrap() { @@ -79,16 +95,21 @@ impl<'a> BinEntries<'a> { &str, // bin name &str, // bin script ) -> Result<(), AnyError>, + mut filter: impl FnMut(&NpmResolutionPackage) -> bool, ) -> Result<(), AnyError> { - if !self.collisions.is_empty() { + if !self.collisions.is_empty() && !self.sorted { // walking the dependency tree to find out the depth of each package // is sort of expensive, so we only do it if there's a collision sort_by_depth(snapshot, &mut self.entries, &mut self.collisions); + self.sorted = true; } let mut seen = HashSet::new(); for (package, package_path) in &self.entries { + if !filter(package) { + continue; + } if let Some(bin_entries) = &package.bin { match bin_entries { deno_npm::registry::NpmPackageVersionBinEntry::String(script) => { @@ -118,8 +139,8 @@ impl<'a> BinEntries<'a> { } /// Collect the bin entries into a vec of (name, script path) - pub fn into_bin_files( - mut self, + pub fn collect_bin_files( + &mut self, snapshot: &NpmResolutionSnapshot, ) -> Vec<(String, PathBuf)> { let mut bins = Vec::new(); @@ -131,17 +152,18 @@ impl<'a> BinEntries<'a> { bins.push((name.to_string(), package_path.join(script))); Ok(()) }, + |_| true, ) .unwrap(); bins } - /// Finish setting up the bin entries, writing the necessary files - /// to disk. - pub fn finish( + fn set_up_entries_filtered( mut self, snapshot: &NpmResolutionSnapshot, bin_node_modules_dir_path: &Path, + filter: impl FnMut(&NpmResolutionPackage) -> bool, + mut handler: impl FnMut(&EntrySetupOutcome<'_>), ) -> Result<(), AnyError> { if !self.entries.is_empty() && !bin_node_modules_dir_path.exists() { std::fs::create_dir_all(bin_node_modules_dir_path).with_context( @@ -160,18 +182,54 @@ impl<'a> BinEntries<'a> { Ok(()) }, |package, package_path, name, script| { - set_up_bin_entry( + let outcome = set_up_bin_entry( package, name, script, package_path, bin_node_modules_dir_path, - ) + )?; + handler(&outcome); + Ok(()) }, + filter, )?; Ok(()) } + + /// Finish setting up the bin entries, writing the necessary files + /// to disk. + pub fn finish( + self, + snapshot: &NpmResolutionSnapshot, + bin_node_modules_dir_path: &Path, + handler: impl FnMut(&EntrySetupOutcome<'_>), + ) -> Result<(), AnyError> { + self.set_up_entries_filtered( + snapshot, + bin_node_modules_dir_path, + |_| true, + handler, + ) + } + + /// Finish setting up the bin entries, writing the necessary files + /// to disk. + pub fn finish_only( + self, + snapshot: &NpmResolutionSnapshot, + bin_node_modules_dir_path: &Path, + handler: impl FnMut(&EntrySetupOutcome<'_>), + only: &HashSet<&NpmPackageId>, + ) -> Result<(), AnyError> { + self.set_up_entries_filtered( + snapshot, + bin_node_modules_dir_path, + |package| only.contains(&package.id), + handler, + ) + } } // walk the dependency tree to find out the depth of each package @@ -233,16 +291,17 @@ fn sort_by_depth( }); } -pub fn set_up_bin_entry( - package: &NpmResolutionPackage, - bin_name: &str, +pub fn set_up_bin_entry<'a>( + package: &'a NpmResolutionPackage, + bin_name: &'a str, #[allow(unused_variables)] bin_script: &str, - #[allow(unused_variables)] package_path: &Path, + #[allow(unused_variables)] package_path: &'a Path, bin_node_modules_dir_path: &Path, -) -> Result<(), AnyError> { +) -> Result, AnyError> { #[cfg(windows)] { set_up_bin_shim(package, bin_name, bin_node_modules_dir_path)?; + Ok(EntrySetupOutcome::Success) } #[cfg(unix)] { @@ -252,9 +311,8 @@ pub fn set_up_bin_entry( bin_script, package_path, bin_node_modules_dir_path, - )?; + ) } - Ok(()) } #[cfg(windows)] @@ -301,14 +359,39 @@ fn make_executable_if_exists(path: &Path) -> Result { Ok(true) } +pub enum EntrySetupOutcome<'a> { + #[cfg_attr(windows, allow(dead_code))] + MissingEntrypoint { + bin_name: &'a str, + package_path: &'a Path, + entrypoint: PathBuf, + package: &'a NpmResolutionPackage, + }, + Success, +} + +impl<'a> EntrySetupOutcome<'a> { + pub fn warn_if_failed(&self) { + match self { + EntrySetupOutcome::MissingEntrypoint { + bin_name, + package_path, + entrypoint, + .. + } => warn_missing_entrypoint(bin_name, package_path, entrypoint), + EntrySetupOutcome::Success => {} + } + } +} + #[cfg(unix)] -fn symlink_bin_entry( - _package: &NpmResolutionPackage, - bin_name: &str, +fn symlink_bin_entry<'a>( + package: &'a NpmResolutionPackage, + bin_name: &'a str, bin_script: &str, - package_path: &Path, + package_path: &'a Path, bin_node_modules_dir_path: &Path, -) -> Result<(), AnyError> { +) -> Result, AnyError> { use std::io; use std::os::unix::fs::symlink; let link = bin_node_modules_dir_path.join(bin_name); @@ -318,14 +401,12 @@ fn symlink_bin_entry( format!("Can't set up '{}' bin at {}", bin_name, original.display()) })?; if !found { - log::warn!( - "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.", - deno_terminal::colors::yellow("Warning"), + return Ok(EntrySetupOutcome::MissingEntrypoint { bin_name, - package_path.display(), - original.display() - ); - return Ok(()); + package_path, + entrypoint: original, + package, + }); } let original_relative = @@ -348,7 +429,7 @@ fn symlink_bin_entry( original_relative.display() ) })?; - return Ok(()); + return Ok(EntrySetupOutcome::Success); } return Err(err).with_context(|| { format!( @@ -359,5 +440,5 @@ fn symlink_bin_entry( }); } - Ok(()) + Ok(EntrySetupOutcome::Success) } diff --git a/cli/npm/managed/resolvers/common/lifecycle_scripts.rs b/cli/npm/managed/resolvers/common/lifecycle_scripts.rs index 5735f5248..5c5755c81 100644 --- a/cli/npm/managed/resolvers/common/lifecycle_scripts.rs +++ b/cli/npm/managed/resolvers/common/lifecycle_scripts.rs @@ -10,6 +10,7 @@ use deno_runtime::deno_io::FromRawIoHandle; use deno_semver::package::PackageNv; use deno_semver::Version; use std::borrow::Cow; +use std::collections::HashSet; use std::rc::Rc; use std::path::Path; @@ -61,7 +62,7 @@ impl<'a> LifecycleScripts<'a> { } } -fn has_lifecycle_scripts( +pub fn has_lifecycle_scripts( package: &NpmResolutionPackage, package_path: &Path, ) -> bool { @@ -83,7 +84,7 @@ fn is_broken_default_install_script(script: &str, package_path: &Path) -> bool { } impl<'a> LifecycleScripts<'a> { - fn can_run_scripts(&self, package_nv: &PackageNv) -> bool { + pub fn can_run_scripts(&self, package_nv: &PackageNv) -> bool { if !self.strategy.can_run_scripts() { return false; } @@ -98,6 +99,9 @@ impl<'a> LifecycleScripts<'a> { PackagesAllowedScripts::None => false, } } + pub fn has_run_scripts(&self, package: &NpmResolutionPackage) -> bool { + self.strategy.has_run(package) + } /// Register a package for running lifecycle scripts, if applicable. /// /// `package_path` is the path containing the package's code (its root dir). @@ -110,12 +114,12 @@ impl<'a> LifecycleScripts<'a> { ) { if has_lifecycle_scripts(package, &package_path) { if self.can_run_scripts(&package.id.nv) { - if !self.strategy.has_run(package) { + if !self.has_run_scripts(package) { self .packages_with_scripts .push((package, package_path.into_owned())); } - } else if !self.strategy.has_run(package) + } else if !self.has_run_scripts(package) && (self.config.explicit_install || !self.strategy.has_warned(package)) { // Skip adding `esbuild` as it is known that it can work properly without lifecycle script @@ -149,22 +153,32 @@ impl<'a> LifecycleScripts<'a> { self, snapshot: &NpmResolutionSnapshot, packages: &[NpmResolutionPackage], - root_node_modules_dir_path: Option<&Path>, + root_node_modules_dir_path: &Path, progress_bar: &ProgressBar, ) -> Result<(), AnyError> { self.warn_not_run_scripts()?; let get_package_path = |p: &NpmResolutionPackage| self.strategy.package_path(p); let mut failed_packages = Vec::new(); + let mut bin_entries = BinEntries::new(); if !self.packages_with_scripts.is_empty() { + let package_ids = self + .packages_with_scripts + .iter() + .map(|(p, _)| &p.id) + .collect::>(); // get custom commands for each bin available in the node_modules dir (essentially // the scripts that are in `node_modules/.bin`) - let base = - resolve_baseline_custom_commands(snapshot, packages, get_package_path)?; + let base = resolve_baseline_custom_commands( + &mut bin_entries, + snapshot, + packages, + get_package_path, + )?; let init_cwd = &self.config.initial_cwd; let process_state = crate::npm::managed::npm_process_state( snapshot.as_valid_serialized(), - root_node_modules_dir_path, + Some(root_node_modules_dir_path), ); let mut env_vars = crate::task_runner::real_env_vars(); @@ -221,7 +235,7 @@ impl<'a> LifecycleScripts<'a> { custom_commands: custom_commands.clone(), init_cwd, argv: &[], - root_node_modules_dir: root_node_modules_dir_path, + root_node_modules_dir: Some(root_node_modules_dir_path), stdio: Some(crate::task_runner::TaskIo { stderr: TaskStdio::piped(), stdout: TaskStdio::piped(), @@ -262,6 +276,17 @@ impl<'a> LifecycleScripts<'a> { } self.strategy.did_run_scripts(package)?; } + + // re-set up bin entries for the packages which we've run scripts for. + // lifecycle scripts can create files that are linked to by bin entries, + // and the only reliable way to handle this is to re-link bin entries + // (this is what PNPM does as well) + bin_entries.finish_only( + snapshot, + &root_node_modules_dir_path.join(".bin"), + |outcome| outcome.warn_if_failed(), + &package_ids, + )?; } if failed_packages.is_empty() { Ok(()) @@ -281,9 +306,10 @@ impl<'a> LifecycleScripts<'a> { // take in all (non copy) packages from snapshot, // and resolve the set of available binaries to create // custom commands available to the task runner -fn resolve_baseline_custom_commands( - snapshot: &NpmResolutionSnapshot, - packages: &[NpmResolutionPackage], +fn resolve_baseline_custom_commands<'a>( + bin_entries: &mut BinEntries<'a>, + snapshot: &'a NpmResolutionSnapshot, + packages: &'a [NpmResolutionPackage], get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf, ) -> Result { let mut custom_commands = crate::task_runner::TaskCustomCommands::new(); @@ -306,6 +332,7 @@ fn resolve_baseline_custom_commands( // doing it for packages that are set up already. // realistically, scripts won't be run very often so it probably isn't too big of an issue. resolve_custom_commands_from_packages( + bin_entries, custom_commands, snapshot, packages, @@ -320,12 +347,12 @@ fn resolve_custom_commands_from_packages< 'a, P: IntoIterator, >( + bin_entries: &mut BinEntries<'a>, mut commands: crate::task_runner::TaskCustomCommands, snapshot: &'a NpmResolutionSnapshot, packages: P, get_package_path: impl Fn(&'a NpmResolutionPackage) -> PathBuf, ) -> Result { - let mut bin_entries = BinEntries::new(); for package in packages { let package_path = get_package_path(package); @@ -333,7 +360,7 @@ fn resolve_custom_commands_from_packages< bin_entries.add(package, package_path); } } - let bins = bin_entries.into_bin_files(snapshot); + let bins: Vec<(String, PathBuf)> = bin_entries.collect_bin_files(snapshot); for (bin_name, script_path) in bins { commands.insert( bin_name.clone(), @@ -356,7 +383,9 @@ fn resolve_custom_commands_from_deps( snapshot: &NpmResolutionSnapshot, get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf, ) -> Result { + let mut bin_entries = BinEntries::new(); resolve_custom_commands_from_packages( + &mut bin_entries, baseline, snapshot, package diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index eddb0dc9b..50c5bd268 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -55,6 +55,7 @@ use crate::util::progress_bar::ProgressMessagePrompt; use super::super::cache::NpmCache; use super::super::cache::TarballCache; use super::super::resolution::NpmResolution; +use super::common::bin_entries; use super::common::NpmPackageFsResolver; use super::common::RegistryReadPermissionChecker; @@ -329,8 +330,7 @@ async fn sync_resolution_with_fs( let mut cache_futures = FuturesUnordered::new(); let mut newest_packages_by_name: HashMap<&String, &NpmResolutionPackage> = HashMap::with_capacity(package_partitions.packages.len()); - let bin_entries = - Rc::new(RefCell::new(super::common::bin_entries::BinEntries::new())); + let bin_entries = Rc::new(RefCell::new(bin_entries::BinEntries::new())); let mut lifecycle_scripts = super::common::lifecycle_scripts::LifecycleScripts::new( lifecycle_scripts, @@ -658,7 +658,28 @@ async fn sync_resolution_with_fs( // 7. Set up `node_modules/.bin` entries for packages that need it. { let bin_entries = std::mem::take(&mut *bin_entries.borrow_mut()); - bin_entries.finish(snapshot, &bin_node_modules_dir_path)?; + bin_entries.finish( + snapshot, + &bin_node_modules_dir_path, + |setup_outcome| { + match setup_outcome { + bin_entries::EntrySetupOutcome::MissingEntrypoint { + package, + package_path, + .. + } if super::common::lifecycle_scripts::has_lifecycle_scripts( + package, + package_path, + ) && lifecycle_scripts.can_run_scripts(&package.id.nv) + && !lifecycle_scripts.has_run_scripts(package) => + { + // ignore, it might get fixed when the lifecycle scripts run. + // if not, we'll warn then + } + outcome => outcome.warn_if_failed(), + } + }, + )?; } // 8. Create symlinks for the workspace packages @@ -708,7 +729,7 @@ async fn sync_resolution_with_fs( .finish( snapshot, &package_partitions.packages, - Some(root_node_modules_dir_path), + root_node_modules_dir_path, progress_bar, ) .await?; -- cgit v1.2.3 From 617350e79c58b6e01984e3d7c7436d243d0e5cff Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 14 Nov 2024 15:24:25 -0500 Subject: refactor(resolver): move more resolution code into deno_resolver (#26873) Follow-up to cjs refactor. This moves most of the resolution code into the deno_resolver crate. Still pending is the npm resolution code. --- cli/npm/byonm.rs | 30 +++++++++++++----------------- cli/npm/managed/mod.rs | 41 +++++++++++++++++++++++++---------------- cli/npm/mod.rs | 32 +++++++++++--------------------- 3 files changed, 49 insertions(+), 54 deletions(-) (limited to 'cli/npm') diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 45fa4cfd1..eca399251 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -2,19 +2,17 @@ use std::borrow::Cow; use std::path::Path; -use std::path::PathBuf; use std::sync::Arc; use deno_core::error::AnyError; use deno_core::serde_json; -use deno_core::url::Url; use deno_resolver::npm::ByonmNpmResolver; use deno_resolver::npm::ByonmNpmResolverCreateOptions; +use deno_resolver::npm::CliNpmReqResolver; use deno_runtime::deno_node::DenoFsNodeResolverEnv; use deno_runtime::deno_node::NodePermissions; use deno_runtime::ops::process::NpmProcessStateProvider; -use deno_semver::package::PackageReq; -use node_resolver::NpmResolver; +use node_resolver::NpmPackageFolderResolver; use crate::args::NpmProcessState; use crate::args::NpmProcessStateKind; @@ -22,7 +20,6 @@ use crate::resolver::CliDenoResolverFs; use super::CliNpmResolver; use super::InnerCliNpmResolverRef; -use super::ResolvePkgFolderFromDenoReqError; pub type CliByonmNpmResolverCreateOptions = ByonmNpmResolverCreateOptions; @@ -47,7 +44,13 @@ impl NpmProcessStateProvider for CliByonmWrapper { } impl CliNpmResolver for CliByonmNpmResolver { - fn into_npm_resolver(self: Arc) -> Arc { + fn into_npm_pkg_folder_resolver( + self: Arc, + ) -> Arc { + self + } + + fn into_npm_req_resolver(self: Arc) -> Arc { self } @@ -57,6 +60,10 @@ impl CliNpmResolver for CliByonmNpmResolver { Arc::new(CliByonmWrapper(self)) } + fn into_maybe_byonm(self: Arc) -> Option> { + Some(self) + } + fn clone_snapshotted(&self) -> Arc { Arc::new(self.clone()) } @@ -69,17 +76,6 @@ impl CliNpmResolver for CliByonmNpmResolver { self.root_node_modules_dir() } - fn resolve_pkg_folder_from_deno_module_req( - &self, - req: &PackageReq, - referrer: &Url, - ) -> Result { - ByonmNpmResolver::resolve_pkg_folder_from_deno_module_req( - self, req, referrer, - ) - .map_err(ResolvePkgFolderFromDenoReqError::Byonm) - } - fn ensure_read_permission<'a>( &self, permissions: &mut dyn NodePermissions, diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index 4a91bc347..2e64f5f18 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -22,6 +22,7 @@ use deno_npm::resolution::ValidSerializedNpmResolutionSnapshot; use deno_npm::NpmPackageId; use deno_npm::NpmResolutionPackage; use deno_npm::NpmSystemInfo; +use deno_resolver::npm::CliNpmReqResolver; use deno_runtime::colors; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; @@ -31,7 +32,7 @@ use deno_semver::package::PackageReq; use node_resolver::errors::PackageFolderResolveError; use node_resolver::errors::PackageFolderResolveIoError; use node_resolver::InNpmPackageChecker; -use node_resolver::NpmResolver; +use node_resolver::NpmPackageFolderResolver; use resolution::AddPkgReqsResult; use crate::args::CliLockfile; @@ -605,7 +606,7 @@ fn npm_process_state( .unwrap() } -impl NpmResolver for ManagedCliNpmResolver { +impl NpmPackageFolderResolver for ManagedCliNpmResolver { fn resolve_package_folder_from_package( &self, name: &str, @@ -635,8 +636,29 @@ impl NpmProcessStateProvider for ManagedCliNpmResolver { } } +impl CliNpmReqResolver for ManagedCliNpmResolver { + fn resolve_pkg_folder_from_deno_module_req( + &self, + req: &PackageReq, + _referrer: &ModuleSpecifier, + ) -> Result { + let pkg_id = self + .resolve_pkg_id_from_pkg_req(req) + .map_err(|err| ResolvePkgFolderFromDenoReqError::Managed(err.into()))?; + self + .resolve_pkg_folder_from_pkg_id(&pkg_id) + .map_err(ResolvePkgFolderFromDenoReqError::Managed) + } +} + impl CliNpmResolver for ManagedCliNpmResolver { - fn into_npm_resolver(self: Arc) -> Arc { + fn into_npm_pkg_folder_resolver( + self: Arc, + ) -> Arc { + self + } + + fn into_npm_req_resolver(self: Arc) -> Arc { self } @@ -687,19 +709,6 @@ impl CliNpmResolver for ManagedCliNpmResolver { self.fs_resolver.node_modules_path() } - fn resolve_pkg_folder_from_deno_module_req( - &self, - req: &PackageReq, - _referrer: &ModuleSpecifier, - ) -> Result { - let pkg_id = self - .resolve_pkg_id_from_pkg_req(req) - .map_err(|err| ResolvePkgFolderFromDenoReqError::Managed(err.into()))?; - self - .resolve_pkg_folder_from_pkg_id(&pkg_id) - .map_err(ResolvePkgFolderFromDenoReqError::Managed) - } - fn ensure_read_permission<'a>( &self, permissions: &mut dyn NodePermissions, diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 0d434ca27..0e955ac5b 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -6,19 +6,18 @@ mod managed; use std::borrow::Cow; use std::path::Path; -use std::path::PathBuf; use std::sync::Arc; use common::maybe_auth_header_for_npm_registry; use dashmap::DashMap; -use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; use deno_core::serde_json; use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; use deno_resolver::npm::ByonmInNpmPackageChecker; use deno_resolver::npm::ByonmNpmResolver; -use deno_resolver::npm::ByonmResolvePkgFolderFromDenoReqError; +use deno_resolver::npm::CliNpmReqResolver; +use deno_resolver::npm::ResolvePkgFolderFromDenoReqError; use deno_runtime::deno_node::NodePermissions; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageNv; @@ -26,8 +25,7 @@ use deno_semver::package::PackageReq; use managed::cache::registry_info::get_package_url; use managed::create_managed_in_npm_pkg_checker; use node_resolver::InNpmPackageChecker; -use node_resolver::NpmResolver; -use thiserror::Error; +use node_resolver::NpmPackageFolderResolver; use crate::file_fetcher::FileFetcher; @@ -38,14 +36,6 @@ pub use self::managed::CliManagedNpmResolverCreateOptions; pub use self::managed::CliNpmResolverManagedSnapshotOption; pub use self::managed::ManagedCliNpmResolver; -#[derive(Debug, Error)] -pub enum ResolvePkgFolderFromDenoReqError { - #[error(transparent)] - Managed(deno_core::error::AnyError), - #[error(transparent)] - Byonm(#[from] ByonmResolvePkgFolderFromDenoReqError), -} - pub enum CliNpmResolverCreateOptions { Managed(CliManagedNpmResolverCreateOptions), Byonm(CliByonmNpmResolverCreateOptions), @@ -95,11 +85,17 @@ pub enum InnerCliNpmResolverRef<'a> { Byonm(&'a CliByonmNpmResolver), } -pub trait CliNpmResolver: NpmResolver { - fn into_npm_resolver(self: Arc) -> Arc; +pub trait CliNpmResolver: NpmPackageFolderResolver + CliNpmReqResolver { + fn into_npm_pkg_folder_resolver( + self: Arc, + ) -> Arc; + fn into_npm_req_resolver(self: Arc) -> Arc; fn into_process_state_provider( self: Arc, ) -> Arc; + fn into_maybe_byonm(self: Arc) -> Option> { + None + } fn clone_snapshotted(&self) -> Arc; @@ -121,12 +117,6 @@ pub trait CliNpmResolver: NpmResolver { fn root_node_modules_path(&self) -> Option<&Path>; - fn resolve_pkg_folder_from_deno_module_req( - &self, - req: &PackageReq, - referrer: &ModuleSpecifier, - ) -> Result; - fn ensure_read_permission<'a>( &self, permissions: &mut dyn NodePermissions, -- cgit v1.2.3