From 9ac405d587ca1465debd4a65a09324b7a6b2c04f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 29 Nov 2023 09:32:23 -0500 Subject: [PATCH] feat(compile): support "bring your own node_modules" in deno compile (#21377) Not tested thoroughly. This is a good start. Closes #21350 --- cli/factory.rs | 11 +- cli/npm/byonm.rs | 11 +- cli/npm/managed/mod.rs | 12 +- cli/npm/managed/resolvers/common.rs | 2 +- cli/npm/managed/resolvers/global.rs | 2 +- cli/npm/managed/resolvers/local.rs | 15 +-- cli/npm/mod.rs | 2 +- cli/standalone/binary.rs | 93 +++++++++----- cli/standalone/mod.rs | 161 ++++++++++++++++--------- cli/standalone/virtual_fs.rs | 10 +- cli/tests/integration/compile_tests.rs | 33 ++++- cli/tools/task.rs | 2 +- cli/util/fs.rs | 12 +- 13 files changed, 245 insertions(+), 121 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 5ea3e2d0bf..fc4819d648 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -43,6 +43,7 @@ use crate::resolver::CliGraphResolverOptions; use crate::standalone::DenoCompileBinaryWriter; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; +use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; use crate::worker::CliMainWorkerFactory; @@ -306,9 +307,13 @@ impl CliFactory { create_cli_npm_resolver(if self.options.unstable_byonm() { CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions { fs: fs.clone(), - // todo(byonm): actually resolve this properly because the package.json - // might be in an ancestor directory - root_node_modules_dir: self.options.initial_cwd().join("node_modules"), + root_node_modules_dir: match self.options.node_modules_dir_path().clone() { + Some(node_modules_path) => node_modules_path, + // path needs to be canonicalized for node resolution + // (node_modules_dir_path above is already canonicalized) + None => canonicalize_path_maybe_not_exists(self.options.initial_cwd())? + .join("node_modules"), + }, }) } else { CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 469f98828e..aeb1c28b82 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -19,7 +19,7 @@ use deno_semver::package::PackageReq; use crate::args::package_json::get_local_package_json_version_reqs; use crate::args::NpmProcessState; use crate::args::NpmProcessStateKind; -use crate::util::fs::canonicalize_path_maybe_not_exists; +use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; use crate::util::path::specifier_to_file_path; use super::common::types_package_name; @@ -188,8 +188,8 @@ impl CliNpmResolver for ByonmCliNpmResolver { InnerCliNpmResolverRef::Byonm(self) } - fn root_node_modules_path(&self) -> Option { - Some(self.root_node_modules_dir.clone()) + fn root_node_modules_path(&self) -> Option<&PathBuf> { + Some(&self.root_node_modules_dir) } fn resolve_pkg_folder_from_deno_module_req( @@ -215,7 +215,10 @@ impl CliNpmResolver for ByonmCliNpmResolver { .unwrap() .join("node_modules") .join(key); - return Ok(canonicalize_path_maybe_not_exists(&package_path)?); + return Ok(canonicalize_path_maybe_not_exists_with_fs( + &package_path, + fs, + )?); } } } diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index 6cf7e6a32b..ba22780454 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -288,12 +288,8 @@ impl ManagedCliNpmResolver { pkg_id: &NpmPackageId, ) -> Result { let path = self.fs_resolver.package_folder(pkg_id)?; - let path = canonicalize_path_maybe_not_exists_with_fs(&path, |path| { - self - .fs - .realpath_sync(path) - .map_err(|err| err.into_io_error()) - })?; + let path = + canonicalize_path_maybe_not_exists_with_fs(&path, self.fs.as_ref())?; log::debug!( "Resolved package folder of {} to {}", pkg_id.as_serialized(), @@ -560,7 +556,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { &self.progress_bar, self.api.base_url().clone(), npm_resolution, - self.root_node_modules_path(), + self.root_node_modules_path().map(ToOwned::to_owned), self.npm_system_info.clone(), ), self.global_npm_cache.clone(), @@ -575,7 +571,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { InnerCliNpmResolverRef::Managed(self) } - fn root_node_modules_path(&self) -> Option { + fn root_node_modules_path(&self) -> Option<&PathBuf> { self.fs_resolver.node_modules_path() } diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index 41b5d8a963..9fc5893fcb 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -29,7 +29,7 @@ pub trait NpmPackageFsResolver: Send + Sync { fn root_dir_url(&self) -> &Url; /// The local node_modules folder if it is applicable to the implementation. - fn node_modules_path(&self) -> Option; + fn node_modules_path(&self) -> Option<&PathBuf>; fn package_folder( &self, diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs index 7a51cead25..e62d9021ca 100644 --- a/cli/npm/managed/resolvers/global.rs +++ b/cli/npm/managed/resolvers/global.rs @@ -75,7 +75,7 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { self.cache.root_dir_url() } - fn node_modules_path(&self) -> Option { + fn node_modules_path(&self) -> Option<&PathBuf> { None } diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index a4a8550f12..4051c9c319 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -122,14 +122,9 @@ impl LocalNpmPackageResolver { }; // Canonicalize the path so it's not pointing to the symlinked directory // in `node_modules` directory of the referrer. - canonicalize_path_maybe_not_exists_with_fs(&path, |path| { - self - .fs - .realpath_sync(path) - .map_err(|err| err.into_io_error()) - }) - .map(Some) - .map_err(|err| err.into()) + canonicalize_path_maybe_not_exists_with_fs(&path, self.fs.as_ref()) + .map(Some) + .map_err(|err| err.into()) } } @@ -139,8 +134,8 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver { &self.root_node_modules_url } - fn node_modules_path(&self) -> Option { - Some(self.root_node_modules_path.clone()) + fn node_modules_path(&self) -> Option<&PathBuf> { + Some(&self.root_node_modules_path) } fn package_folder(&self, id: &NpmPackageId) -> Result { diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index f4ea08186f..474f493d5a 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -75,7 +75,7 @@ pub trait CliNpmResolver: NpmResolver { } } - fn root_node_modules_path(&self) -> Option; + fn root_node_modules_path(&self) -> Option<&PathBuf>; fn resolve_pkg_folder_from_deno_module_req( &self, diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index 38ef3648e4..d1a5863eed 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -122,6 +122,18 @@ impl SerializablePackageJsonDeps { } } +#[derive(Deserialize, Serialize)] +pub enum NodeModules { + Managed { + /// Whether this uses a node_modules directory (true) or the global cache (false). + node_modules_dir: bool, + package_json_deps: Option, + }, + Byonm { + package_json_deps: Option, + }, +} + #[derive(Deserialize, Serialize)] pub struct Metadata { pub argv: Vec, @@ -136,9 +148,7 @@ pub struct Metadata { pub unsafely_ignore_certificate_errors: Option>, pub maybe_import_map: Option<(Url, String)>, pub entrypoint: ModuleSpecifier, - /// Whether this uses a node_modules directory (true) or the global cache (false). - pub node_modules_dir: bool, - pub package_json_deps: Option, + pub node_modules: Option, } pub fn load_npm_vfs(root_dir_path: PathBuf) -> Result { @@ -490,23 +500,44 @@ impl<'a> DenoCompileBinaryWriter<'a> { .resolve_import_map(self.file_fetcher) .await? .map(|import_map| (import_map.base_url().clone(), import_map.to_json())); - let (npm_vfs, npm_files) = match self.npm_resolver.as_inner() { - InnerCliNpmResolverRef::Managed(managed) => { - let snapshot = - managed.serialized_valid_snapshot_for_system(&self.npm_system_info); - if !snapshot.as_serialized().packages.is_empty() { - let (root_dir, files) = self.build_vfs()?.into_dir_and_files(); - eszip.add_npm_snapshot(snapshot); - (Some(root_dir), files) - } else { - (None, Vec::new()) + let (npm_vfs, npm_files, node_modules) = + match self.npm_resolver.as_inner() { + InnerCliNpmResolverRef::Managed(managed) => { + let snapshot = + managed.serialized_valid_snapshot_for_system(&self.npm_system_info); + if !snapshot.as_serialized().packages.is_empty() { + let (root_dir, files) = self.build_vfs()?.into_dir_and_files(); + eszip.add_npm_snapshot(snapshot); + ( + Some(root_dir), + files, + Some(NodeModules::Managed { + node_modules_dir: self + .npm_resolver + .root_node_modules_path() + .is_some(), + package_json_deps: self.package_json_deps_provider.deps().map( + |deps| SerializablePackageJsonDeps::from_deps(deps.clone()), + ), + }), + ) + } else { + (None, Vec::new(), None) + } } - } - InnerCliNpmResolverRef::Byonm(_) => { - let (root_dir, files) = self.build_vfs()?.into_dir_and_files(); - (Some(root_dir), files) - } - }; + InnerCliNpmResolverRef::Byonm(_) => { + let (root_dir, files) = self.build_vfs()?.into_dir_and_files(); + ( + Some(root_dir), + files, + Some(NodeModules::Byonm { + package_json_deps: self.package_json_deps_provider.deps().map( + |deps| SerializablePackageJsonDeps::from_deps(deps.clone()), + ), + }), + ) + } + }; let metadata = Metadata { argv: compile_flags.args.clone(), @@ -523,11 +554,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { ca_data, entrypoint: entrypoint.clone(), maybe_import_map, - node_modules_dir: self.npm_resolver.root_node_modules_path().is_some(), - package_json_deps: self - .package_json_deps_provider - .deps() - .map(|deps| SerializablePackageJsonDeps::from_deps(deps.clone())), + node_modules, }; write_binary_bytes( @@ -545,7 +572,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { InnerCliNpmResolverRef::Managed(npm_resolver) => { if let Some(node_modules_path) = npm_resolver.root_node_modules_path() { let mut builder = VfsBuilder::new(node_modules_path.clone())?; - builder.add_dir_recursive(&node_modules_path)?; + builder.add_dir_recursive(node_modules_path)?; Ok(builder) } else { // DO NOT include the user's registry url as it may contain credentials, @@ -565,9 +592,19 @@ impl<'a> DenoCompileBinaryWriter<'a> { Ok(builder) } } - InnerCliNpmResolverRef::Byonm(_) => { - // todo(#18967): should use the node_modules directory - todo!() + InnerCliNpmResolverRef::Byonm(npm_resolver) => { + // the root_node_modules directory will always exist for byonm + let node_modules_path = npm_resolver.root_node_modules_path().unwrap(); + let parent_path = node_modules_path.parent().unwrap(); + let mut builder = VfsBuilder::new(parent_path.to_path_buf())?; + let package_json_path = parent_path.join("package.json"); + if package_json_path.exists() { + builder.add_file_at_path(&package_json_path)?; + } + if node_modules_path.exists() { + builder.add_dir_recursive(node_modules_path)?; + } + Ok(builder) } } } diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 13123a8d63..a6bf128624 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -16,6 +16,7 @@ use crate::module_loader::CliNodeResolver; use crate::module_loader::NpmModuleLoader; use crate::node::CliCjsCodeAnalyzer; use crate::npm::create_cli_npm_resolver; +use crate::npm::CliNpmResolverByonmCreateOptions; use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedCreateOptions; use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; @@ -311,61 +312,113 @@ pub async fn run( .join("node_modules"); let npm_cache_dir = NpmCacheDir::new(root_path.clone()); let npm_global_cache_dir = npm_cache_dir.get_cache_location(); - let (fs, vfs_root, maybe_node_modules_path, maybe_snapshot) = - if let Some(snapshot) = eszip.take_npm_snapshot() { - let vfs_root_dir_path = if metadata.node_modules_dir { - root_path - } else { - npm_cache_dir.registry_folder(&npm_registry_url) - }; - let vfs = load_npm_vfs(vfs_root_dir_path.clone()) - .context("Failed to load npm vfs.")?; - let node_modules_path = if metadata.node_modules_dir { - Some(vfs.root().to_path_buf()) - } else { - None - }; - ( - Arc::new(DenoCompileFileSystem::new(vfs)) - as Arc, - Some(vfs_root_dir_path), - node_modules_path, - Some(snapshot), - ) - } else { - ( - Arc::new(deno_fs::RealFs) as Arc, - None, - None, - None, - ) + let cache_setting = CacheSetting::Only; + let (package_json_deps_provider, fs, npm_resolver, maybe_vfs_root) = + match metadata.node_modules { + Some(binary::NodeModules::Managed { + node_modules_dir, + package_json_deps, + }) => { + // this will always have a snapshot + let snapshot = eszip.take_npm_snapshot().unwrap(); + let vfs_root_dir_path = if node_modules_dir { + root_path + } else { + npm_cache_dir.registry_folder(&npm_registry_url) + }; + let vfs = load_npm_vfs(vfs_root_dir_path.clone()) + .context("Failed to load npm vfs.")?; + let maybe_node_modules_path = if node_modules_dir { + Some(vfs.root().to_path_buf()) + } else { + None + }; + let package_json_deps_provider = + Arc::new(PackageJsonDepsProvider::new( + package_json_deps.map(|serialized| serialized.into_deps()), + )); + let fs = Arc::new(DenoCompileFileSystem::new(vfs)) + as Arc; + let npm_resolver = create_cli_npm_resolver( + CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { + snapshot: CliNpmResolverManagedSnapshotOption::Specified(Some(snapshot)), + maybe_lockfile: None, + fs: fs.clone(), + http_client: http_client.clone(), + npm_global_cache_dir, + cache_setting, + text_only_progress_bar: progress_bar, + maybe_node_modules_path, + package_json_installer: + CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( + package_json_deps_provider.clone(), + ), + npm_registry_url, + npm_system_info: Default::default(), + }), + ) + .await?; + ( + package_json_deps_provider, + fs, + npm_resolver, + Some(vfs_root_dir_path), + ) + } + Some(binary::NodeModules::Byonm { package_json_deps }) => { + let vfs_root_dir_path = root_path; + let vfs = load_npm_vfs(vfs_root_dir_path.clone()) + .context("Failed to load npm vfs.")?; + let node_modules_path = vfs.root().join("node_modules"); + let package_json_deps_provider = + Arc::new(PackageJsonDepsProvider::new( + package_json_deps.map(|serialized| serialized.into_deps()), + )); + let fs = Arc::new(DenoCompileFileSystem::new(vfs)) + as Arc; + let npm_resolver = + create_cli_npm_resolver(CliNpmResolverCreateOptions::Byonm( + CliNpmResolverByonmCreateOptions { + fs: fs.clone(), + root_node_modules_dir: node_modules_path, + }, + )) + .await?; + ( + package_json_deps_provider, + fs, + npm_resolver, + Some(vfs_root_dir_path), + ) + } + None => { + let package_json_deps_provider = + Arc::new(PackageJsonDepsProvider::new(None)); + let fs = Arc::new(deno_fs::RealFs) as Arc; + let npm_resolver = create_cli_npm_resolver( + CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { + snapshot: CliNpmResolverManagedSnapshotOption::Specified(None), + maybe_lockfile: None, + fs: fs.clone(), + http_client: http_client.clone(), + npm_global_cache_dir, + cache_setting, + text_only_progress_bar: progress_bar, + maybe_node_modules_path: None, + package_json_installer: + CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( + package_json_deps_provider.clone(), + ), + npm_registry_url, + npm_system_info: Default::default(), + }), + ) + .await?; + (package_json_deps_provider, fs, npm_resolver, None) + } }; - let has_node_modules_dir = maybe_node_modules_path.is_some(); - let package_json_deps_provider = Arc::new(PackageJsonDepsProvider::new( - metadata - .package_json_deps - .map(|serialized| serialized.into_deps()), - )); - let npm_resolver = create_cli_npm_resolver( - CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { - snapshot: CliNpmResolverManagedSnapshotOption::Specified(maybe_snapshot), - maybe_lockfile: None, - fs: fs.clone(), - http_client: http_client.clone(), - npm_global_cache_dir, - cache_setting: CacheSetting::Only, - text_only_progress_bar: progress_bar, - maybe_node_modules_path, - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( - package_json_deps_provider.clone(), - ), - npm_registry_url, - npm_system_info: Default::default(), - }), - ) - .await?; + let has_node_modules_dir = npm_resolver.root_node_modules_path().is_some(); let node_resolver = Arc::new(NodeResolver::new( fs.clone(), npm_resolver.clone().into_npm_resolver(), @@ -409,7 +462,7 @@ pub async fn run( let permissions = { let mut permissions = metadata.permissions; // if running with an npm vfs, grant read access to it - if let Some(vfs_root) = vfs_root { + if let Some(vfs_root) = maybe_vfs_root { match &mut permissions.allow_read { Some(vec) if vec.is_empty() => { // do nothing, already granted diff --git a/cli/standalone/virtual_fs.rs b/cli/standalone/virtual_fs.rs index c96aed1434..ee870611b2 100644 --- a/cli/standalone/virtual_fs.rs +++ b/cli/standalone/virtual_fs.rs @@ -92,9 +92,7 @@ impl VfsBuilder { if file_type.is_dir() { self.add_dir_recursive_internal(&path)?; } else if file_type.is_file() { - let file_bytes = std::fs::read(&path) - .with_context(|| format!("Reading {}", path.display()))?; - self.add_file(&path, file_bytes)?; + self.add_file_at_path(&path)?; } else if file_type.is_symlink() { let target = util::fs::canonicalize_path(&path) .with_context(|| format!("Reading symlink {}", path.display()))?; @@ -163,6 +161,12 @@ impl VfsBuilder { Ok(current_dir) } + pub fn add_file_at_path(&mut self, path: &Path) -> Result<(), AnyError> { + let file_bytes = std::fs::read(path) + .with_context(|| format!("Reading {}", path.display()))?; + self.add_file(path, file_bytes) + } + fn add_file(&mut self, path: &Path, data: Vec) -> Result<(), AnyError> { log::debug!("Adding file '{}'", path.display()); let checksum = util::checksum::gen(&[&data]); diff --git a/cli/tests/integration/compile_tests.rs b/cli/tests/integration/compile_tests.rs index 1d0a36145a..df339c3690 100644 --- a/cli/tests/integration/compile_tests.rs +++ b/cli/tests/integration/compile_tests.rs @@ -798,15 +798,36 @@ testing[WILDCARD]this r#"{ "dependencies": { "@denotest/esm-basic": "1" } }"#, ); - let output = context + context .new_command() .args("compile --output binary main.ts") - .run(); - output.assert_exit_code(0); - output.skip_output_check(); + .run() + .assert_exit_code(0) + .skip_output_check(); - let output = context.new_command().name(binary_path).run(); - output.assert_matches_text("2\n"); + context + .new_command() + .name(&binary_path) + .run() + .assert_matches_text("2\n"); + + // now try with byonm + temp_dir.remove_dir_all("node_modules"); + temp_dir.write("deno.json", r#"{"unstable":["byonm"]}"#); + context.run_npm("install"); + + context + .new_command() + .args("compile --output binary main.ts") + .run() + .assert_exit_code(0) + .assert_matches_text("Check file:///[WILDCARD]/main.ts\nCompile file:///[WILDCARD]/main.ts to binary[WILDCARD]\n"); + + context + .new_command() + .name(&binary_path) + .run() + .assert_matches_text("2\n"); } #[test] diff --git a/cli/tools/task.rs b/cli/tools/task.rs index f3acd1b195..d929dc6662 100644 --- a/cli/tools/task.rs +++ b/cli/tools/task.rs @@ -122,7 +122,7 @@ pub async fn execute_script( None => Default::default(), }; let env_vars = match npm_resolver.root_node_modules_path() { - Some(dir_path) => collect_env_vars_with_node_modules_dir(&dir_path), + Some(dir_path) => collect_env_vars_with_node_modules_dir(dir_path), None => collect_env_vars(), }; let local = LocalSet::new(); diff --git a/cli/util/fs.rs b/cli/util/fs.rs index 33eb3af9d7..4881d08153 100644 --- a/cli/util/fs.rs +++ b/cli/util/fs.rs @@ -6,6 +6,7 @@ pub use deno_core::normalize_path; use deno_core::unsync::spawn_blocking; use deno_core::ModuleSpecifier; use deno_runtime::deno_crypto::rand; +use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::PathClean; use std::borrow::Cow; use std::env::current_dir; @@ -187,10 +188,19 @@ pub fn canonicalize_path(path: &Path) -> Result { pub fn canonicalize_path_maybe_not_exists( path: &Path, ) -> Result { - canonicalize_path_maybe_not_exists_with_fs(path, canonicalize_path) + canonicalize_path_maybe_not_exists_with_custom_fn(path, canonicalize_path) } pub fn canonicalize_path_maybe_not_exists_with_fs( + path: &Path, + fs: &dyn FileSystem, +) -> Result { + canonicalize_path_maybe_not_exists_with_custom_fn(path, |path| { + fs.realpath_sync(path).map_err(|err| err.into_io_error()) + }) +} + +fn canonicalize_path_maybe_not_exists_with_custom_fn( path: &Path, canonicalize: impl Fn(&Path) -> Result, ) -> Result {