Skip to content

Add add'l rescue for Utils.gain_permissions_rmdir#21508

Merged
dduugg merged 1 commit intomainfrom
unrescue-enotempty
Feb 3, 2026
Merged

Add add'l rescue for Utils.gain_permissions_rmdir#21508
dduugg merged 1 commit intomainfrom
unrescue-enotempty

Conversation

@dduugg
Copy link
Copy Markdown
Member

@dduugg dduugg commented Feb 3, 2026

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

Partial revert of #21499

Intended to resolve #21507

@bevanjkay
Copy link
Copy Markdown
Member

bevanjkay commented Feb 3, 2026

Essentially we shouldn't exit with 1 when Utils.gain_permissions_rmdir(resolved_path, command:) fails due to the folder not being empty. Using rmdir in the zap should only remove the folder if it is empty.

I added the rescue block to ensure that we don't exit 1 if the folder isn't empty, as this is expected behaviour, but it looks like maybe another error is being thrown in #21507

I can't replicate the error in the issue on main but I can with this PR checked out.

I'm not sure what caused the errors to start occurring with the sig change?

@dduugg dduugg force-pushed the unrescue-enotempty branch 2 times, most recently from 4bb7521 to 99b484c Compare February 3, 2026 03:38
@dduugg dduugg changed the title Don't rescue Errno::ENOTEMPTY in recursive_rmdir Add add'l rescue for Utils.gain_permissions_rmdir Feb 3, 2026
@dduugg
Copy link
Copy Markdown
Member Author

dduugg commented Feb 3, 2026

Ok, in that case, let's just rescue the add'l error class. PR Updated. I'll continue to investigate what is causing these errors to surface now. (There's an add'l, unrelated type fix in this PR from my experiments.)

Thanks for the elaboration and effort here, @bevanjkay

@dduugg dduugg requested a review from bevanjkay February 3, 2026 03:41
@dduugg dduugg marked this pull request as ready for review February 3, 2026 03:41
Copilot AI review requested due to automatic review settings February 3, 2026 03:41
@dduugg dduugg enabled auto-merge February 3, 2026 03:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a regression introduced in #21499 where brew uninstall --cask --zap would fail with a hard error when attempting to remove non-empty directories that require sudo permissions. The fix restores the previous behavior of gracefully skipping non-empty directories.

Changes:

  • Added .to_s to User.current in gain_permissions method for type safety
  • Added ErrorDuringExecution to the rescue clause in recursive_rmdir to catch errors from sudo-based rmdir operations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Library/Homebrew/cask/utils.rb Fixed type issue by explicitly converting User.current to string in chown command
Library/Homebrew/cask/artifact/abstract_uninstall.rb Added rescue for ErrorDuringExecution to gracefully handle non-empty directory errors when using sudo

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dduugg dduugg force-pushed the unrescue-enotempty branch from 99b484c to f7e0faf Compare February 3, 2026 04:14
@dduugg dduugg force-pushed the unrescue-enotempty branch from f7e0faf to ae110ca Compare February 3, 2026 05:18
@dduugg dduugg added this pull request to the merge queue Feb 3, 2026
Merged via the queue into main with commit 7788a02 Feb 3, 2026
37 checks passed
@dduugg dduugg deleted the unrescue-enotempty branch February 3, 2026 06:20
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.

Regression: brew uninstall --cask --zap fails with ErrorDuringExecution on non-empty directories

3 participants