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

poc: Enable volta uninstall [email protected] #1658

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/volta-core/src/tool/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ pub trait Tool: Display {
fn install(self: Box<Self>, session: &mut Session) -> Fallible<()>;
/// Pin a tool in the local project so that it is usable within the project
fn pin(self: Box<Self>, session: &mut Session) -> Fallible<()>;
/// Uninstall a tool
fn uninstall(self: Box<Self>, session: &mut Session) -> Fallible<()>;
}

/// Specification for a tool and its associated version.
Expand Down
21 changes: 19 additions & 2 deletions crates/volta-core/src/tool/node/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ use super::{
info_project_version, FetchStatus, Tool,
};
use crate::error::{ErrorKind, Fallible};
use crate::fs::remove_dir_if_exists;
use crate::inventory::node_available;
use crate::layout::volta_home;
use crate::session::Session;
use crate::style::{note_prefix, tool_version};
use crate::style::{note_prefix, success_prefix, tool_version};
use crate::sync::VoltaLock;
use cfg_if::cfg_if;
use log::info;
use log::{info, warn};
use node_semver::Version;

mod fetch;
Expand Down Expand Up @@ -244,6 +246,21 @@ impl Tool for Node {
Err(ErrorKind::NotInPackage.into())
}
}
fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
let home = volta_home()?;
// Acquire a lock on the Volta directory, if possible, to prevent concurrent changes
let _lock: Result<VoltaLock, crate::error::VoltaError> = VoltaLock::acquire();

let node_dir = home.node_image_root_dir().join(self.version.to_string());
if node_dir.exists() {
remove_dir_if_exists(&node_dir)?;
info!("{} 'node@{}' uninstalled", success_prefix(), self.version);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe remove node-version.tar.gz in home.tmp_dir is also needed

} else {
warn!("No version 'node@{}' found to uninstall", self.version);
}

Ok(())
}
}

impl Display for Node {
Expand Down
13 changes: 13 additions & 0 deletions crates/volta-core/src/tool/npm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ impl Tool for Npm {
Err(ErrorKind::NotInPackage.into())
}
}
fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
Err(ErrorKind::Unimplemented {
feature: "Uninstalling npm".into(),
}
.into())
}
}

impl Display for Npm {
Expand Down Expand Up @@ -168,6 +174,13 @@ impl Tool for BundledNpm {
None => Err(ErrorKind::NotInPackage.into()),
}
}

fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
Err(ErrorKind::Unimplemented {
feature: "Uninstalling bundled npm".into(),
}
.into())
}
}

impl Display for BundledNpm {
Expand Down
4 changes: 4 additions & 0 deletions crates/volta-core/src/tool/package/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ impl Tool for Package {
fn pin(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
Err(ErrorKind::CannotPinPackage { package: self.name }.into())
}

fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
uninstall(&self.name)
}
}

impl Display for Package {
Expand Down
14 changes: 14 additions & 0 deletions crates/volta-core/src/tool/pnpm/mod.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use node_semver::Version;
use std::env;
use std::fmt::{self, Display};

use crate::error::{ErrorKind, Fallible};
use crate::inventory::pnpm_available;
use crate::session::Session;
use crate::style::tool_version;
use crate::sync::VoltaLock;
use crate::VOLTA_FEATURE_PNPM;

use super::{
check_fetched, debug_already_fetched, info_fetched, info_installed, info_pinned,
Expand All @@ -15,6 +17,7 @@ use super::{
mod fetch;
mod resolve;

use super::package::uninstall;
pub use resolve::resolve;

/// The Tool implementation for fetching and installing pnpm
Expand Down Expand Up @@ -87,6 +90,17 @@ impl Tool for Pnpm {
Err(ErrorKind::NotInPackage.into())
}
}

fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
if env::var_os(VOLTA_FEATURE_PNPM).is_some() {
Err(ErrorKind::Unimplemented {
feature: "Uninstalling pnpm".into(),
}
.into())
} else {
uninstall("pnpm")
}
}
}

impl Display for Pnpm {
Expand Down
6 changes: 6 additions & 0 deletions crates/volta-core/src/tool/yarn/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ impl Tool for Yarn {
Err(ErrorKind::NotInPackage.into())
}
}
fn uninstall(self: Box<Self>, _session: &mut Session) -> Fallible<()> {
Err(ErrorKind::Unimplemented {
feature: "Uninstalling yarn".into(),
}
.into())
}
}

impl Display for Yarn {
Expand Down
7 changes: 2 additions & 5 deletions src/command/uninstall.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use volta_core::error::{ExitCode, Fallible};
use volta_core::session::{ActivityKind, Session};
use volta_core::tool;
use volta_core::version::VersionSpec;

use crate::command::Command;

Expand All @@ -15,10 +14,8 @@ impl Command for Uninstall {
fn run(self, session: &mut Session) -> Fallible<ExitCode> {
session.add_event_start(ActivityKind::Uninstall);

let version = VersionSpec::default();
let tool = tool::Spec::from_str_and_version(&self.tool, version);

tool.uninstall()?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uninstall is a function of Spec. ↓

/// Uninstall a tool, removing it from the local inventory
///
/// This is implemented on Spec, instead of Resolved, because there is currently no need to
/// resolve the specific version before uninstalling a tool.
pub fn uninstall(self) -> Fallible<()> {

What I want to do now is to perform an uninstall with a specified version like volta uninstall [email protected], so Spec::uninstall is not appropriate. Therefore, I considered defining Tool::uninstall. (Since Tool is the return type of Spec::resolve, I believe that "Resolved" mentioned in this comment refers to Tool).

But I'm not sure that I can replace Spec::uninstall with the Tool::uninstall completely.

let tool = tool::Spec::try_from_str(&self.tool)?;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

        for tool in Spec::from_strings(&self.tools, "uninstall")? {
            tool.resolve(session)?.uninstall(session)?;
        }

just the same way as install which enable to install/uninstall multiple node/package once.

tool.resolve(session)?.uninstall(session)?;

session.add_event_end(ActivityKind::Uninstall, ExitCode::Success);
Ok(ExitCode::Success)
Expand Down
Loading