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

Conversation

morishin
Copy link

This pull request is for a PoC.

As per #327, the volta uninstall command does not allow you to uninstall node or yarn.
However, I have many patch versions of node installed on my machine, and I wanted to delete older versions to free up unnecessary disk space.
Therefore, it is inconvenient that volta uninstall [email protected] is not possible.

I looked through the issues and Discord but couldn't understand why this has not been implemented for so many years, so I created a simple implementation for discussion.
I would like to know whether this approach is acceptable or if there are any concerns.

(The core of the implementation is here. The rest of the changes are not significant.)

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);
    } else {
        warn!("No version 'node@{}' found to uninstall", self.version);
    }

    Ok(())
}

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::from_str_and_version(&self.tool, version);

tool.uninstall()?;
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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants