From 5dea510b021d78a2c9b6aef9462ae6f4e0fd527a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 2 Nov 2022 16:32:30 +0100 Subject: [PATCH] fix(lock): autodiscovery of lockfile (#16498) This commit adds autodiscovery of lockfile. This only happens if Deno discovers the configuration file (either "deno.json" or "deno.jsonc"). In such case Deno tries to load "deno.lock" file that sits next to the configuration file, or creates one for user if the lockfile doesn't exist yet. As a consequence, "--lock" and "--lock-write" flags had been updated. "--lock" no longer requires a value, if one is not provided, it defaults to "./deno.lock" resolved from the current working directory. "--lock-write" description was updated to say that it forces to overwrite a lockfile. Autodiscovery is currently not handled by the LSP. --- cli/args/flags.rs | 68 ++++++++- cli/args/mod.rs | 25 ++-- cli/lockfile.rs | 132 ++++++++++++++---- cli/lsp/language_server.rs | 2 + cli/proc_state.rs | 12 +- cli/tests/integration/npm_tests.rs | 48 +++++++ cli/tests/integration/run_tests.rs | 13 +- cli/tests/testdata/jsx/deno.lock | 7 + cli/tests/testdata/npm/lock_file/main.out | 11 +- .../run/auto_discover_lockfile/deno.json | 3 + .../run/auto_discover_lockfile/deno.lock | 7 + .../run/auto_discover_lockfile/main.out | 5 + .../run/auto_discover_lockfile/main.ts | 1 + cli/tests/testdata/run/config_types/deno.lock | 6 + .../testdata/run/lock_write_requires_lock.out | 3 - 15 files changed, 283 insertions(+), 60 deletions(-) create mode 100644 cli/tests/testdata/jsx/deno.lock create mode 100644 cli/tests/testdata/run/auto_discover_lockfile/deno.json create mode 100644 cli/tests/testdata/run/auto_discover_lockfile/deno.lock create mode 100644 cli/tests/testdata/run/auto_discover_lockfile/main.out create mode 100644 cli/tests/testdata/run/auto_discover_lockfile/main.ts create mode 100644 cli/tests/testdata/run/config_types/deno.lock delete mode 100644 cli/tests/testdata/run/lock_write_requires_lock.out diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 90899704e6..d024f65767 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -458,7 +458,9 @@ impl Flags { Some(files.clone()) } else if let Run(RunFlags { script }) = &self.subcommand { if let Ok(module_specifier) = deno_core::resolve_url_or_path(script) { - if module_specifier.scheme() == "file" { + if module_specifier.scheme() == "file" + || module_specifier.scheme() == "npm" + { if let Ok(p) = module_specifier.to_file_path() { Some(vec![p]) } else { @@ -2145,16 +2147,17 @@ fn lock_arg<'a>() -> Arg<'a> { Arg::new("lock") .long("lock") .value_name("FILE") - .help("Check the specified lock file") + .help("Check the specified lock file. If value is not provided, defaults to \"deno.lock\" in the current working directory.") .takes_value(true) + .min_values(0) + .max_values(1) .value_hint(ValueHint::FilePath) } fn lock_write_arg<'a>() -> Arg<'a> { Arg::new("lock-write") .long("lock-write") - .requires("lock") - .help("Write lock file (use with --lock)") + .help("Force overwriting the lock file.") } static CONFIG_HELP: Lazy = Lazy::new(|| { @@ -3098,7 +3101,11 @@ fn lock_args_parse(flags: &mut Flags, matches: &clap::ArgMatches) { fn lock_arg_parse(flags: &mut Flags, matches: &clap::ArgMatches) { if matches.is_present("lock") { - let lockfile = matches.value_of("lock").unwrap(); + let lockfile = if let Some(path) = matches.value_of("lock") { + path + } else { + "./deno.lock" + }; flags.lock = Some(PathBuf::from(lockfile)); } } @@ -5335,6 +5342,57 @@ mod tests { ..Flags::default() } ); + + let r = flags_from_vec(svec![ + "deno", + "run", + "--lock", + "--lock-write", + "script.ts" + ]); + assert_eq!( + r.unwrap(), + Flags { + subcommand: DenoSubcommand::Run(RunFlags { + script: "script.ts".to_string(), + }), + lock_write: true, + lock: Some(PathBuf::from("./deno.lock")), + ..Flags::default() + } + ); + + let r = flags_from_vec(svec![ + "deno", + "run", + "--lock-write", + "--lock", + "lock.json", + "script.ts" + ]); + assert_eq!( + r.unwrap(), + Flags { + subcommand: DenoSubcommand::Run(RunFlags { + script: "script.ts".to_string(), + }), + lock_write: true, + lock: Some(PathBuf::from("lock.json")), + ..Flags::default() + } + ); + + let r = flags_from_vec(svec!["deno", "run", "--lock-write", "script.ts"]); + assert_eq!( + r.unwrap(), + Flags { + subcommand: DenoSubcommand::Run(RunFlags { + script: "script.ts".to_string(), + }), + lock_write: true, + ..Flags::default() + } + ); } #[test] diff --git a/cli/args/mod.rs b/cli/args/mod.rs index e46313858e..64755a4948 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -24,6 +24,7 @@ use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::normalize_path; +use deno_core::parking_lot::Mutex; use deno_core::url::Url; use deno_runtime::colors; use deno_runtime::deno_tls::rustls::RootCertStore; @@ -33,6 +34,7 @@ use std::collections::BTreeMap; use std::env; use std::net::SocketAddr; use std::path::PathBuf; +use std::sync::Arc; use crate::args::config_file::JsxImportSourceConfig; use crate::deno_dir::DenoDir; @@ -61,11 +63,16 @@ pub struct CliOptions { // application need not concern itself with, so keep these private flags: Flags, maybe_config_file: Option, + maybe_lockfile: Option>>, overrides: CliOptionOverrides, } impl CliOptions { - pub fn new(flags: Flags, maybe_config_file: Option) -> Self { + pub fn new( + flags: Flags, + maybe_config_file: Option, + maybe_lockfile: Option, + ) -> Self { if let Some(insecure_allowlist) = flags.unsafely_ignore_certificate_errors.as_ref() { @@ -80,8 +87,11 @@ impl CliOptions { eprintln!("{}", colors::yellow(msg)); } + let maybe_lockfile = maybe_lockfile.map(|l| Arc::new(Mutex::new(l))); + Self { maybe_config_file, + maybe_lockfile, flags, overrides: Default::default(), } @@ -89,7 +99,9 @@ impl CliOptions { pub fn from_flags(flags: Flags) -> Result { let maybe_config_file = ConfigFile::discover(&flags)?; - Ok(Self::new(flags, maybe_config_file)) + let maybe_lock_file = + Lockfile::discover(&flags, maybe_config_file.as_ref())?; + Ok(Self::new(flags, maybe_config_file, maybe_lock_file)) } pub fn maybe_config_file_specifier(&self) -> Option { @@ -210,13 +222,8 @@ impl CliOptions { .map(|host| InspectorServer::new(host, version::get_user_agent())) } - pub fn resolve_lock_file(&self) -> Result, AnyError> { - if let Some(filename) = &self.flags.lock { - let lockfile = Lockfile::new(filename.clone(), self.flags.lock_write)?; - Ok(Some(lockfile)) - } else { - Ok(None) - } + pub fn maybe_lock_file(&self) -> Option>> { + self.maybe_lockfile.clone() } pub fn resolve_tasks_config( diff --git a/cli/lockfile.rs b/cli/lockfile.rs index 78f5f2ab8b..b0cf689177 100644 --- a/cli/lockfile.rs +++ b/cli/lockfile.rs @@ -15,9 +15,11 @@ use std::path::PathBuf; use std::rc::Rc; use std::sync::Arc; +use crate::args::ConfigFile; use crate::npm::NpmPackageReq; use crate::npm::NpmResolutionPackage; use crate::tools::fmt::format_json; +use crate::Flags; #[derive(Debug)] pub struct LockfileError(String); @@ -73,6 +75,16 @@ pub struct LockfileContent { pub npm: NpmContent, } +impl LockfileContent { + fn empty() -> Self { + Self { + version: "2".to_string(), + remote: BTreeMap::new(), + npm: NpmContent::default(), + } + } +} + #[derive(Debug, Clone)] pub struct Lockfile { pub overwrite: bool, @@ -82,36 +94,91 @@ pub struct Lockfile { } impl Lockfile { + pub fn discover( + flags: &Flags, + maybe_config_file: Option<&ConfigFile>, + ) -> Result, AnyError> { + let filename = match flags.lock { + Some(ref lock) => PathBuf::from(lock), + None => match maybe_config_file { + Some(config_file) => { + if config_file.specifier.scheme() == "file" { + let mut path = config_file.specifier.to_file_path().unwrap(); + path.set_file_name("deno.lock"); + path + } else { + return Ok(None); + } + } + None => return Ok(None), + }, + }; + + let lockfile = Self::new(filename, flags.lock_write)?; + Ok(Some(lockfile)) + } + pub fn new(filename: PathBuf, overwrite: bool) -> Result { // Writing a lock file always uses the new format. - let content = if overwrite { + if overwrite { + return Ok(Lockfile { + overwrite, + has_content_changed: false, + content: LockfileContent::empty(), + filename, + }); + } + + let result = match std::fs::read_to_string(&filename) { + Ok(content) => Ok(content), + Err(e) => { + if e.kind() == std::io::ErrorKind::NotFound { + return Ok(Lockfile { + overwrite, + has_content_changed: false, + content: LockfileContent::empty(), + filename, + }); + } else { + Err(e) + } + } + }; + + let s = result.with_context(|| { + format!("Unable to read lockfile: \"{}\"", filename.display()) + })?; + let value: serde_json::Value = + serde_json::from_str(&s).with_context(|| { + format!( + "Unable to parse contents of the lockfile \"{}\"", + filename.display() + ) + })?; + let version = value.get("version").and_then(|v| v.as_str()); + let content = if version == Some("2") { + serde_json::from_value::(value).with_context(|| { + format!( + "Unable to parse contents of the lockfile \"{}\"", + filename.display() + ) + })? + } else { + // If there's no version field, we assume that user is using the old + // version of the lockfile. We'll migrate it in-place into v2 and it + // will be writte in v2 if user uses `--lock-write` flag. + let remote: BTreeMap = serde_json::from_value(value) + .with_context(|| { + format!( + "Unable to parse contents of the lockfile \"{}\"", + filename.display() + ) + })?; LockfileContent { version: "2".to_string(), - remote: BTreeMap::new(), + remote, npm: NpmContent::default(), } - } else { - let s = std::fs::read_to_string(&filename).with_context(|| { - format!("Unable to read lockfile: {}", filename.display()) - })?; - let value: serde_json::Value = serde_json::from_str(&s) - .context("Unable to parse contents of the lockfile")?; - let version = value.get("version").and_then(|v| v.as_str()); - if version == Some("2") { - serde_json::from_value::(value) - .context("Unable to parse lockfile")? - } else { - // If there's no version field, we assume that user is using the old - // version of the lockfile. We'll migrate it in-place into v2 and it - // will be writte in v2 if user uses `--lock-write` flag. - let remote: BTreeMap = - serde_json::from_value(value).context("Unable to parse lockfile")?; - LockfileContent { - version: "2".to_string(), - remote, - npm: NpmContent::default(), - } - } }; Ok(Lockfile { @@ -209,10 +276,15 @@ impl Lockfile { .unwrap_or(&package.dist.shasum); if &package_info.integrity != integrity { return Err(LockfileError(format!( - "Integrity check failed for npm package: \"{}\". - Cache has \"{}\" and lockfile has \"{}\". - Use \"--lock-write\" flag to update the lockfile.", - package.id, integrity, package_info.integrity + "Integrity check failed for npm package: \"{}\". Unable to verify that the package +is the same as when the lockfile was generated. + +This could be caused by: + * the lock file may be corrupt + * the source itself may be corrupt + +Use \"--lock-write\" flag to regenerate the lockfile at \"{}\".", + package.id, self.filename.display() ))); } } else { @@ -338,9 +410,9 @@ mod tests { } #[test] - fn new_nonexistent_lockfile() { + fn create_lockfile_for_nonexistent_path() { let file_path = PathBuf::from("nonexistent_lock_file.json"); - assert!(Lockfile::new(file_path, false).is_err()); + assert!(Lockfile::new(file_path, false).is_ok()); } #[test] diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index a3f516615a..3a0906636d 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -2916,6 +2916,8 @@ impl Inner { ..Default::default() }, self.maybe_config_file.clone(), + // TODO(#16510): add support for lockfile + None, ); cli_options.set_import_map_specifier(self.maybe_import_map_uri.clone()); diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 3ea4214ab5..148f44923d 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -168,9 +168,7 @@ impl ProcState { Some(progress_bar.clone()), )?; - let lockfile = cli_options - .resolve_lock_file()? - .map(|f| Arc::new(Mutex::new(f))); + let lockfile = cli_options.maybe_lock_file(); let maybe_import_map_specifier = cli_options.resolve_import_map_specifier()?; @@ -296,7 +294,6 @@ impl ProcState { npm_package_reqs.push(package_ref.req); } } - let roots = roots .into_iter() .map(|s| (s, ModuleKind::Esm)) @@ -312,6 +309,13 @@ impl ProcState { self.options.type_check_mode() != TypeCheckMode::None, false, ) { + // TODO(bartlomieju): this is strange... ideally there should be only + // one codepath in `prepare_module_load` so we don't forget things + // like writing a lockfile. Figure a way to refactor this function. + if let Some(ref lockfile) = self.lockfile { + let g = lockfile.lock(); + g.write()?; + } return result; } } diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index a5b4431716..72af72a76c 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -998,6 +998,54 @@ fn lock_file_missing_top_level_package() { ); } +#[test] +fn auto_discover_lock_file() { + let _server = http_server(); + + let deno_dir = util::new_deno_dir(); + let temp_dir = util::TempDir::new(); + + // write empty config file + temp_dir.write("deno.json", "{}"); + + // write a lock file with borked integrity + let lock_file_content = r#"{ + "version": "2", + "remote": {}, + "npm": { + "specifiers": { "@denotest/bin": "@denotest/bin@1.0.0" }, + "packages": { + "@denotest/bin@1.0.0": { + "integrity": "sha512-foobar", + "dependencies": {} + } + } + } + }"#; + temp_dir.write("deno.lock", lock_file_content); + + let deno = util::deno_cmd_with_deno_dir(&deno_dir) + .current_dir(temp_dir.path()) + .arg("run") + .arg("--unstable") + .arg("-A") + .arg("npm:@denotest/bin/cli-esm") + .arg("test") + .envs(env_vars()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let output = deno.wait_with_output().unwrap(); + assert!(!output.status.success()); + assert_eq!(output.status.code(), Some(10)); + + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!(stderr.contains( + "Integrity check failed for npm package: \"@denotest/bin@1.0.0\"" + )); +} + fn env_vars_no_sync_download() -> Vec<(String, String)> { vec![ ("DENO_NODE_COMPAT_URL".to_string(), util::std_file_url()), diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 99e122c69b..68d15bfd9b 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -609,12 +609,6 @@ itest!(private_field_presence_no_check { output: "run/private_field_presence.ts.out", }); -itest!(lock_write_requires_lock { - args: "run --lock-write some_file.ts", - output: "run/lock_write_requires_lock.out", - exit_code: 1, -}); - // TODO(bartlomieju): remove --unstable once Deno.spawn is stabilized itest!(lock_write_fetch { args: @@ -3634,3 +3628,10 @@ fn websocket_server_idletimeout() { assert!(child.wait().unwrap().success()); } + +itest!(auto_discover_lockfile { + args: "run run/auto_discover_lockfile/main.ts", + output: "run/auto_discover_lockfile/main.out", + http_server: true, + exit_code: 10, +}); diff --git a/cli/tests/testdata/jsx/deno.lock b/cli/tests/testdata/jsx/deno.lock new file mode 100644 index 0000000000..64429f30a1 --- /dev/null +++ b/cli/tests/testdata/jsx/deno.lock @@ -0,0 +1,7 @@ +{ + "version": "2", + "remote": { + "http://localhost:4545/jsx/jsx-dev-runtime": "7cac3d940791b3c8e671b24f9678ca37d87d40487ed2b3720a2a40891aa6173d", + "http://localhost:4545/jsx/jsx-runtime": "7cac3d940791b3c8e671b24f9678ca37d87d40487ed2b3720a2a40891aa6173d" + } +} diff --git a/cli/tests/testdata/npm/lock_file/main.out b/cli/tests/testdata/npm/lock_file/main.out index 4c034d03b1..b8447bee63 100644 --- a/cli/tests/testdata/npm/lock_file/main.out +++ b/cli/tests/testdata/npm/lock_file/main.out @@ -1,4 +1,9 @@ Download [WILDCARD] -error: Integrity check failed for npm package: "@babel/parser@7.19.0". - Cache has "sha512-74bEXKX2h+8rrfQUfsBfuZZHzsEs6Eql4pqy/T4Nn6Y9wNPggQOqD6z6pn5Bl8ZfysKouFZT/UXEH94ummEeQw==" and lockfile has "sha512-foobar!". - Use "--lock-write" flag to update the lockfile. +error: Integrity check failed for npm package: "@babel/parser@7.19.0". Unable to verify that the package +is the same as when the lockfile was generated. + +This could be caused by: + * the lock file may be corrupt + * the source itself may be corrupt + +Use "--lock-write" flag to regenerate the lockfile at "[WILDCARD]lock.json". diff --git a/cli/tests/testdata/run/auto_discover_lockfile/deno.json b/cli/tests/testdata/run/auto_discover_lockfile/deno.json new file mode 100644 index 0000000000..90faa728a1 --- /dev/null +++ b/cli/tests/testdata/run/auto_discover_lockfile/deno.json @@ -0,0 +1,3 @@ +{ + "tasks": {} +} diff --git a/cli/tests/testdata/run/auto_discover_lockfile/deno.lock b/cli/tests/testdata/run/auto_discover_lockfile/deno.lock new file mode 100644 index 0000000000..059f66789f --- /dev/null +++ b/cli/tests/testdata/run/auto_discover_lockfile/deno.lock @@ -0,0 +1,7 @@ +{ + "version": "2", + "remote": { + "http://localhost:4545/subdir/mod2.ts": "cae1d3e9f3c38cd415ff52dff854be8f3d17d35f8d7b3d285e813fb0f6393a2f", + "http://localhost:4545/subdir/print_hello.ts": "foobar" + } +} diff --git a/cli/tests/testdata/run/auto_discover_lockfile/main.out b/cli/tests/testdata/run/auto_discover_lockfile/main.out new file mode 100644 index 0000000000..28f4724e98 --- /dev/null +++ b/cli/tests/testdata/run/auto_discover_lockfile/main.out @@ -0,0 +1,5 @@ +Download http://localhost:4545/subdir/mod2.ts +Download http://localhost:4545/subdir/print_hello.ts +error: The source code is invalid, as it does not match the expected hash in the lock file. + Specifier: http://localhost:4545/subdir/print_hello.ts + Lock file: [WILDCARD]auto_discover_lockfile[WILDCARD]deno.lock diff --git a/cli/tests/testdata/run/auto_discover_lockfile/main.ts b/cli/tests/testdata/run/auto_discover_lockfile/main.ts new file mode 100644 index 0000000000..baa52775d4 --- /dev/null +++ b/cli/tests/testdata/run/auto_discover_lockfile/main.ts @@ -0,0 +1 @@ +import "http://localhost:4545/subdir/mod2.ts"; diff --git a/cli/tests/testdata/run/config_types/deno.lock b/cli/tests/testdata/run/config_types/deno.lock new file mode 100644 index 0000000000..157bd98a2e --- /dev/null +++ b/cli/tests/testdata/run/config_types/deno.lock @@ -0,0 +1,6 @@ +{ + "version": "2", + "remote": { + "http://localhost:4545/run/config_types/types.d.ts": "741c39165e37de0c12acc5c081841f4362487e3f17dc4cad7017b70af72c4605" + } +} diff --git a/cli/tests/testdata/run/lock_write_requires_lock.out b/cli/tests/testdata/run/lock_write_requires_lock.out deleted file mode 100644 index 7cc5906f6c..0000000000 --- a/cli/tests/testdata/run/lock_write_requires_lock.out +++ /dev/null @@ -1,3 +0,0 @@ -error: The following required arguments were not provided: - --lock -[WILDCARD] \ No newline at end of file