From 60c60520604a430c98e395d0299f9d027cc024e1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 13 Nov 2023 09:44:01 -0500 Subject: [PATCH] fix(install): should work with non-existent relative root (#21161) Closes #21160 --- cli/args/flags.rs | 12 +- cli/tests/integration/install_tests.rs | 275 +++++++++++++------------ cli/tools/installer.rs | 49 ++--- cli/util/fs.rs | 10 +- 4 files changed, 179 insertions(+), 167 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 4bad301402..96cd0bbfe7 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -163,7 +163,7 @@ pub struct InstallFlags { pub module_url: String, pub args: Vec, pub name: Option, - pub root: Option, + pub root: Option, pub force: bool, } @@ -177,7 +177,7 @@ pub struct JupyterFlags { #[derive(Clone, Debug, Eq, PartialEq)] pub struct UninstallFlags { pub name: String, - pub root: Option, + pub root: Option, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -1762,7 +1762,6 @@ These must be added to the path manually if required.") Arg::new("root") .long("root") .help("Installation root") - .value_parser(value_parser!(PathBuf)) .value_hint(ValueHint::DirPath)) .arg( Arg::new("force") @@ -1822,7 +1821,6 @@ The installation root is determined, in order of precedence: Arg::new("root") .long("root") .help("Installation root") - .value_parser(value_parser!(PathBuf)) .value_hint(ValueHint::DirPath)) ) } @@ -3407,7 +3405,7 @@ fn info_parse(flags: &mut Flags, matches: &mut ArgMatches) { fn install_parse(flags: &mut Flags, matches: &mut ArgMatches) { runtime_args_parse(flags, matches, true, true); - let root = matches.remove_one::("root"); + let root = matches.remove_one::("root"); let force = matches.get_flag("force"); let name = matches.remove_one::("name"); @@ -3438,7 +3436,7 @@ fn jupyter_parse(flags: &mut Flags, matches: &mut ArgMatches) { } fn uninstall_parse(flags: &mut Flags, matches: &mut ArgMatches) { - let root = matches.remove_one::("root"); + let root = matches.remove_one::("root"); let name = matches.remove_one::("name").unwrap(); flags.subcommand = DenoSubcommand::Uninstall(UninstallFlags { name, root }); @@ -6361,7 +6359,7 @@ mod tests { name: Some("file_server".to_string()), module_url: "https://deno.land/std/http/file_server.ts".to_string(), args: svec!["foo", "bar"], - root: Some(PathBuf::from("/foo")), + root: Some("/foo".to_string()), force: true, }), import_map_path: Some("import_map.json".to_string()), diff --git a/cli/tests/integration/install_tests.rs b/cli/tests/integration/install_tests.rs index 0756d460c3..d9a7a4fb64 100644 --- a/cli/tests/integration/install_tests.rs +++ b/cli/tests/integration/install_tests.rs @@ -1,38 +1,33 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use std::fs; -use std::process::Command; use test_util as util; use test_util::assert_contains; -use test_util::assert_ends_with; -use test_util::TempDir; +use util::TestContext; +use util::TestContextBuilder; #[test] fn install_basic() { - let _guard = util::http_server(); - let temp_dir = TempDir::new(); + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); // ensure a lockfile doesn't get created or updated locally temp_dir.write("deno.json", "{}"); - let status = util::deno_cmd() - .current_dir(temp_dir.path()) - .arg("install") - .arg("--check") - .arg("--name") - .arg("echo_test") - .arg("http://localhost:4545/echo.ts") + context + .new_command() + .args("install --check --name echo_test http://localhost:4545/echo.ts") .envs([ ("HOME", temp_dir_str.as_str()), ("USERPROFILE", temp_dir_str.as_str()), ("DENO_INSTALL_ROOT", ""), ]) - .spawn() - .unwrap() - .wait() - .unwrap(); - assert!(status.success()); + .run() + .skip_output_check() + .assert_exit_code(0); // no lockfile should be created locally assert!(!temp_dir.path().join("deno.lock").exists()); @@ -62,20 +57,17 @@ fn install_basic() { } // now uninstall - let status = util::deno_cmd() - .current_dir(temp_dir.path()) - .arg("uninstall") - .arg("echo_test") + context + .new_command() + .args("uninstall echo_test") .envs([ ("HOME", temp_dir_str.as_str()), ("USERPROFILE", temp_dir_str.as_str()), ("DENO_INSTALL_ROOT", ""), ]) - .spawn() - .unwrap() - .wait() - .unwrap(); - assert!(status.success()); + .run() + .skip_output_check() + .assert_exit_code(0); // ensure local lockfile still doesn't exist assert!(!temp_dir.path().join("deno.lock").exists()); @@ -85,27 +77,22 @@ fn install_basic() { #[test] fn install_custom_dir_env_var() { - let _guard = util::http_server(); - let temp_dir = TempDir::new(); + let context = TestContext::with_http_server(); + let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); - let status = util::deno_cmd() - .current_dir(util::root_path()) // different cwd - .arg("install") - .arg("--check") - .arg("--name") - .arg("echo_test") - .arg("http://localhost:4545/echo.ts") + context + .new_command() + .cwd(util::root_path()) // different cwd + .args("install --check --name echo_test http://localhost:4545/echo.ts") .envs([ ("HOME", temp_dir_str.as_str()), ("USERPROFILE", temp_dir_str.as_str()), ("DENO_INSTALL_ROOT", temp_dir_str.as_str()), ]) - .spawn() - .unwrap() - .wait() - .unwrap(); - assert!(status.success()); + .run() + .skip_output_check() + .assert_exit_code(0); let mut file_path = temp_dir.path().join("bin/echo_test"); assert!(file_path.exists()); @@ -114,7 +101,7 @@ fn install_custom_dir_env_var() { file_path = file_path.with_extension("cmd"); } - let content = fs::read_to_string(file_path).unwrap(); + let content = file_path.read_to_string(); if cfg!(windows) { assert_contains!( content, @@ -130,114 +117,134 @@ fn install_custom_dir_env_var() { #[test] fn installer_test_local_module_run() { - let temp_dir = TempDir::new(); - let bin_dir = temp_dir.path().join("bin"); - std::fs::create_dir(&bin_dir).unwrap(); - let status = util::deno_cmd() - .current_dir(util::root_path()) - .arg("install") - .arg("--name") - .arg("echo_test") - .arg("--root") - .arg(temp_dir.path()) - .arg(util::testdata_path().join("echo.ts")) - .arg("hello") - .spawn() - .unwrap() - .wait() - .unwrap(); - assert!(status.success()); - let mut file_path = bin_dir.join("echo_test"); - if cfg!(windows) { - file_path = file_path.with_extension("cmd"); - } - assert!(file_path.exists()); - // NOTE: using file_path here instead of exec_name, because tests - // shouldn't mess with user's PATH env variable - let output = Command::new(file_path) - .current_dir(temp_dir.path()) - .arg("foo") - .env("PATH", util::target_dir()) - .output() - .unwrap(); - let stdout_str = std::str::from_utf8(&output.stdout).unwrap().trim(); - assert_ends_with!(stdout_str, "hello, foo"); -} - -#[test] -fn installer_test_remote_module_run() { - let _g = util::http_server(); - let temp_dir = TempDir::new(); - let bin_dir = temp_dir.path().join("bin"); - std::fs::create_dir(&bin_dir).unwrap(); - let status = util::deno_cmd() - .current_dir(util::testdata_path()) - .arg("install") - .arg("--name") - .arg("echo_test") - .arg("--root") - .arg(temp_dir.path()) - .arg("http://localhost:4545/echo.ts") - .arg("hello") - .spawn() - .unwrap() - .wait() - .unwrap(); - assert!(status.success()); - let mut file_path = bin_dir.join("echo_test"); - if cfg!(windows) { - file_path = file_path.with_extension("cmd"); - } - assert!(file_path.exists()); - let output = Command::new(file_path) - .current_dir(temp_dir.path()) - .arg("foo") - .env("PATH", util::target_dir()) - .output() - .unwrap(); - assert_ends_with!( - std::str::from_utf8(&output.stdout).unwrap().trim(), - "hello, foo", - ); -} - -#[test] -fn check_local_by_default() { - let _guard = util::http_server(); - let temp_dir = TempDir::new(); + let context = TestContext::with_http_server(); + let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); + let echo_ts_str = util::testdata_path().join("echo.ts").to_string(); - let status = util::deno_cmd() - .current_dir(temp_dir.path()) - .arg("install") - .arg(util::testdata_path().join("./install/check_local_by_default.ts")) + context + .new_command() + .cwd(util::root_path()) + .args_vec([ + "install", + "--name", + "echo_test", + "--root", + temp_dir_str.as_str(), + echo_ts_str.as_str(), + "hello", + ]) .envs([ ("HOME", temp_dir_str.as_str()), ("USERPROFILE", temp_dir_str.as_str()), ("DENO_INSTALL_ROOT", ""), ]) - .status() - .unwrap(); - assert!(status.success()); + .run() + .skip_output_check() + .assert_exit_code(0); + + let bin_dir = temp_dir.path().join("bin"); + let mut file_path = bin_dir.join("echo_test"); + if cfg!(windows) { + file_path = file_path.with_extension("cmd"); + } + assert!(file_path.exists()); + let output = context + .new_command() + .name(&file_path) + .cwd(temp_dir.path()) + .args("foo") + .env("PATH", util::target_dir()) + .run(); + output.assert_matches_text("hello, foo"); + output.assert_exit_code(0); +} + +#[test] +fn installer_test_remote_module_run() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + let root_dir = temp_dir.path().join("root"); + let bin_dir = root_dir.join("bin"); + context + .new_command() + .args("install --name echo_test --root ./root http://localhost:4545/echo.ts hello") + .run() + .skip_output_check() + .assert_exit_code(0); + let mut bin_file_path = bin_dir.join("echo_test"); + if cfg!(windows) { + bin_file_path = bin_file_path.with_extension("cmd"); + } + assert!(bin_file_path.exists()); + let output = context + .new_command() + .name(&bin_file_path) + .cwd(root_dir) + .args("foo") + .env("PATH", util::target_dir()) + .run(); + output.assert_matches_text("hello, foo"); + output.assert_exit_code(0); + + // now uninstall with the relative path + context + .new_command() + .args("uninstall --root ./root echo_test") + .run() + .skip_output_check() + .assert_exit_code(0); + assert!(!bin_file_path.exists()); +} + +#[test] +fn check_local_by_default() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + let temp_dir_str = temp_dir.path().to_string(); + let script_path = + util::testdata_path().join("./install/check_local_by_default.ts"); + let script_path_str = script_path.to_string_lossy().to_string(); + context + .new_command() + .args_vec(["install", script_path_str.as_str()]) + .envs([ + ("HOME", temp_dir_str.as_str()), + ("USERPROFILE", temp_dir_str.as_str()), + ("DENO_INSTALL_ROOT", ""), + ]) + .run() + .skip_output_check() + .assert_exit_code(0); } #[test] fn check_local_by_default2() { - let _guard = util::http_server(); - let temp_dir = TempDir::new(); + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); - - let status = util::deno_cmd() - .current_dir(temp_dir.path()) - .arg("install") - .arg(util::testdata_path().join("./install/check_local_by_default2.ts")) + let script_path = + util::testdata_path().join("./install/check_local_by_default2.ts"); + let script_path_str = script_path.to_string_lossy().to_string(); + context + .new_command() + .args_vec(["install", script_path_str.as_str()]) .envs([ ("HOME", temp_dir_str.as_str()), ("NO_COLOR", "1"), ("USERPROFILE", temp_dir_str.as_str()), ("DENO_INSTALL_ROOT", ""), ]) - .status() - .unwrap(); - assert!(status.success()); + .run() + .skip_output_check() + .assert_exit_code(0); } diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index c221168fe2..dfefb74d10 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -176,9 +176,10 @@ pub async fn infer_name_from_url(url: &Url) -> Option { Some(stem) } -pub fn uninstall(name: String, root: Option) -> Result<(), AnyError> { +pub fn uninstall(name: String, root: Option) -> Result<(), AnyError> { + let cwd = std::env::current_dir().context("Unable to get CWD")?; let root = if let Some(root) = root { - canonicalize_path_maybe_not_exists(&root)? + canonicalize_path_maybe_not_exists(&cwd.join(root))? } else { get_installer_root()? }; @@ -303,15 +304,15 @@ async fn resolve_shim_data( flags: &Flags, install_flags: &InstallFlags, ) -> Result { + let cwd = std::env::current_dir().context("Unable to get CWD")?; let root = if let Some(root) = &install_flags.root { - canonicalize_path_maybe_not_exists(root)? + canonicalize_path_maybe_not_exists(&cwd.join(root))? } else { get_installer_root()? }; let installation_dir = root.join("bin"); // Check if module_url is remote - let cwd = std::env::current_dir().context("Unable to get CWD")?; let module_url = resolve_url_or_path(&install_flags.module_url, &cwd)?; let name = if install_flags.name.is_some() { @@ -635,7 +636,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -669,7 +670,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: None, - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -691,7 +692,7 @@ mod tests { module_url: "http://localhost:4545/subdir/main.ts".to_string(), args: vec![], name: None, - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -715,7 +716,7 @@ mod tests { .to_string(), args: vec![], name: None, - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -741,7 +742,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -769,7 +770,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec!["--foobar".to_string()], name: Some("echo_test".to_string()), - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -802,7 +803,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -831,7 +832,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -861,7 +862,7 @@ mod tests { module_url: "npm:cowsay".to_string(), args: vec![], name: None, - root: Some(temp_dir.clone()), + root: Some(temp_dir.to_string_lossy().to_string()), force: false, }, ) @@ -895,7 +896,7 @@ mod tests { module_url: "npm:cowsay".to_string(), args: vec![], name: None, - root: Some(env::temp_dir()), + root: Some(env::temp_dir().to_string_lossy().to_string()), force: false, }, ) @@ -930,7 +931,7 @@ mod tests { module_url: local_module_str.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -959,7 +960,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -979,7 +980,7 @@ mod tests { module_url: "http://localhost:4545/cat.ts".to_string(), // using a different URL args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -1000,7 +1001,7 @@ mod tests { module_url: "http://localhost:4545/cat.ts".to_string(), // using a different URL args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: true, }, ) @@ -1030,7 +1031,7 @@ mod tests { module_url: "http://localhost:4545/cat.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: true, }, ) @@ -1059,7 +1060,7 @@ mod tests { module_url: "http://localhost:4545/echo_server.ts".to_string(), args: vec!["\"".to_string()], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -1099,7 +1100,7 @@ mod tests { module_url: local_module_str.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: false, }, ) @@ -1143,7 +1144,7 @@ mod tests { module_url: "http://localhost:4545/cat.ts".to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: true, }, ) @@ -1185,7 +1186,7 @@ mod tests { module_url: file_module_string.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_path_buf()), + root: Some(temp_dir.path().to_string()), force: true, }, ) @@ -1237,7 +1238,7 @@ mod tests { File::create(file_path).unwrap(); } - uninstall("echo_test".to_string(), Some(temp_dir.path().to_path_buf())) + uninstall("echo_test".to_string(), Some(temp_dir.path().to_string())) .unwrap(); assert!(!file_path.exists()); diff --git a/cli/util/fs.rs b/cli/util/fs.rs index c1fd00f5ea..33eb3af9d7 100644 --- a/cli/util/fs.rs +++ b/cli/util/fs.rs @@ -206,8 +206,14 @@ pub fn canonicalize_path_maybe_not_exists_with_fs( return Ok(canonicalized_path); } Err(err) if err.kind() == ErrorKind::NotFound => { - names_stack.push(path.file_name().unwrap()); - path = path.parent().unwrap(); + names_stack.push(match path.file_name() { + Some(name) => name.to_owned(), + None => return Err(err), + }); + path = match path.parent() { + Some(parent) => parent, + None => return Err(err), + }; } Err(err) => return Err(err), }