Repository: denoland/deno
Pull request: feat(BREAKING/install): require `--` for script arg in `deno install -g` and support installing multiple packages

Changes the behaviour of `deno install -g` to require `--` for the script argument.

This has the side effect of now supporting `deno install -g <pkg1> <pkg2>` similar to `npm install -g`.

Closes https://github.com/denoland/deno/issues/31024

Changed files:
filename: cli/args/flags.rs
patch: @@ -300,7 +300,7 @@ pub struct InfoFlags {
 
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct InstallFlagsGlobal {
-  pub module_url: String,
+  pub module_urls: Vec<String>,
   pub args: Vec<String>,
   pub name: Option<String>,
   pub root: Option<String>,
@@ -3193,6 +3193,7 @@ These must be added to the path manually if required."), UnstableArgsConfig::Res
             .num_args(1..)
             .value_hint(ValueHint::FilePath),
         )
+        .arg(script_arg().last(true))
         .arg(
           Arg::new("name")
             .long("name")
@@ -5904,16 +5905,29 @@ fn install_parse(
     let root = matches.remove_one::<String>("root");
     let force = matches.get_flag("force");
     let name = matches.remove_one::<String>("name");
-    let mut cmd_values =
-      matches.remove_many::<String>("cmd").unwrap_or_default();
-
-    let module_url = cmd_values.next().unwrap();
-    let args = cmd_values.collect();
+    let module_urls = matches
+      .remove_many::<String>("cmd")
+      .map(|values| values.collect::<Vec<_>>())
+      .unwrap_or_default();
+    let args = matches
+      .remove_many::<String>("script_arg")
+      .map(|values| values.collect::<Vec<_>>())
+      .unwrap_or_default();
+
+    if module_urls.len() > 1 && name.is_some() {
+      return Err(clap::Error::raw(
+        clap::error::ErrorKind::InvalidValue,
+        format!(
+          "Cannot specify --name when providing multiple packages to install ({}).",
+          module_urls.join(", ")
+        ),
+      ));
+    }
 
     flags.subcommand =
       DenoSubcommand::Install(InstallFlags::Global(InstallFlagsGlobal {
         name,
-        module_url,
+        module_urls,
         args,
         root,
         force,
@@ -9931,15 +9945,16 @@ mod tests {
       "deno",
       "install",
       "-g",
-      "jsr:@std/http/file-server"
+      "jsr:@std/http/file-server",
+      "npm:chalk",
     ]);
     assert_eq!(
       r.unwrap(),
       Flags {
         subcommand: DenoSubcommand::Install(InstallFlags::Global(
           InstallFlagsGlobal {
             name: None,
-            module_url: "jsr:@std/http/file-server".to_string(),
+            module_urls: svec!["jsr:@std/http/file-server", "npm:chalk"],
             args: vec![],
             root: None,
             force: false,
@@ -9961,7 +9976,7 @@ mod tests {
         subcommand: DenoSubcommand::Install(InstallFlags::Global(
           InstallFlagsGlobal {
             name: None,
-            module_url: "jsr:@std/http/file-server".to_string(),
+            module_urls: svec!["jsr:@std/http/file-server"],
             args: vec![],
             root: None,
             force: false,
@@ -9975,14 +9990,14 @@ mod tests {
   #[test]
   fn install_with_flags() {
     #[rustfmt::skip]
-    let r = flags_from_vec(svec!["deno", "install", "--global", "--import-map", "import_map.json", "--no-remote", "--config", "tsconfig.json", "--no-check", "--unsafely-ignore-certificate-errors", "--reload", "--lock", "lock.json", "--cert", "example.crt", "--cached-only", "--allow-read", "--allow-net", "--v8-flags=--help", "--seed", "1", "--inspect=127.0.0.1:9229", "--name", "file_server", "--root", "/foo", "--force", "--env=.example.env", "jsr:@std/http/file-server", "foo", "bar"]);
+    let r = flags_from_vec(svec!["deno", "install", "--global", "--import-map", "import_map.json", "--no-remote", "--config", "tsconfig.json", "--no-check", "--unsafely-ignore-certificate-errors", "--reload", "--lock", "lock.json", "--cert", "example.crt", "--cached-only", "--allow-read", "--allow-net", "--v8-flags=--help", "--seed", "1", "--inspect=127.0.0.1:9229", "--name", "file_server", "--root", "/foo", "--force", "--env=.example.env", "jsr:@std/http/file-server", "--", "foo", "bar"]);
     assert_eq!(
       r.unwrap(),
       Flags {
         subcommand: DenoSubcommand::Install(InstallFlags::Global(
           InstallFlagsGlobal {
             name: Some("file_server".to_string()),
-            module_url: "jsr:@std/http/file-server".to_string(),
+            module_urls: svec!["jsr:@std/http/file-server"],
             args: svec!["foo", "bar"],
             root: Some("/foo".to_string()),
             force: true,

filename: cli/args/mod.rs
patch: @@ -1155,9 +1155,11 @@ impl CliOptions {
         DenoSubcommand::Check(check_flags) => {
           Some(files_to_urls(&check_flags.files))
         }
-        DenoSubcommand::Install(InstallFlags::Global(flags)) => {
-          file_to_url(&flags.module_url).map(|url| vec![url])
-        }
+        DenoSubcommand::Install(InstallFlags::Global(flags)) => flags
+          .module_urls
+          .first()
+          .and_then(|url| file_to_url(url))
+          .map(|url| vec![url]),
         DenoSubcommand::Doc(DocFlags {
           source_files: DocSourceFileFlag::Paths(paths),
           ..

filename: cli/tools/installer/mod.rs
patch: @@ -826,49 +826,6 @@ async fn install_global(
     factory.npm_version_resolver()?.clone(),
   ));
 
-  let entry_text = install_flags_global.module_url.as_str();
-  if !cli_options.initial_cwd().join(entry_text).exists() {
-    // check for package requirement missing prefix
-    if let Ok(Err(package_req)) =
-      super::pm::AddRmPackageReq::parse(entry_text, None)
-    {
-      if jsr_resolver
-        .req_to_nv(&package_req)
-        .await
-        .ok()
-        .flatten()
-        .is_some()
-      {
-        bail!(
-          "{entry_text} is missing a prefix. Did you mean `{}`?",
-          crate::colors::yellow(format!("deno install -g jsr:{package_req}"))
-        );
-      } else if npm_resolver
-        .req_to_nv(&package_req)
-        .await
-        .ok()
-        .flatten()
-        .is_some()
-      {
-        bail!(
-          "{entry_text} is missing a prefix. Did you mean `{}`?",
-          crate::colors::yellow(format!("deno install -g npm:{package_req}"))
-        );
-      }
-    }
-  }
-
-  factory
-    .main_module_graph_container()
-    .await?
-    .load_and_type_check_files(
-      std::slice::from_ref(&install_flags_global.module_url),
-      CollectSpecifiersOptions {
-        include_ignored_specified: true,
-      },
-    )
-    .await?;
-
   if matches!(flags.config_flag, ConfigFlag::Discover)
     && cli_options.workspace().deno_jsons().next().is_some()
   {
@@ -878,27 +835,95 @@ async fn install_global(
     );
   }
 
-  let bin_name_resolver = factory.bin_name_resolver()?;
+  for (i, module_url) in install_flags_global.module_urls.iter().enumerate() {
+    let entry_text = module_url;
+    if !cli_options.initial_cwd().join(entry_text).exists() {
+      // provide a helpful error message for users migrating from Deno < 3.0
+      if i == 1
+        && install_flags_global.args.is_empty()
+        && Url::parse(entry_text).is_err()
+      {
+        bail!(
+          concat!(
+            "{} is missing a prefix. Deno 3.0 requires `--` before script arguments in `deno install -g`. ",
+            "Did you mean `deno install -g {} -- {}`? Or maybe provide a `jsr:` or `npm:` prefix?",
+          ),
+          entry_text,
+          &install_flags_global.module_urls[0],
+          install_flags_global.module_urls[1..].join(" "),
+        )
+      }
+      // check for package requirement missing prefix
+      if let Ok(Err(package_req)) =
+        super::pm::AddRmPackageReq::parse(entry_text, None)
+      {
+        if package_req.name.starts_with("@")
+          && jsr_resolver
+            .req_to_nv(&package_req)
+            .await
+            .ok()
+            .flatten()
+            .is_some()
+        {
+          bail!(
+            "{entry_text} is missing a prefix. Did you mean `{}`?",
+            crate::colors::yellow(format!("deno install -g jsr:{package_req}"))
+          );
+        } else if npm_resolver
+          .req_to_nv(&package_req)
+          .await
+          .ok()
+          .flatten()
+          .is_some()
+        {
+          bail!(
+            "{entry_text} is missing a prefix. Did you mean `{}`?",
+            crate::colors::yellow(format!("deno install -g npm:{package_req}"))
+          );
+        }
+      }
+    }
+
+    factory
+      .main_module_graph_container()
+      .await?
+      .load_and_type_check_files(
+        std::slice::from_ref(module_url),
+        CollectSpecifiersOptions {
+          include_ignored_specified: true,
+        },
+      )
+      .await?;
 
-  // create the install shim
-  create_install_shim(
-    &bin_name_resolver,
-    cli_options.initial_cwd(),
-    &flags,
-    install_flags_global,
-  )
-  .await
+    let bin_name_resolver = factory.bin_name_resolver()?;
+    // create the install shim
+    create_install_shim(
+      &bin_name_resolver,
+      cli_options.initial_cwd(),
+      &flags,
+      &install_flags_global,
+      module_url,
+    )
+    .await?;
+  }
+  Ok(())
 }
 
 async fn create_install_shim(
   bin_name_resolver: &BinNameResolver<'_>,
   cwd: &Path,
   flags: &Flags,
-  install_flags_global: InstallFlagsGlobal,
+  install_flags_global: &InstallFlagsGlobal,
+  module_url: &str,
 ) -> Result<(), AnyError> {
-  let shim_data =
-    resolve_shim_data(bin_name_resolver, cwd, flags, &install_flags_global)
-      .await?;
+  let shim_data = resolve_shim_data(
+    bin_name_resolver,
+    cwd,
+    flags,
+    install_flags_global,
+    module_url,
+  )
+  .await?;
 
   // ensure directory exists
   if let Ok(metadata) = fs::metadata(&shim_data.installation_dir) {
@@ -953,13 +978,13 @@ async fn resolve_shim_data(
   cwd: &Path,
   flags: &Flags,
   install_flags_global: &InstallFlagsGlobal,
+  module_url: &str,
 ) -> Result<ShimData, AnyError> {
   let installation_dir =
     get_installer_bin_dir(cwd, install_flags_global.root.as_deref())?;
 
   // Check if module_url is remote
-  let module_url = resolve_url_or_path(&install_flags_global.module_url, cwd)?;
-
+  let module_url = resolve_url_or_path(module_url, cwd)?;
   let name = if install_flags_global.name.is_some() {
     install_flags_global.name.clone()
   } else {
@@ -1182,8 +1207,14 @@ mod tests {
     let npm_version_resolver = NpmVersionResolver::default();
     let resolver =
       BinNameResolver::new(&http_client, &registry_api, &npm_version_resolver);
-    super::create_install_shim(&resolver, &cwd, flags, install_flags_global)
-      .await
+    super::create_install_shim(
+      &resolver,
+      &cwd,
+      flags,
+      &install_flags_global,
+      &install_flags_global.module_urls[0],
+    )
+    .await
   }
 
   async fn resolve_shim_data(
@@ -1196,7 +1227,14 @@ mod tests {
     let npm_version_resolver = NpmVersionResolver::default();
     let resolver =
       BinNameResolver::new(&http_client, &registry_api, &npm_version_resolver);
-    super::resolve_shim_data(&resolver, &cwd, flags, install_flags_global).await
+    super::resolve_shim_data(
+      &resolver,
+      &cwd,
+      flags,
+      install_flags_global,
+      &install_flags_global.module_urls[0],
+    )
+    .await
   }
 
   #[tokio::test]
@@ -1208,7 +1246,7 @@ mod tests {
     create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1244,7 +1282,7 @@ mod tests {
     let shim_data = resolve_shim_data(
       &Flags::default(),
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1266,7 +1304,7 @@ mod tests {
     let shim_data = resolve_shim_data(
       &Default::default(),
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1294,7 +1332,7 @@ mod tests {
         ..Default::default()
       },
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1322,7 +1360,7 @@ mod tests {
     let shim_data = resolve_shim_data(
       &Flags::default(),
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/subdir/main.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/subdir/main.ts".to_string()],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1345,8 +1383,10 @@ mod tests {
     let shim_data = resolve_shim_data(
       &Flags::default(),
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4550/?redirect_to=/subdir/redirects/a.ts"
-          .to_string(),
+        module_urls: vec![
+          "http://localhost:4550/?redirect_to=/subdir/redirects/a.ts"
+            .to_string(),
+        ],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1372,7 +1412,7 @@ mod tests {
     let shim_data = resolve_shim_data(
       &Flags::default(),
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1403,7 +1443,7 @@ mod tests {
         ..Flags::default()
       },
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec!["--foobar".to_string()],
         name: Some("echo_test".to_string()),
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1439,7 +1479,7 @@ mod tests {
         ..Flags::default()
       },
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1471,7 +1511,7 @@ mod tests {
         ..Flags::default()
       },
       &InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1504,7 +1544,7 @@ mod tests {
         ..Flags::default()
       },
       &InstallFlagsGlobal {
-        module_url: "npm:cowsay".to_string(),
+        module_urls: vec!["npm:cowsay".to_string()],
         args: vec![],
         name: None,
         root: Some(temp_dir.to_string_lossy().into_owned()),
@@ -1541,7 +1581,7 @@ mod tests {
         ..Flags::default()
       },
       &InstallFlagsGlobal {
-        module_url: "npm:cowsay".to_string(),
+        module_urls: vec!["npm:cowsay".to_string()],
         args: vec![],
         name: None,
         root: Some(env::temp_dir().to_string_lossy().into_owned()),
@@ -1576,7 +1616,7 @@ mod tests {
     create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: local_module_str.to_string(),
+        module_urls: vec![local_module_str.to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1605,7 +1645,7 @@ mod tests {
     create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/echo_server.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1625,7 +1665,7 @@ mod tests {
     let no_force_result = create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/cat.ts".to_string(), // using a different URL
+        module_urls: vec!["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()),
@@ -1648,7 +1688,7 @@ mod tests {
     let force_result = create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/cat.ts".to_string(), // using a different URL
+        module_urls: vec!["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()),
@@ -1678,7 +1718,7 @@ mod tests {
         ..Flags::default()
       },
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/cat.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/cat.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1707,7 +1747,7 @@ mod tests {
     create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/echo_server.ts".to_string(),
+        module_urls: vec!["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()),
@@ -1747,7 +1787,7 @@ mod tests {
     create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: local_module_str.to_string(),
+        module_urls: vec![local_module_str.to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1792,7 +1832,7 @@ mod tests {
         ..Flags::default()
       },
       InstallFlagsGlobal {
-        module_url: "http://localhost:4545/cat.ts".to_string(),
+        module_urls: vec!["http://localhost:4545/cat.ts".to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),
@@ -1834,7 +1874,7 @@ mod tests {
     let result = create_install_shim(
       &Flags::default(),
       InstallFlagsGlobal {
-        module_url: file_module_string.to_string(),
+        module_urls: vec![file_module_string.to_string()],
         args: vec![],
         name: Some("echo_test".to_string()),
         root: Some(temp_dir.path().to_string()),

filename: tests/integration/install_tests.rs
patch: @@ -254,6 +254,7 @@ fn installer_test_local_module_run() {
       "--root",
       temp_dir_str.as_str(),
       echo_ts_str.as_str(),
+      "--",
       "hello",
     ])
     .envs([
@@ -293,7 +294,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 -g http://localhost:4545/echo.ts -- hello")
     .run()
     .skip_output_check()
     .assert_exit_code(0);
@@ -428,3 +429,48 @@ fn show_prefix_hint_on_global_install() {
   let output_text = output.combined_output();
   assert_contains!(output_text, "error: Module not found");
 }
+
+#[test]
+fn installer_multiple() {
+  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 --root ./root -g http://localhost:4545/echo.ts http://localhost:4545/cat.ts")
+    .run()
+    .assert_matches_text("[WILDCARD]Successfully installed echo[WILDCARD]Successfully installed cat[WILDCARD]")
+    .assert_exit_code(0);
+  for name in ["echo", "cat"] {
+    let mut bin_file_path = bin_dir.join(name);
+    if cfg!(windows) {
+      bin_file_path = bin_file_path.with_extension("cmd");
+    }
+    assert!(bin_file_path.exists());
+  }
+}
+
+#[test]
+fn installer_second_module_looks_like_script_argument() {
+  let context = TestContextBuilder::new()
+    .use_http_server()
+    .use_temp_cwd()
+    .build();
+  // in Deno < 3.0, we didn't require `--` before script arguments, so we try to provide a helpful
+  // error message for people migrating
+  context
+    .new_command()
+    .args("install --root ./root -g http://localhost:4545/echo.ts non_existent")
+    .run()
+    .assert_matches_text(concat!(
+      "[WILDCARD]Successfully installed echo[WILDCARD]error: non_existent is missing a prefix. ",
+      "Deno 3.0 requires `--` before script arguments in `deno install -g`. ",
+      "Did you mean `deno install -g http://localhost:4545/echo.ts -- non_existent`? ",
+      "Or maybe provide a `jsr:` or `npm:` prefix?\n"
+    ))
+    .assert_exit_code(1);
+}

filename: tests/specs/install/global/config_file_import_map/__test__.jsonc
patch: @@ -1,5 +1,5 @@
 {
   "tempDir": true,
-  "args": "install -g --root ./folder --config deno.json main.ts --name my-cli",
+  "args": "install -g --root ./folder --config deno.json --name my-cli main.ts",
   "output": "install.out"
 }