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

Add uninstall subcommand #112

Merged
merged 70 commits into from
Nov 27, 2024
Merged

Add uninstall subcommand #112

merged 70 commits into from
Nov 27, 2024

Conversation

marcoesters
Copy link
Contributor

@marcoesters marcoesters commented Nov 13, 2024

Description

Uninstalling distributions for conda currently only has incomplete solutions with files and broken auto-run/initializer scripts left behind (see conda/constructor#642, conda/constructor#588, and conda/constructor#572). A clean uninstallation is currently not available without manually running a series of commands that are still not 100% safe - e.g., conda init --reverse --all will remove initialization of conda, no matter what installation it is pointed to.

This PR introduces a cross-platform uninstallation subcommand that uses conda-internal features to perform the following tasks:

  • Uninstall all environments within a base environment or environments directory (such as a directory set by envs_dirs). This uninstalls all shortcuts, performs pre-unlink actions, and de-registers the environments from the environments.txt file.
  • Runs conda init reverse if a configuration file points to any of the environments that are removed.
  • Allows a user to remove .condarc files, either for the user, system-wide, or all.
  • Allows the user to run conda clean --all to remove package caches outside the installation directory.
  • Allows the user to remove cache directories, including the .conda file.

This is implemented in conda-standalone so that a single binary can be used on macOS and Linux. It also allows decoupling of the installer "front end" (built by constructor) from the installer "back end" (such as conda-standalone or micromamba), as outlined in the following issue: conda/constructor#549

Known gap: Soft links are currently not well supported: files are unlinked, but no efforts are made to ensure that this doesn't leave unused files behind.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

src/entry_point.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
),
)
uninstall_subcommand.add_argument(
"--remove-caches",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"--remove-caches",
"--remove-additional-caches",

The term is a bit convoluted with pkgs/ and pkgs/cache being taken care of in conda-clean, but this argument targets ~/.conda and anaconda-client stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately, conda overloads the term "cache". I changed it to --remove-conda-caches because it's more descriptive than --remove-additional-caches.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think that if a user sees both --conda-clean and --remove-conda-caches might get confused about the difference between both. Maybe the best option is to split it explicitly: --remove-anaconda-client-data, --remove-conda-notices-data. I'd argue the second one belongs in conda clean responsibilities though, so what iffff:

  • The --conda-clean flag also takes care of conda notices, there's no choice. Eventually we upstream this to conda.
  • Folks can optionally remove anaconda-client data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the best option is to split it explicitly: --remove-anaconda-client-data, --remove-conda-notices-data.

I think this will result in an overwhelming amount of CLI options, especially for inexperienced users. You would then also need another flag for the ~/.conda directory. That's a total of five CLI flags of options for the first iteration.

The reason why I separated conda clean is because I can think of a use case for conda clean that isn't covered by the removing cache use case: removing all your caches, especially ~/.conda really only makes sense if you are removing the last installation on your system.

On the other hand, conda clean has its uses when you still have an installation left on your system, but share a package cache. Here, conda clean will remove unused packages and free space on your system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm more amenable to the option that --clean-caches removes the notices cache and runs conda clean and that --clean-conda-data or --clean-user-data removes ~/.conda and anaconda-client data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I think about this (and after talking to a conda maintainer) the more convinced I am that we shouldn't handle anaconda-client data here. This should be handled by the package instead. Otherwise, we would have to implement clean-up operations for other packages or an arbitrary selection.

I think conda-standalone should only handle conda files. So, my final proposal is:

  • --clean-caches to run conda clean and remove the notices cache.
  • --remove-user-data to just delete ~/.conda for now.

This also follows somewhat the XDG_* convention where we have cache, data, and configs (here .condarc files). In that case, I can also rename --remove-condarcs to --remove-config-files to allow constructor to use more generic terms.

What do you think about this solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this last proposal and the XDG rationale. I think we should stick to a verb only: either clean, clear or remove, but not a mix. So pick one (using clean as an example) and then we say:

  • --clean-caches
  • --clean-user-data
  • --clean-config-files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer remove-* because it tells the user that files will be deleted.

src/entry_point.py Outdated Show resolved Hide resolved
@marcoesters marcoesters requested a review from jaimergp November 19, 2024 23:57
src/entry_point.py Outdated Show resolved Hide resolved
@marcoesters marcoesters requested a review from jaimergp November 25, 2024 18:58
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/entry_point.py Outdated Show resolved Hide resolved
Comment on lines +372 to +375
if ON_WIN:
system_condarc = Path("C:/ProgramData/conda/.condarc")
else:
system_condarc = Path("/etc/conda/.condarc")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we rely on env vars like PROGRAMDATA and XDG_DATA_* for this test? Or do you think it's not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree that we should rely on PROGRAMDATA, but unfortunately, conda doesn't use those variables: https://github.com/conda/conda/blob/main/conda/base/constants.py#L29-L42

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's probably worth an issue in conda/conda, but not a blocker here.

marcoesters and others added 2 commits November 26, 2024 10:10
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Copy link
Collaborator

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks for making it all the way through the leeengthy review!

@marcoesters marcoesters merged commit dce4ddb into conda:main Nov 27, 2024
7 checks passed
@marcoesters marcoesters deleted the uninstallation branch November 27, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants