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

feat: call make uninstall when removing a plugin/snippet #468

Closed
wants to merge 1 commit into from

Conversation

psprint
Copy link
Contributor

@psprint psprint commented Jan 29, 2023

Description

If the uninstall target is there in the Makefile of the removed plugin/snippet, it'll be run as: make -C {plugin/snippet dir} uninstall before removing the dir (with zinit delete).

Motivation and Context

@vladdoster mentioned (#333) that --prefix=$ZPFX installed plugins aren't cleaned up lik --prefix=$PWD ones, so I added make uninstall support.

Related Issue(s)

#333

Usage examples

# Will first uninstall ctags from the --prefix=($ZPFX,$PWD,etc.) path
zinit delete -y unversal-ctags/ctags

2023-01-29-122000_1901x884_scrot

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@vladdoster
Copy link
Member

DRAFT RESPONSE

This is a good idea. However, I still favor using plugin source directory instead of $ZPFX as a prefix for the following reasons.

@psprint
Copy link
Contributor Author

psprint commented Feb 9, 2023 via email

@psprint
Copy link
Contributor Author

psprint commented Mar 29, 2023

Ping?

@vladdoster
Copy link
Member

@psprint,

As I have said before, I will not review PRs with failing CI checks except when the CI tests are broken or you can't get a check to pass.

So, is there an issue generating the documentation?

@psprint
Copy link
Contributor Author

psprint commented Apr 4, 2023

Yes I cannot generate docs because there is no docker for PCLinuxOS. I've tried RPM for Fedora but it wasn't compatible.

@vladdoster
Copy link
Member

vladdoster commented Apr 29, 2023

@psprint,

This doesn't work for an example you gave in #458

zi id-as as"null" from"gitlab.matrix.org" configure make'install' for matrix-org/olm

I tried your commit before I changed it. Seems to be since it uses cmake?

Screenshot 2023-04-28 at 23 18 46

@psprint
Copy link
Contributor Author

psprint commented Apr 29, 2023 via email

@psprint
Copy link
Contributor Author

psprint commented Apr 29, 2023

@vladdoster It turns out that CMake doesn't support uninstall target like Autotools do. However, one can simply rm -f files in install_manifest.txt. So this could be added to the PR.
https://stackoverflow.com/questions/41471620/cmake-support-make-uninstall

@psprint
Copy link
Contributor Author

psprint commented Apr 29, 2023

@vladdoster: I've resolved conflicts (fresh branch) and added CMake support. The commit is all lowercase and has the prefix, I cannot generate zshelldocs because there's no docker for my OS.

@vladdoster
Copy link
Member

vladdoster commented May 1, 2023

@vladdoster: I've resolved conflicts (fresh branch) and added CMake support. The commit is all lowercase and has the prefix, and I cannot generate zshelldocs because there's no docker for my OS.

@psprint,

FWIW, You don't need Docker to generate the documentation. I don't use Docker, either, because it is painfully slow (on macOS atleast) due to virtualization.

OS time
macOS 1m 41s
Docker 6m 15s

I use the following:

zi for id-as lbin'!build/zsd*' light-mode make'--always-make' null @zdharma-continuum/zshelldoc

Screenshot 2023-04-30 at 22 33 56

Note
If working on zshelldoc, add --always-make, so zinit update --urge will recreate the files.

Screenshot 2023-04-30 at 22 28 54

@vladdoster
Copy link
Member

Closing due to the changes were merged in #616 (code found here).

@vladdoster vladdoster closed this Feb 1, 2024
@zdharma-continuum zdharma-continuum locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants