From 64b3a2d9f81aaf2679efd205fc3564dd5dfc8030 Mon Sep 17 00:00:00 2001 From: Luka Leer Date: Thu, 26 Dec 2024 03:53:32 +0100 Subject: [PATCH] fix(cli): do not create `bin` folder within `DENO_INSTALL_ROOT` As can be seen in the documentation, `DENO_INSTALL_ROOT` is meant to be the folder wherein Deno will install executables, with the default being `$HOME/.deno/bin`. However, the current behaviour of the `install` and `uninstall` commands, if `DENO_INSTALL_ROOT` is set, is to still append "bin" to the path. This leads to executables being installed in a folder like `$HOME/.deno/bin/bin`, which is obviously not the intended behaviour. This commit removes this appending. Do note that this is a breaking change in the behaviour of the `--root` flag, as previously it referred to a folder wherein the `bin` folder was located, but now it refers to the `bin` folder itself. For example: `deno install --root /foo/bar test.ts` would previously install the executable in `/foo/bar/bin/test`, but now it will install it in `/foo/bar/test`. --- cli/args/flags.rs | 8 ++--- cli/tools/installer.rs | 34 +++++++++---------- tests/integration/install_tests.rs | 17 ++++++---- .../specs/cert/cafile_install/__test__.jsonc | 2 +- .../install/future_install_global/assert.js | 2 +- 5 files changed, 34 insertions(+), 29 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 418edcf34b..5cb2436930 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -2579,12 +2579,12 @@ The executable name is inferred by default: - If the resulting name has an @... suffix, strip it. To change the installation root, use --root: - deno install -g --allow-net --allow-read --root /usr/local jsr:@std/http/file-server + deno install -g --allow-net --allow-read --root /usr/local/bin jsr:@std/http/file-server The installation root is determined, in order of precedence: - --root option - DENO_INSTALL_ROOT environment variable - - $HOME/.deno + - $HOME/.deno/bin These must be added to the path manually if required."), UnstableArgsConfig::ResolutionAndRuntime) .visible_alias("i") @@ -2755,12 +2755,12 @@ fn uninstall_subcommand() -> Command { deno uninstall --global file_server To change the installation root, use --root flag: - deno uninstall --global --root /usr/local serve + deno uninstall --global --root /usr/local/bin serve The installation root is determined, in order of precedence: - --root option - DENO_INSTALL_ROOT environment variable - - $HOME/.deno"), + - $HOME/.deno/bin"), UnstableArgsConfig::None, ) .defer(|cmd| { diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index ec538ecb0a..fcf806b37a 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -136,6 +136,7 @@ fn get_installer_root() -> Result { ) })?; home_path.push(".deno"); + home_path.push("bin"); Ok(home_path) } @@ -213,12 +214,11 @@ pub async fn uninstall( }; let cwd = std::env::current_dir().context("Unable to get CWD")?; - let root = if let Some(root) = uninstall_flags.root { + let installation_dir = if let Some(root) = uninstall_flags.root { canonicalize_path_maybe_not_exists(&cwd.join(root))? } else { get_installer_root()? }; - let installation_dir = root.join("bin"); // ensure directory exists if let Ok(metadata) = fs::metadata(&installation_dir) { @@ -473,12 +473,11 @@ async fn resolve_shim_data( install_flags_global: &InstallFlagsGlobal, ) -> Result { let cwd = std::env::current_dir().context("Unable to get CWD")?; - let root = if let Some(root) = &install_flags_global.root { + let installation_dir = if let Some(root) = &install_flags_global.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 module_url = resolve_url_or_path(&install_flags_global.module_url, &cwd)?; @@ -872,7 +871,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_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1162,6 +1161,7 @@ mod tests { #[tokio::test] async fn install_npm_lockfile_default() { let temp_dir = canonicalize_path(&env::temp_dir()).unwrap(); + let bin_dir = temp_dir.join("bin"); let shim_data = resolve_shim_data( &HttpClientProvider::new(None, None), &Flags { @@ -1175,14 +1175,14 @@ mod tests { module_url: "npm:cowsay".to_string(), args: vec![], name: None, - root: Some(temp_dir.to_string_lossy().to_string()), + root: Some(bin_dir.to_string_lossy().to_string()), force: false, }, ) .await .unwrap(); - let lock_path = temp_dir.join("bin").join(".cowsay.lock.json"); + let lock_path = bin_dir.join(".cowsay.lock.json"); assert_eq!( shim_data.args, vec![ @@ -1249,7 +1249,7 @@ mod tests { module_url: local_module_str.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1279,7 +1279,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_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1300,7 +1300,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_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1322,7 +1322,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_string()), + root: Some(bin_dir.to_string()), force: true, }, ) @@ -1353,7 +1353,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_string()), + root: Some(bin_dir.to_string()), force: true, }, ) @@ -1383,7 +1383,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_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1424,7 +1424,7 @@ mod tests { module_url: local_module_str.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_string()), + root: Some(bin_dir.to_string()), force: false, }, ) @@ -1470,7 +1470,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_string()), + root: Some(bin_dir.to_string()), force: true, }, ) @@ -1513,7 +1513,7 @@ mod tests { module_url: file_module_string.to_string(), args: vec![], name: Some("echo_test".to_string()), - root: Some(temp_dir.path().to_string()), + root: Some(bin_dir.to_string()), force: true, }, ) @@ -1570,7 +1570,7 @@ mod tests { UninstallFlags { kind: UninstallKind::Global(UninstallFlagsGlobal { name: "echo_test".to_string(), - root: Some(temp_dir.path().to_string()), + root: Some(bin_dir.to_string()), }), }, ) diff --git a/tests/integration/install_tests.rs b/tests/integration/install_tests.rs index b0c1e44778..cc4c1e5f8b 100644 --- a/tests/integration/install_tests.rs +++ b/tests/integration/install_tests.rs @@ -160,6 +160,9 @@ fn install_custom_dir_env_var() { let context = TestContext::with_http_server(); let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); + let bin_dir = temp_dir.path().join("custom_bin"); + std::fs::create_dir(&bin_dir).unwrap(); + let bin_dir_str = bin_dir.to_string(); context .new_command() @@ -168,13 +171,13 @@ fn install_custom_dir_env_var() { .envs([ ("HOME", temp_dir_str.as_str()), ("USERPROFILE", temp_dir_str.as_str()), - ("DENO_INSTALL_ROOT", temp_dir_str.as_str()), + ("DENO_INSTALL_ROOT", bin_dir_str.as_str()), ]) .run() .skip_output_check() .assert_exit_code(0); - let mut file_path = temp_dir.path().join("bin/echo_test"); + let mut file_path = bin_dir.join("echo_test"); assert!(file_path.exists()); if cfg!(windows) { @@ -200,6 +203,9 @@ fn installer_test_local_module_run() { let context = TestContext::with_http_server(); let temp_dir = context.temp_dir(); let temp_dir_str = temp_dir.path().to_string(); + let bin_dir = temp_dir.path().join("bin"); + std::fs::create_dir(&bin_dir).unwrap(); + let bin_dir_str = bin_dir.to_string(); let echo_ts_str = util::testdata_path().join("echo.ts").to_string(); context @@ -211,7 +217,7 @@ fn installer_test_local_module_run() { "--name", "echo_test", "--root", - temp_dir_str.as_str(), + bin_dir_str.as_str(), echo_ts_str.as_str(), "hello", ]) @@ -224,7 +230,6 @@ fn installer_test_local_module_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"); @@ -252,7 +257,7 @@ fn installer_test_remote_module_run() { let bin_dir = root_dir.join("bin"); context .new_command() - .args("install --name echo_test --root ./root -g http://localhost:4545/echo.ts hello") + .args("install --name echo_test --root ./root/bin -g http://localhost:4545/echo.ts hello") .run() .skip_output_check() .assert_exit_code(0); @@ -274,7 +279,7 @@ fn installer_test_remote_module_run() { // now uninstall with the relative path context .new_command() - .args("uninstall -g --root ./root echo_test") + .args("uninstall -g --root ./root/bin echo_test") .run() .skip_output_check() .assert_exit_code(0); diff --git a/tests/specs/cert/cafile_install/__test__.jsonc b/tests/specs/cert/cafile_install/__test__.jsonc index 67af9be05e..a0032229fc 100644 --- a/tests/specs/cert/cafile_install/__test__.jsonc +++ b/tests/specs/cert/cafile_install/__test__.jsonc @@ -8,7 +8,7 @@ "-n", "echo_test", "--root", - "$PWD", + "$PWD/bin", "-g", "https://localhost:5545/echo.ts" ], diff --git a/tests/specs/install/future_install_global/assert.js b/tests/specs/install/future_install_global/assert.js index c7a6a06807..c0872a3966 100644 --- a/tests/specs/install/future_install_global/assert.js +++ b/tests/specs/install/future_install_global/assert.js @@ -1,4 +1,4 @@ -const dirs = Deno.readDir("./bins/bin"); +const dirs = Deno.readDir("./bins"); let found = false; for await (const entry of dirs) {