From f396b3d1c8aaa7bf40fb1960f9ec81c3708ea2a8 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 4 Jul 2024 20:41:01 -0400 Subject: [PATCH] fix(publish): unfurling should always be done with the package json (#24435) Closes https://github.com/denoland/deno/issues/24430 --- .github/workflows/ci.generate.ts | 2 +- .github/workflows/ci.yml | 8 ++-- Cargo.lock | 35 ++++++++------- cli/args/mod.rs | 8 +--- cli/factory.rs | 12 ++++- cli/standalone/binary.rs | 36 +++++++-------- cli/tools/registry/mod.rs | 45 +++++++++---------- cli/tools/registry/unfurl.rs | 31 ++++++++----- cli/util/logger.rs | 1 + .../byonm_with_package_json/__test__.jsonc | 13 ++++++ .../publish/byonm_with_package_json/jsr.json | 12 +++++ .../byonm_with_package_json/package.json | 8 ++++ .../byonm_with_package_json/publish.out | 3 ++ .../byonm_with_package_json/src/index.ts | 3 ++ 14 files changed, 134 insertions(+), 83 deletions(-) create mode 100644 tests/specs/publish/byonm_with_package_json/__test__.jsonc create mode 100644 tests/specs/publish/byonm_with_package_json/jsr.json create mode 100644 tests/specs/publish/byonm_with_package_json/package.json create mode 100644 tests/specs/publish/byonm_with_package_json/publish.out create mode 100644 tests/specs/publish/byonm_with_package_json/src/index.ts diff --git a/.github/workflows/ci.generate.ts b/.github/workflows/ci.generate.ts index ba2e067f24..95b4688dc6 100755 --- a/.github/workflows/ci.generate.ts +++ b/.github/workflows/ci.generate.ts @@ -5,7 +5,7 @@ import { stringify } from "jsr:@std/yaml@^0.221/stringify"; // Bump this number when you want to purge the cache. // Note: the tools/release/01_bump_crate_versions.ts script will update this version // automatically via regex, so ensure that this line maintains this format. -const cacheVersion = 1; +const cacheVersion = 2; const ubuntuX86Runner = "ubuntu-22.04"; const ubuntuX86XlRunner = "ubuntu-22.04-xl"; diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f1014451d..061c794fd4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -367,8 +367,8 @@ jobs: path: |- ~/.cargo/registry/index ~/.cargo/registry/cache - key: '1-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}' - restore-keys: '1-cargo-home-${{ matrix.os }}-${{ matrix.arch }}' + key: '2-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}' + restore-keys: '2-cargo-home-${{ matrix.os }}-${{ matrix.arch }}' if: '!(matrix.skip)' - name: Restore cache build output (PR) uses: actions/cache/restore@v4 @@ -380,7 +380,7 @@ jobs: !./target/*/*.zip !./target/*/*.tar.gz key: never_saved - restore-keys: '1-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-' + restore-keys: '2-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-' - name: Apply and update mtime cache if: '!(matrix.skip) && (!startsWith(github.ref, ''refs/tags/''))' uses: ./.github/mtime_cache @@ -669,7 +669,7 @@ jobs: !./target/*/gn_out !./target/*/*.zip !./target/*/*.tar.gz - key: '1-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}' + key: '2-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}' publish-canary: name: publish canary runs-on: ubuntu-22.04 diff --git a/Cargo.lock b/Cargo.lock index 85f63ba37b..70a16de465 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1288,7 +1288,7 @@ dependencies = [ "indexmap", "log", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "serde", "serde_json", "sha2", @@ -1351,7 +1351,7 @@ dependencies = [ "futures", "libc", "memoffset 0.9.1", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "pin-project", "serde", "serde_json", @@ -1531,7 +1531,7 @@ dependencies = [ "log", "monch", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "regex", "serde", "serde_json", @@ -1591,7 +1591,7 @@ dependencies = [ "log", "once_cell", "os_pipe", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "rand", "tokio", "winapi", @@ -1947,10 +1947,11 @@ dependencies = [ [[package]] name = "deno_unsync" -version = "0.3.6" +version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "10eb3aaf83c3431d4215741140ec3a63b0c0edb972ee898c89bdf8462e9e136b" +checksum = "c3c8b95582c2023dbb66fccc37421b374026f5915fa507d437cb566904db9a3a" dependencies = [ + "parking_lot 0.12.3", "tokio", ] @@ -2716,7 +2717,7 @@ dependencies = [ "anyhow", "crossbeam-channel", "deno_terminal", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "regex", "thiserror", ] @@ -4593,9 +4594,9 @@ dependencies = [ [[package]] name = "parking_lot" -version = "0.12.1" +version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f" +checksum = "f1bf18183cf54e8d6059647fc3063646a1801cf30896933ec2311622cc4b9a27" dependencies = [ "lock_api", "parking_lot_core 0.9.9", @@ -6141,7 +6142,7 @@ checksum = "f91138e76242f575eb1d3b38b4f1362f10d3a43f47d182a5b359af488a02293b" dependencies = [ "new_debug_unreachable", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "phf_shared 0.10.0", "precomputed-hash", "serde", @@ -6237,7 +6238,7 @@ dependencies = [ "indexmap", "is-macro", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "petgraph", "radix_fmt", "relative-path", @@ -6798,7 +6799,7 @@ dependencies = [ "nix 0.26.2", "once_cell", "os_pipe", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "pretty_assertions", "prost", "prost-build", @@ -6921,7 +6922,7 @@ dependencies = [ "libc", "mio", "num_cpus", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "pin-project-lite", "signal-hook-registry", "socket2", @@ -7205,7 +7206,7 @@ dependencies = [ "ipconfig", "lru-cache", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "rand", "resolv-conf", "serde", @@ -7679,7 +7680,7 @@ dependencies = [ "log", "naga", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "profiling", "raw-window-handle", "ron", @@ -7721,7 +7722,7 @@ dependencies = [ "ndk-sys", "objc", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "profiling", "range-alloc", "raw-window-handle", @@ -8148,7 +8149,7 @@ dependencies = [ "log", "num-traits", "once_cell", - "parking_lot 0.12.1", + "parking_lot 0.12.3", "rand", "regex", "thiserror", diff --git a/cli/args/mod.rs b/cli/args/mod.rs index f747271b81..83f038ec02 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1060,6 +1060,7 @@ impl CliOptions { pub async fn create_workspace_resolver( &self, file_fetcher: &FileFetcher, + pkg_json_dep_resolution: PackageJsonDepResolution, ) -> Result { let overrode_no_import_map = self .overrides @@ -1102,12 +1103,7 @@ impl CliOptions { .workspace .create_resolver( CreateResolverOptions { - // todo(dsherret): this should be false for nodeModulesDir: true - pkg_json_dep_resolution: if self.use_byonm() { - PackageJsonDepResolution::Disabled - } else { - PackageJsonDepResolution::Enabled - }, + pkg_json_dep_resolution, specified_import_map: cli_arg_specified_import_map, }, |specifier| { diff --git a/cli/factory.rs b/cli/factory.rs index 62ab251f16..5b066c67fb 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -55,6 +55,7 @@ use std::collections::BTreeSet; use std::path::PathBuf; use deno_config::package_json::PackageJsonDepValue; +use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::WorkspaceResolver; use deno_config::ConfigFile; use deno_core::error::AnyError; @@ -458,7 +459,15 @@ impl CliFactory { .get_or_try_init_async(async { let resolver = self .options - .create_workspace_resolver(self.file_fetcher()?) + .create_workspace_resolver( + self.file_fetcher()?, + if self.options.use_byonm() { + PackageJsonDepResolution::Disabled + } else { + // todo(dsherret): this should be false for nodeModulesDir: true + PackageJsonDepResolution::Enabled + }, + ) .await?; if !resolver.diagnostics().is_empty() { warn!( @@ -759,6 +768,7 @@ impl CliFactory { self.file_fetcher()?, self.http_client_provider(), self.npm_resolver().await?.as_ref(), + self.workspace_resolver().await?.as_ref(), self.options.npm_system_info(), )) } diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index bf035577c9..c9371d853e 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -18,6 +18,7 @@ use std::process::Command; use deno_ast::ModuleSpecifier; use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::Workspace; +use deno_config::workspace::WorkspaceResolver; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; @@ -376,6 +377,7 @@ pub struct DenoCompileBinaryWriter<'a> { file_fetcher: &'a FileFetcher, http_client_provider: &'a HttpClientProvider, npm_resolver: &'a dyn CliNpmResolver, + workspace_resolver: &'a WorkspaceResolver, npm_system_info: NpmSystemInfo, } @@ -386,6 +388,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { file_fetcher: &'a FileFetcher, http_client_provider: &'a HttpClientProvider, npm_resolver: &'a dyn CliNpmResolver, + workspace_resolver: &'a WorkspaceResolver, npm_system_info: NpmSystemInfo, ) -> Self { Self { @@ -393,6 +396,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { file_fetcher, http_client_provider, npm_resolver, + workspace_resolver, npm_system_info, } } @@ -419,17 +423,15 @@ impl<'a> DenoCompileBinaryWriter<'a> { } set_windows_binary_to_gui(&mut original_binary)?; } - self - .write_standalone_binary( - writer, - original_binary, - eszip, - root_dir_url, - entrypoint, - cli_options, - compile_flags, - ) - .await + self.write_standalone_binary( + writer, + original_binary, + eszip, + root_dir_url, + entrypoint, + cli_options, + compile_flags, + ) } async fn get_base_binary( @@ -512,7 +514,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { /// This functions creates a standalone deno binary by appending a bundle /// and magic trailer to the currently executing binary. #[allow(clippy::too_many_arguments)] - async fn write_standalone_binary( + fn write_standalone_binary( &self, writer: &mut impl Write, original_bin: Vec, @@ -530,9 +532,6 @@ impl<'a> DenoCompileBinaryWriter<'a> { Some(CaData::Bytes(bytes)) => Some(bytes.clone()), None => None, }; - let workspace_resolver = cli_options - .create_workspace_resolver(self.file_fetcher) - .await?; let root_path = root_dir_url.inner().to_file_path().unwrap(); let (npm_vfs, npm_files, node_modules) = match self.npm_resolver.as_inner() { @@ -599,7 +598,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { ca_data, entrypoint_key: root_dir_url.specifier_key(entrypoint).into_owned(), workspace_resolver: SerializedWorkspaceResolver { - import_map: workspace_resolver.maybe_import_map().map(|i| { + import_map: self.workspace_resolver.maybe_import_map().map(|i| { SerializedWorkspaceResolverImportMap { specifier: if i.base_url().scheme() == "file" { root_dir_url.specifier_key(i.base_url()).into_owned() @@ -610,7 +609,8 @@ impl<'a> DenoCompileBinaryWriter<'a> { json: i.to_json(), } }), - package_jsons: workspace_resolver + package_jsons: self + .workspace_resolver .package_jsons() .map(|pkg_json| { ( @@ -621,7 +621,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { ) }) .collect(), - pkg_json_resolution: workspace_resolver.pkg_json_dep_resolution(), + pkg_json_resolution: self.workspace_resolver.pkg_json_dep_resolution(), }, node_modules, disable_deprecated_api_warning: cli_options diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index 134a973f7d..3f59f4e146 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -12,7 +12,7 @@ use base64::prelude::BASE64_STANDARD; use base64::Engine; use deno_ast::ModuleSpecifier; use deno_config::workspace::JsrPackageConfig; -use deno_config::workspace::WorkspaceResolver; +use deno_config::workspace::PackageJsonDepResolution; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; @@ -24,7 +24,6 @@ use deno_core::serde_json; use deno_core::serde_json::json; use deno_core::serde_json::Value; use deno_runtime::deno_fetch::reqwest; -use deno_runtime::deno_fs::FileSystem; use deno_terminal::colors; use lsp_types::Url; use serde::Deserialize; @@ -81,8 +80,6 @@ pub async fn publish( let auth_method = get_auth_method(publish_flags.token, publish_flags.dry_run)?; - let workspace_resolver = cli_factory.workspace_resolver().await?.clone(); - let directory_path = cli_factory.cli_options().initial_cwd(); let cli_options = cli_factory.cli_options(); let publish_configs = cli_options.workspace.jsr_packages_for_publish(); @@ -103,6 +100,20 @@ pub async fn publish( } } } + let specifier_unfurler = Arc::new(SpecifierUnfurler::new( + if cli_options.unstable_sloppy_imports() { + Some(SloppyImportsResolver::new(cli_factory.fs().clone())) + } else { + None + }, + cli_options + .create_workspace_resolver( + cli_factory.file_fetcher()?, + PackageJsonDepResolution::Enabled, + ) + .await?, + cli_options.unstable_bare_node_builtins(), + )); let diagnostics_collector = PublishDiagnosticsCollector::default(); let publish_preparer = PublishPreparer::new( @@ -110,9 +121,8 @@ pub async fn publish( cli_factory.module_graph_creator().await?.clone(), cli_factory.parsed_source_cache().clone(), cli_factory.type_checker().await?.clone(), - cli_factory.fs().clone(), cli_factory.cli_options().clone(), - workspace_resolver, + specifier_unfurler, ); let prepared_data = publish_preparer @@ -191,8 +201,7 @@ struct PublishPreparer { source_cache: Arc, type_checker: Arc, cli_options: Arc, - sloppy_imports_resolver: Option>, - workspace_resolver: Arc, + specifier_unfurler: Arc, } impl PublishPreparer { @@ -201,23 +210,16 @@ impl PublishPreparer { module_graph_creator: Arc, source_cache: Arc, type_checker: Arc, - fs: Arc, cli_options: Arc, - workspace_resolver: Arc, + specifier_unfurler: Arc, ) -> Self { - let sloppy_imports_resolver = if cli_options.unstable_sloppy_imports() { - Some(Arc::new(SloppyImportsResolver::new(fs.clone()))) - } else { - None - }; Self { graph_diagnostics_collector, module_graph_creator, source_cache, type_checker, cli_options, - sloppy_imports_resolver, - workspace_resolver, + specifier_unfurler, } } @@ -432,18 +434,11 @@ impl PublishPreparer { let tarball = deno_core::unsync::spawn_blocking({ let diagnostics_collector = diagnostics_collector.clone(); - let workspace_resolver = self.workspace_resolver.clone(); - let sloppy_imports_resolver = self.sloppy_imports_resolver.clone(); + let unfurler = self.specifier_unfurler.clone(); let cli_options = self.cli_options.clone(); let source_cache = self.source_cache.clone(); let config_path = config_path.clone(); move || { - let bare_node_builtins = cli_options.unstable_bare_node_builtins(); - let unfurler = SpecifierUnfurler::new( - sloppy_imports_resolver.as_deref(), - &workspace_resolver, - bare_node_builtins, - ); let root_specifier = ModuleSpecifier::from_directory_path(&root_dir).unwrap(); let publish_paths = diff --git a/cli/tools/registry/unfurl.rs b/cli/tools/registry/unfurl.rs index 147b59f30c..758db07964 100644 --- a/cli/tools/registry/unfurl.rs +++ b/cli/tools/registry/unfurl.rs @@ -5,6 +5,7 @@ use deno_ast::SourceRange; use deno_ast::SourceTextInfo; use deno_config::package_json::PackageJsonDepValue; use deno_config::workspace::MappedResolution; +use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::WorkspaceResolver; use deno_core::ModuleSpecifier; use deno_graph::DependencyDescriptor; @@ -40,18 +41,22 @@ impl SpecifierUnfurlerDiagnostic { } } -pub struct SpecifierUnfurler<'a> { - sloppy_imports_resolver: Option<&'a SloppyImportsResolver>, - workspace_resolver: &'a WorkspaceResolver, +pub struct SpecifierUnfurler { + sloppy_imports_resolver: Option, + workspace_resolver: WorkspaceResolver, bare_node_builtins: bool, } -impl<'a> SpecifierUnfurler<'a> { +impl SpecifierUnfurler { pub fn new( - sloppy_imports_resolver: Option<&'a SloppyImportsResolver>, - workspace_resolver: &'a WorkspaceResolver, + sloppy_imports_resolver: Option, + workspace_resolver: WorkspaceResolver, bare_node_builtins: bool, ) -> Self { + debug_assert_eq!( + workspace_resolver.pkg_json_dep_resolution(), + PackageJsonDepResolution::Enabled + ); Self { sloppy_imports_resolver, workspace_resolver, @@ -136,7 +141,7 @@ impl<'a> SpecifierUnfurler<'a> { // resolved // }; let resolved = - if let Some(sloppy_imports_resolver) = self.sloppy_imports_resolver { + if let Some(sloppy_imports_resolver) = &self.sloppy_imports_resolver { sloppy_imports_resolver .resolve(&resolved, deno_graph::source::ResolutionMode::Execution) .as_specifier() @@ -148,6 +153,12 @@ impl<'a> SpecifierUnfurler<'a> { if relative_resolved == specifier { None // nothing to unfurl } else { + log::debug!( + "Unfurled specifier: {} from {} -> {}", + specifier, + referrer, + relative_resolved + ); Some(relative_resolved) } } @@ -395,11 +406,9 @@ mod tests { deno_config::workspace::PackageJsonDepResolution::Enabled, ); let fs = Arc::new(RealFs); - let sloppy_imports_resolver = SloppyImportsResolver::new(fs); - let unfurler = SpecifierUnfurler::new( - Some(&sloppy_imports_resolver), - &workspace_resolver, + Some(SloppyImportsResolver::new(fs)), + workspace_resolver, true, ); diff --git a/cli/util/logger.rs b/cli/util/logger.rs index 3cd0cbe5d2..f3510c5020 100644 --- a/cli/util/logger.rs +++ b/cli/util/logger.rs @@ -41,6 +41,7 @@ pub fn init(maybe_level: Option) { // wgpu crates (gfx_backend), have a lot of useless INFO and WARN logs .filter_module("wgpu", log::LevelFilter::Error) .filter_module("gfx", log::LevelFilter::Error) + .filter_module("globset", log::LevelFilter::Error) // used to make available the lsp_debug which is then filtered out at runtime // in the cli logger .filter_module("deno::lsp::performance", log::LevelFilter::Debug) diff --git a/tests/specs/publish/byonm_with_package_json/__test__.jsonc b/tests/specs/publish/byonm_with_package_json/__test__.jsonc new file mode 100644 index 0000000000..77c23bae12 --- /dev/null +++ b/tests/specs/publish/byonm_with_package_json/__test__.jsonc @@ -0,0 +1,13 @@ +{ + "envs": { + "DENO_FUTURE": "1" + }, + "tempDir": true, + "steps": [{ + "args": "install", + "output": "[WILDCARD]" + }, { + "args": "publish --log-level=debug --dry-run --allow-dirty", + "output": "publish.out" + }] +} diff --git a/tests/specs/publish/byonm_with_package_json/jsr.json b/tests/specs/publish/byonm_with_package_json/jsr.json new file mode 100644 index 0000000000..c92b1a60c3 --- /dev/null +++ b/tests/specs/publish/byonm_with_package_json/jsr.json @@ -0,0 +1,12 @@ +{ + "name": "@scope/package", + "version": "0.0.0", + "exports": { + ".": "./src/index.ts" + }, + "publish": { + "include": [ + "src/**/*" + ] + } +} diff --git a/tests/specs/publish/byonm_with_package_json/package.json b/tests/specs/publish/byonm_with_package_json/package.json new file mode 100644 index 0000000000..eb93c075e5 --- /dev/null +++ b/tests/specs/publish/byonm_with_package_json/package.json @@ -0,0 +1,8 @@ +{ + "name": "@scope/pkg", + "module": "src/index.ts", + "type": "module", + "dependencies": { + "@denotest/add": "1" + } +} diff --git a/tests/specs/publish/byonm_with_package_json/publish.out b/tests/specs/publish/byonm_with_package_json/publish.out new file mode 100644 index 0000000000..6ce644bac6 --- /dev/null +++ b/tests/specs/publish/byonm_with_package_json/publish.out @@ -0,0 +1,3 @@ +[WILDCARD]Unfurled specifier: @denotest/add from [WILDLINE]/src/index.ts -> npm:@denotest/add@1 +[WILDCARD] +Warning Aborting due to --dry-run diff --git a/tests/specs/publish/byonm_with_package_json/src/index.ts b/tests/specs/publish/byonm_with_package_json/src/index.ts new file mode 100644 index 0000000000..1ca631410f --- /dev/null +++ b/tests/specs/publish/byonm_with_package_json/src/index.ts @@ -0,0 +1,3 @@ +import { add } from "@denotest/add"; + +console.log(add(1, 2));