Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Volta npm shim ignores the --dry-run flag #1597

Open
jasonmobley opened this issue Dec 19, 2023 · 3 comments
Open

Volta npm shim ignores the --dry-run flag #1597

jasonmobley opened this issue Dec 19, 2023 · 3 comments
Labels

Comments

@jasonmobley
Copy link

jasonmobley commented Dec 19, 2023

Running npm install -g --dry-run <package> should not actually install anything. Volta's npm shim doesn't honor the --dry-run flag and does the actual install.

→ volta ls yarn
⚡️ Yarn versions in your toolchain:

    v1.22.17
    v1.22.19
    v1.22.21 (default)

→ npm install -g --dry-run yarn
   note: using Volta to install Yarn
success: installed and set [email protected] as default

→ volta ls yarn
⚡️ Yarn versions in your toolchain:

    v1.22.17
    v1.22.19
    v1.22.21
    v4.0.2 (default)

→
@chriskrycho
Copy link
Contributor

Thanks for the report! Is this package-manager-specific behavior or do we get it wrong for regular packages as well?

@jasonmobley
Copy link
Author

Thanks for the report! Is this package-manager-specific behavior or do we get it wrong for regular packages as well?

I think it is specific to the special packages that volta considers "tools" like npm, pnpm, yarn, etc. But there's also a volta bug in the general package case, because it thinks it should be trying to identify a newly installed package when there isn't one.

→ volta ls gulp
⚡️ No tools or packages installed.

You can safely install packages by running `volta install <package name>`.
See `volta help install` for details and more options.

→ npm ls -g gulp
/Users/me/.volta/tools/image/node/16.20.2/lib
└── (empty)


→ npm install -g --dry-run gulp

added 336 packages in 8s

16 packages are looking for funding
  run `npm fund` for details
Volta error: Could not determine the name of the package that was just installed.

Please rerun the command that triggered this error with the environment
variable `VOLTA_LOGLEVEL` set to `debug` and open an issue at
https://github.com/volta-cli/volta/issues with the details!

@chriskrycho
Copy link
Contributor

Ah, very interesting. Thanks for flagging up that second issue as well! If you have time and interest to dig in, the relevant code is here:

use std::path::PathBuf;
use super::manager::PackageManager;
use crate::command::create_command;
use crate::error::{Context, ErrorKind, Fallible};
use crate::platform::Image;
use crate::style::progress_spinner;
use log::debug;
/// Use `npm install --global` to install the package
///
/// Sets the environment variable `npm_config_prefix` to redirect the install to the Volta
/// data directory, taking advantage of the standard global install behavior with a custom
/// location
pub(super) fn run_global_install(
package: String,
staging_dir: PathBuf,
platform_image: &Image,
) -> Fallible<()> {
let mut command = create_command("npm");
command.args(&[
"install",
"--global",
"--loglevel=warn",
"--no-update-notifier",
"--no-audit",
]);
command.arg(&package);
command.env("PATH", platform_image.path()?);
PackageManager::Npm.setup_global_command(&mut command, staging_dir);
debug!("Installing {} with command: {:?}", package, command);
let spinner = progress_spinner(format!("Installing {}", package));
let output_result = command
.output()
.with_context(|| ErrorKind::PackageInstallFailed {
package: package.clone(),
});
spinner.finish_and_clear();
let output = output_result?;
let stderr = String::from_utf8_lossy(&output.stderr);
debug!("[install stderr]\n{}", stderr);
debug!(
"[install stdout]\n{}",
String::from_utf8_lossy(&output.stdout)
);
if output.status.success() {
Ok(())
} else if stderr.contains("code E404") {
// npm outputs "code E404" as part of the error output when a package couldn't be found
// Detect that and show a nicer error message (since we likely know the problem in that case)
Err(ErrorKind::PackageNotFound { package }.into())
} else {
Err(ErrorKind::PackageInstallFailed { package }.into())
}
}

(No worries if not!)

chriskrycho added a commit that referenced this issue Dec 21, 2023
chriskrycho added a commit that referenced this issue Dec 21, 2023
Co-authored-by: Rob Jackson <[email protected]>
chriskrycho added a commit that referenced this issue Jan 10, 2024
Co-authored-by: Rob Jackson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants