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

flox: update zap stanza #176407

Closed
wants to merge 1 commit into from
Closed

Conversation

tomberek
Copy link

@tomberek tomberek commented Jun 11, 2024

Move full uninstallation script to "zap".

Also bump to 1.0.7 (edit: merged elsewhere)

@tomberek tomberek marked this pull request as draft June 11, 2024 15:45
@razvanazamfirei
Copy link
Member

What happens if we run brew uninstall flox? Which files remain?

@tomberek
Copy link
Author

What happens if we run brew uninstall flox? Which files remain?

Still working on this and open to any ideas. The thought is that the Nix Store remains. Removing it and then downloading various portions is possible, just slow, duplicative, and frustrating for users. It also means that the history of older generations might not be recoverable either.

It's a bit like Anaconda (https://github.com/Homebrew/homebrew-cask/blob/d63d8b045ea5f4f96b7c6ceb793d96fa652fb433/Casks/m/miniconda.rb), where much of the content is only removed with zap.

@tomberek tomberek force-pushed the tomberek.nix_is_cache branch 2 times, most recently from 561a2b3 to bcc207b Compare June 12, 2024 04:44
@tomberek tomberek marked this pull request as ready for review June 12, 2024 05:08
@tomberek
Copy link
Author

tomberek commented Jun 13, 2024

Updated with a bump to 1.0.7.

Updated to transform a brew uninstall flox to leave a functioning daemon-less Store behind. This can then be adopted by a subsequent brew install flox. This makes brew upgrade flox function as expected without removal of the cache.

@krehel
Copy link
Member

krehel commented Jun 14, 2024

Hey Tom, nice to see you again! 👋

Trying to figure out how we should approach this one. Probably too much logic in the Caskfile that should be shunted to the package. I’m not deep into this but: is there a reason these postflight commands can’t be run as part of your installer / uninstall package?

For the zap: it would be more amenable to print something to the user to action as a final cleanup step. Again: is there any part of this can that be moved to your package, and leave some minimal more destructive cleanup action to the zap or to the user to run manually?

@tomberek
Copy link
Author

Hey Tom, nice to see you again! 👋

👋

Trying to figure out how we should approach this one. Probably too much logic in the Caskfile that should be shunted to the package. I’m not deep into this but: is there a reason these postflight commands can’t be run as part of your installer / uninstall package?

The *flight commands are specifics for brew support; therefore I'd prefer it to retain locality with the Cask. We don't want these run for a non-brew .pkg install because things like upgrade instructions and upgrade behavior is different.

We are also supporting .pkg install/uninstall. So an option would be to have two separate uninstall scripts, one for .pkg and one for brew. That was my first approach, but the uninstall script ended up getting uninstalled itself during brew uninstall flox, making it unavailable for a subsequent zap operation. I ended up inlining the un-installation into the cask as a workaround. Can this be solved with a pkgutil exception or another idiomatic mechanism?

For the zap: it would be more amenable to print something to the user to action as a final cleanup step. Again: is there any part of this can that be moved to your package, and leave some minimal more destructive cleanup action to the zap or to the user to run manually?

There are a few steps needed for zap: so something like brew uninstall --zap --force flox should be a nice way to run the relevant cleanup, even if you happen to be in an intermediate state. Is there a way to reference an uninstallation script in a package that can run even if someone has already done an uninstall?

@tomberek tomberek force-pushed the tomberek.nix_is_cache branch 3 times, most recently from 6db37da to 8776e22 Compare June 21, 2024 21:08
@tomberek
Copy link
Author

@krehel I've updated the method to be cleaner and remove the postflight stanzas. There is some more cleanup we can do after 1.1.0 is out as well, but I'd like to have the auto-bump system have a chance to work.

@stahnma
Copy link
Contributor

stahnma commented Jun 23, 2024

Note the 1.0.7 bump was handled in another PR, but I can't edit @tomberek's description.

@tomberek tomberek changed the title flox: consider Store to be a cache to prevent upgrade from removing flox: update zap stanza Jun 25, 2024
@stahnma
Copy link
Contributor

stahnma commented Jun 26, 2024

Do we need to do something to get this re-reviewed? /cc @krehel @razvanazamfirei

@tomberek
Copy link
Author

tomberek commented Jul 2, 2024

Simplified and rebase'd.

@p-linnane
Copy link
Member

Sorry, going to pass on this. I'm not comfortable adding an entire script inline.

@p-linnane p-linnane closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants