bundle cleanup: report failed removals

Library/Homebrew/bundle/extensions/cargo.rb

@@ -160,7 +160,7 @@ def installed_packages
           @installed_packages = packages.dup
         end
 
-        sig { override.params(name: String, executable: Pathname).void }
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
         def uninstall_package!(name, executable: Pathname.new(""))
           Bundle.system(executable.to_s, "uninstall", name, verbose: false)
         end

Library/Homebrew/bundle/extensions/extension.rb

@@ -9,6 +9,7 @@ module Bundle
 
     class Extension < Homebrew::Bundle::PackageType
       extend T::Helpers
+      extend Utils::Output::Mixin
 
       abstract!
 
@@ -63,9 +64,9 @@ def self.package_manager_executable!
         package_manager_executable || raise("#{package_manager_name} is not installed")
       end
 
-      sig { params(executable: Pathname).returns(T::Hash[String, String]) }
-      def self.package_manager_env(executable)
-        { "PATH" => "#{executable.dirname}:#{ORIGINAL_PATHS.join(":")}" }
+      sig { params(_executable: Pathname).returns(T::Hash[String, String]) }
+      def self.package_manager_env(_executable)
+        {}
       end
 
       sig {
@@ -239,17 +240,22 @@ def self.legacy_check_step
       sig { params(items: T::Array[String]).void }
       def self.cleanup!(items)
         executable = package_manager_executable
-        return if executable.nil?
+        if executable.nil?
+          ofail "Failed to uninstall #{banner_name}: executable not found" unless items.empty?
+          return
+        end
 
-        with_env(package_manager_env(executable)) do
-          items.each do |name|
-            uninstall_package!(name, executable:)
+        successes = with_env(package_manager_env(executable)) do
+          items.count do |item|
+            success = uninstall_package!(item, executable:)
+            ofail "Failed to uninstall #{banner_name}: #{cleanup_item_name(item)}" unless success
+            success
           end
         end
-        puts "Uninstalled #{items.size} #{banner_name}#{"s" if items.size != 1}"
+        puts "Uninstalled #{Utils.pluralize(package_description, successes, include_count: true)}"
       end
 
-      sig { params(name: String, executable: Pathname).void }
+      sig { params(name: String, executable: Pathname).returns(T::Boolean) }
       def self.uninstall_package!(name, executable: Pathname.new(""))
         raise NotImplementedError, "#{self} must override `uninstall_package!` or `cleanup!`."
       end

Library/Homebrew/bundle/extensions/flatpak.rb

@@ -408,12 +408,9 @@ def cleanup_items(entries)
           packages - kept_flatpaks
         end
 
-        sig { params(flatpaks: T::Array[String]).void }
-        def cleanup!(flatpaks)
-          flatpaks.each do |flatpak_name|
-            Kernel.system("flatpak", "uninstall", "-y", "--system", flatpak_name)
-          end
-          puts "Uninstalled #{flatpaks.size} flatpak#{"s" if flatpaks.size != 1}"
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
+        def uninstall_package!(name, executable: Pathname.new(""))
+          Kernel.system(executable.to_s, "uninstall", "-y", "--system", name) == true
         end
       end
 

Library/Homebrew/bundle/extensions/go.rb

@@ -98,7 +98,10 @@ def installed_packages
         sig { override.params(items: T::Array[String]).void }
         def cleanup!(items)
           go = package_manager_executable
-          return if go.nil?
+          if go.nil?
+            ofail "Failed to remove Go executables: go executable not found" unless items.empty?
+            return
+          end
 
           gobin = Utils.popen_read(go, "env", "GOBIN", err: :err).chomp
           gopath = Utils.popen_read(go, "env", "GOPATH", err: :err).chomp
@@ -118,10 +121,17 @@ def cleanup!(items)
             module_path = path_line.split("\t")[2]&.strip
             next unless items.include?(module_path)
 
-            FileUtils.rm_f(binary)
+            begin
+              File.unlink(binary)
+            rescue Errno::ENOENT
+              next
+            rescue SystemCallError => e
+              ofail "Failed to remove Go executable #{binary}: #{e}"
+              next
+            end
             removed += 1
           end
-          puts "Uninstalled #{removed} #{banner_name}#{"s" if removed != 1}"
+          puts "Removed #{Utils.pluralize("Go executable", removed, include_count: true)}"
         end
       end
     end

Library/Homebrew/bundle/extensions/krew.rb

@@ -33,6 +33,11 @@ def package_manager_executable
           @package_manager_executable ||= T.let(which("kubectl-krew", ORIGINAL_PATHS), T.nilable(Pathname))
         end
 
+        sig { override.params(executable: Pathname).returns(T::Hash[String, String]) }
+        def package_manager_env(executable)
+          { "PATH" => "#{executable.dirname}:#{ORIGINAL_PATHS.join(":")}" }
+        end
+
         sig { override.returns(T::Array[String]) }
         def packages
           packages = @packages
@@ -82,7 +87,7 @@ def parse_plugin_list(output)
         end
         private :parse_plugin_list
 
-        sig { override.params(name: String, executable: Pathname).void }
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
         def uninstall_package!(name, executable: Pathname.new(""))
           Bundle.system(executable.to_s, "uninstall", name, verbose: false)
         end

Library/Homebrew/bundle/extensions/mac_app_store.rb

@@ -154,15 +154,9 @@ def cleanup_items(entries)
                   .map { |app| cleanup_item(app) }
         end
 
-        sig { override.params(items: T::Array[String]).void }
-        def cleanup!(items)
-          mas = package_manager_executable
-          return if mas.nil?
-
-          items.each do |item|
-            Bundle.system(mas, "uninstall", parse_cleanup_item(item).id, verbose: false)
-          end
-          puts "Uninstalled #{items.size} Mac App Store app#{"s" if items.size != 1}"
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
+        def uninstall_package!(name, executable: Pathname.new(""))
+          Bundle.system(executable, "uninstall", parse_cleanup_item(name).id, verbose: false)
         end
 
         sig { params(id: Integer).returns(T::Boolean) }

Library/Homebrew/bundle/extensions/npm.rb

@@ -39,6 +39,11 @@ def package_manager_executable
           which("npm", ORIGINAL_PATHS)
         end
 
+        sig { override.params(executable: Pathname).returns(T::Hash[String, String]) }
+        def package_manager_env(executable)
+          { "PATH" => "#{executable.dirname}:#{ORIGINAL_PATHS.join(":")}" }
+        end
+
         sig { override.params(name: String, _options: Homebrew::Bundle::EntryOptions).returns(String) }
         def install_verb(name = "", _options = {})
           package_installed?(name) ? "Upgrading" : "Installing"
@@ -141,7 +146,7 @@ def outdated_packages
           @outdated_packages = []
         end
 
-        sig { override.params(name: String, executable: Pathname).void }
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
         def uninstall_package!(name, executable: Pathname.new(""))
           Bundle.system(executable.to_s, "uninstall", "-g", name, verbose: false)
         end

Library/Homebrew/bundle/extensions/uv.rb

@@ -363,7 +363,7 @@ def package_installed?(name, with: nil, source: nil)
           installed_packages.include?(package_record(name, with:, source:))
         end
 
-        sig { override.params(name: String, executable: Pathname).void }
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
         def uninstall_package!(name, executable: Pathname.new(""))
           Bundle.system(executable.to_s, "tool", "uninstall", name, verbose: false)
         end

Library/Homebrew/bundle/extensions/vscode_extension.rb

@@ -184,14 +184,9 @@ def cleanup_items(entries)
           packages - kept_extensions
         end
 
-        sig { params(extensions: T::Array[String]).void }
-        def cleanup!(extensions)
-          vscode = package_manager_executable
-          return if vscode.nil?
-
-          extensions.each do |extension|
-            Kernel.system(vscode.to_s, "--uninstall-extension", extension)
-          end
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
+        def uninstall_package!(name, executable: Pathname.new(""))
+          Kernel.system(executable.to_s, "--uninstall-extension", name) == true
         end
       end
     end

Library/Homebrew/bundle/extensions/winget.rb

@@ -322,17 +322,11 @@ def cleanup_items(entries)
           packages_to_cleanup.map { |app| cleanup_item(app) }
         end
 
-        sig { override.params(items: T::Array[String]).void }
-        def cleanup!(items)
-          winget = package_manager_executable
-          return if winget.nil?
-
-          items.each do |item|
-            app = parse_cleanup_item(item)
-            Bundle.system(winget, "uninstall", "--id", app.id, "--exact", "--source", app.source,
-                          "--accept-source-agreements", "--disable-interactivity", verbose: false)
-          end
-          puts "Uninstalled #{items.size} WinGet package#{"s" if items.size != 1}"
+        sig { override.params(name: String, executable: Pathname).returns(T::Boolean) }
+        def uninstall_package!(name, executable: Pathname.new(""))
+          app = parse_cleanup_item(name)
+          Bundle.system(executable, "uninstall", "--id", app.id, "--exact", "--source", app.source,
+                        "--accept-source-agreements", "--disable-interactivity", verbose: false)
         end
 
         sig { params(id: String, source: String).returns(T::Boolean) }

Library/Homebrew/bundle/subcommand/cleanup.rb

@@ -16,6 +16,8 @@ module Homebrew
   module Cmd
     class Bundle < Homebrew::AbstractCommand
       class CleanupSubcommand < Homebrew::AbstractSubcommand
+        extend Utils::Output::Mixin
+
         subcommand_args do
           usage_banner <<~EOS
             `brew bundle cleanup`:
@@ -164,16 +166,24 @@ def self.cleanup(global: false, file: nil, force: false, zap: false, dsl: nil,
               else
                 []
               end
-              Kernel.system HOMEBREW_BREW_FILE, "uninstall", "--cask", *args, "--force", *casks
-              puts "Uninstalled #{casks.size} cask#{"s" if casks.size != 1}"
+              if Kernel.system HOMEBREW_BREW_FILE, "uninstall", "--cask", *args, "--force", *casks
+                puts "Uninstalled #{Utils.pluralize("cask", casks.size, include_count: true)}"
+              else
+                ofail "Failed to uninstall casks. Some may have been removed."
+              end
             end
 
             if formulae.any?
-              Kernel.system HOMEBREW_BREW_FILE, "uninstall", "--formula", "--force", *formulae
-              puts "Uninstalled #{formulae.size} formula#{"e" if formulae.size != 1}"
+              if Kernel.system HOMEBREW_BREW_FILE, "uninstall", "--formula", "--force", *formulae
+                puts "Uninstalled #{Utils.pluralize("formula", formulae.size, include_count: true)}"
+              else
+                ofail "Failed to uninstall formulae. Some may have been removed."
+              end
             end
 
-            Kernel.system HOMEBREW_BREW_FILE, "untap", *taps if taps.any?
+            if taps.any? && !Kernel.system(HOMEBREW_BREW_FILE, "untap", *taps)
+              ofail "Failed to untap. Some taps may have been removed."
+            end
 
             cleanup_extensions.each do |extension, items|
               next if items.empty?

Library/Homebrew/test/bundle/cargo_spec.rb

@@ -395,6 +395,15 @@
       expect(described_class.cleanup_items(entries)).to eql(%w[fd-find bat])
     end
 
+    it "returns the uninstall command result" do
+      allow(Homebrew::Bundle).to receive(:system)
+        .with("/tmp/rust/bin/cargo", "uninstall", "example", verbose: false).and_return(false)
+
+      expect(
+        described_class.uninstall_package!("example", executable: Pathname("/tmp/rust/bin/cargo")),
+      ).to be(false)
+    end
+
     it "returns frozen empty array when cargo is not installed" do
       allow(described_class).to receive(:package_manager_installed?).and_return(false)
       entries = [Homebrew::Bundle::Dsl::Entry.new(:cargo, "ripgrep")]

Library/Homebrew/test/bundle/flatpak_spec.rb

@@ -6,6 +6,35 @@
 require "bundle/extensions/flatpak"
 
 RSpec.describe Homebrew::Bundle::Flatpak do
+  describe "cleanup" do
+    test_each([false, nil]) do |result|
+      it "continues after #{result.inspect} with the resolved executable and unchanged environment" do
+        Homebrew.failed = false
+        ENV["PATH"] = "/usr/bin:/bin"
+        allow(described_class).to receive(:package_manager_executable)
+          .and_return(Pathname("/example/bin/flatpak"))
+        expect(Kernel).to receive(:system)
+          .with("/example/bin/flatpak", "uninstall", "-y", "--system", "org.example.App").ordered do
+          expect(ENV.fetch("PATH")).to eq("/usr/bin:/bin")
+          result
+        end
+        expect(Kernel).to receive(:system)
+          .with("/example/bin/flatpak", "uninstall", "-y", "--system", "org.example.Second")
+          .ordered.and_return(true)
+
+        expect do
+          described_class.cleanup!(%w[org.example.App org.example.Second])
+        end.to(
+          change(Homebrew, :failed?).from(false).to(true)
+            .and(output(
+              a_string_including("Failed to uninstall Flatpak packages: org.example.App"),
+            ).to_stderr)
+            .and(output("Uninstalled 1 flatpak\n").to_stdout),
+        )
+      end
+    end
+  end
+
   describe "checking" do
     subject(:checker) { described_class.new }