Modernize macOS uninstaller: fix service management and add force mode #2423
+773
−25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The pmproxy service was incorrectly installing io.pcp.pmlogger.plist instead of io.pcp.pmproxy.plist, causing the pmproxy service to use the wrong configuration. Issue performancecopilot#2347
- Replace obsolete /Library/StartupItems with modern launchctl commands - Support both 'bootout' (macOS 10.11+) and 'unload' for compatibility - Add support for all 4 services: pmcd, pmie, pmlogger, pmproxy (previously only handled pmcd and pmlogger) - Use pkgutil for modern package receipt management - Explicitly remove LaunchDaemons plist files - Preserve config/log files with user notification This addresses the core service management issues reported in performancecopilot#2347 where the uninstaller failed because StartupItems no longer exist on modern macOS systems. Issue performancecopilot#2347
- Use mktemp(1) for secure temp directory creation instead of hardcoded /tmp paths - Add -f/--force flag to skip confirmation prompt and perform complete removal including config/data/log directories - Add --help flag for usage information - Implement selective aggressive cleanup in force mode that removes /etc/pcp/, /var/lib/pcp/, and /var/log/pcp/ This addresses Nathan Scott's recommendations in performancecopilot#2347 and solves the "Directory not empty" errors by providing a force mode that removes all PCP-related directories even when they contain files created post-installation. Issue performancecopilot#2347
- Add HOMEBREW_TAP_UPDATES.md with recommendations for aligning the Homebrew tap with .pkg uninstaller improvements - Add TESTING_GUIDE.md with comprehensive manual and automated testing procedures for macOS uninstallation - Document force mode usage and testing procedures - Include UTM-based testing strategies and CI/CD recommendations Issue performancecopilot#2347
…mode In force mode, the install-pcp script's "Directory not empty" warnings are confusing since we're about to forcefully remove those directories with rm -rf anyway. Redirect stderr to /dev/null for install-pcp when running in force mode to provide cleaner output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@natoscott I think this is good to review and merge if you're ok with it. I've tested manually both force and non-force modes in a UTM VM, they seem to be good. I am going to now aim to integrate any necessary changes into the Homebrew cask repo so that it can uninstall things cleanly there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR modernizes the macOS uninstaller to fix critical issues reported in #2347 and implements all recommendations from @natoscott.
Problem
The current
uninstall-pcpscript has several critical issues:/Library/StartupItemswhich hasn't existed on macOS for yearspkgutil/tmppathsChanges
This PR addresses all issues through 4 logical commits:
Commit 1: Fix pmproxy plist typo
Commit 2: Modernize service management
/Library/StartupItemswith modernlaunchctlcommandsbootout(macOS 10.11+) andunloadfor compatibilitypkgutil --forgetfor modern package receipt managementCommit 3: Add force mode and mktemp
mktemp(1)for secure temp directory creation-f/--forceflag to skip confirmation and perform complete removal--helpflag for usage information/etc/pcp//var/lib/pcp//var/log/pcp/Commit 4: Documentation
HOMEBREW_TAP_UPDATES.mdwith recommendations for aligning Homebrew tapTESTING_GUIDE.mdwith comprehensive testing proceduresTesting
Standard uninstall preserves config/logs:
Force mode removes everything:
See
build/mac/TESTING_GUIDE.mdfor comprehensive testing procedures.Impact
Notes
This PR is marked as DRAFT for manual testing on macOS VMs before final review.
Fixes #2347