Improve writable directory handling

Consolidate the handling of which directories need to exist and which
need to be writable. Additionally, add a fatal check for formula
installations to ensure that any directories that need to be writable
are so before attempting an installation.

Fixes #4626.

Library/Homebrew/cleanup.rb

@@ -271,8 +271,8 @@ module Homebrew
     def cleanup_lockfiles(*lockfiles)
       return if dry_run?
 
-      if lockfiles.empty? && HOMEBREW_LOCK_DIR.directory?
+      if lockfiles.empty? && HOMEBREW_LOCKS.directory?
-        lockfiles = HOMEBREW_LOCK_DIR.children.select(&:file?)
+        lockfiles = HOMEBREW_LOCKS.children.select(&:file?)
       end
 
       lockfiles.each do |file|
@@ -288,11 +288,16 @@ module Homebrew
     end
 
     def rm_ds_store(dirs = nil)
-      dirs ||= %w[Caskroom Cellar Frameworks Library bin etc include lib opt sbin share var]
+      dirs ||= begin
-               .map { |path| HOMEBREW_PREFIX/path }
+        Keg::MUST_EXIST_DIRECTORIES + [
-
+          HOMEBREW_CELLAR,
+          HOMEBREW_PREFIX/"Caskroom",
+        ]
+      end
       dirs.select(&:directory?).each.parallel do |dir|
-        system_command "find", args: [dir, "-name", ".DS_Store", "-delete"], print_stderr: false
+        system_command "find",
+          args: [dir, "-name", ".DS_Store", "-delete"],
+          print_stderr: false
       end
     end
   end

Library/Homebrew/cmd/reinstall.rb

@@ -14,6 +14,8 @@ module Homebrew
   def reinstall
     FormulaInstaller.prevent_build_flags unless DevelopmentTools.installed?
 
+    Install.perform_preinstall_checks
+
     ARGV.resolved_formulae.each do |f|
       if f.pinned?
         onoe "#{f.full_name} is pinned. You must unpin it to reinstall."

Library/Homebrew/config.rb

@@ -24,7 +24,7 @@ HOMEBREW_LINKED_KEGS = HOMEBREW_PREFIX/"var/homebrew/linked"
 HOMEBREW_PINNED_KEGS = HOMEBREW_PREFIX/"var/homebrew/pinned"
 
 # Where we store lock files
-HOMEBREW_LOCK_DIR = HOMEBREW_PREFIX/"var/homebrew/locks"
+HOMEBREW_LOCKS = HOMEBREW_PREFIX/"var/homebrew/locks"
 
 # Where we store built products
 HOMEBREW_CELLAR = Pathname.new(ENV["HOMEBREW_CELLAR"])

Library/Homebrew/diagnostic.rb

@@ -71,6 +71,12 @@ module Homebrew
       end
       ############# END HELPERS
 
+      def fatal_install_checks
+        %w[
+          check_access_directories
+        ].freeze
+      end
+
       def development_tools_checks
         %w[
           check_for_installed_developer_tools
@@ -293,115 +299,35 @@ module Homebrew
         EOS
       end
 
-      def check_access_homebrew_repository
+      def check_exist_directories
-        return if HOMEBREW_REPOSITORY.writable_real?
+        not_exist_dirs = Keg::MUST_EXIST_DIRECTORIES.reject(&:exist?)
+        return if not_exist_dirs.empty?
 
         <<~EOS
-          #{HOMEBREW_REPOSITORY} is not writable.
+          The following directories do not exist:
+          #{not_exist_dirs.join("\n")}
 
-          You should change the ownership and permissions of #{HOMEBREW_REPOSITORY}
+          You should create these directories and change their ownership to your account.
-          back to your user account.
+            sudo mkdir -p #{not_exist_dirs.join(" ")}
-            sudo chown -R $(whoami) #{HOMEBREW_REPOSITORY}
+            sudo chown -R $(whoami) #{not_exist_dirs.join(" ")}
         EOS
       end
 
-      def check_access_prefix_directories
+      def check_access_directories
-        not_writable_dirs = []
+        not_writable_dirs =
-
+          Keg::MUST_BE_WRITABLE_DIRECTORIES.select(&:exist?)
-        Keg::ALL_TOP_LEVEL_DIRECTORIES.each do |dir|
+                                           .reject(&:writable_real?)
-          path = HOMEBREW_PREFIX/dir
-          next unless path.exist?
-          next if path.writable_real?
-          not_writable_dirs << path
-        end
-
         return if not_writable_dirs.empty?
 
         <<~EOS
-          The following directories are not writable:
+          The following directories are not writable by your user:
           #{not_writable_dirs.join("\n")}
 
-          This can happen if you "sudo make install" software that isn't managed
+          You should change the ownership of these directories to your user.
-          by Homebrew. If a formula tries to write a file to this directory, the
-          install will fail during the link step.
-
-          You should change the ownership of these directories to your account.
             sudo chown -R $(whoami) #{not_writable_dirs.join(" ")}
         EOS
       end
 
-      def check_access_site_packages
-        return unless Language::Python.homebrew_site_packages.exist?
-        return if Language::Python.homebrew_site_packages.writable_real?
-
-        <<~EOS
-          #{Language::Python.homebrew_site_packages} isn't writable.
-          This can happen if you "sudo pip install" software that isn't managed
-          by Homebrew. If you install a formula with Python modules, the install
-          will fail during the link step.
-
-          You should change the ownership and permissions of #{Language::Python.homebrew_site_packages}
-          back to your user account.
-            sudo chown -R $(whoami) #{Language::Python.homebrew_site_packages}
-        EOS
-      end
-
-      def check_access_lock_dir
-        return unless HOMEBREW_LOCK_DIR.exist?
-        return if HOMEBREW_LOCK_DIR.writable_real?
-
-        <<~EOS
-          #{HOMEBREW_LOCK_DIR} isn't writable.
-          Homebrew writes lock files to this location.
-
-          You should change the ownership and permissions of #{HOMEBREW_LOCK_DIR}
-          back to your user account.
-            sudo chown -R $(whoami) #{HOMEBREW_LOCK_DIR}
-        EOS
-      end
-
-      def check_access_logs
-        return unless HOMEBREW_LOGS.exist?
-        return if HOMEBREW_LOGS.writable_real?
-
-        <<~EOS
-          #{HOMEBREW_LOGS} isn't writable.
-          Homebrew writes debugging logs to this location.
-
-          You should change the ownership and permissions of #{HOMEBREW_LOGS}
-          back to your user account.
-            sudo chown -R $(whoami) #{HOMEBREW_LOGS}
-        EOS
-      end
-
-      def check_access_cache
-        return unless HOMEBREW_CACHE.exist?
-        return if HOMEBREW_CACHE.writable_real?
-
-        <<~EOS
-          #{HOMEBREW_CACHE} isn't writable.
-          This can happen if you run `brew install` or `brew fetch` as another user.
-          Homebrew caches downloaded files to this location.
-
-          You should change the ownership and permissions of #{HOMEBREW_CACHE}
-          back to your user account.
-            sudo chown -R $(whoami) #{HOMEBREW_CACHE}
-        EOS
-      end
-
-      def check_access_cellar
-        return unless HOMEBREW_CELLAR.exist?
-        return if HOMEBREW_CELLAR.writable_real?
-
-        <<~EOS
-          #{HOMEBREW_CELLAR} isn't writable.
-
-          You should change the ownership and permissions of #{HOMEBREW_CELLAR}
-          back to your user account.
-            sudo chown -R $(whoami) #{HOMEBREW_CELLAR}
-        EOS
-      end
-
       def check_multiple_cellars
         return if HOMEBREW_PREFIX.to_s == HOMEBREW_REPOSITORY.to_s
         return unless (HOMEBREW_REPOSITORY/"Cellar").exist?

Library/Homebrew/formula_installer.rb

@@ -205,7 +205,7 @@ class FormulaInstaller
   def install
     start_time = Time.now
     if !formula.bottle_unneeded? && !pour_bottle? && DevelopmentTools.installed?
-      Homebrew::Install.check_development_tools
+      Homebrew::Install.perform_development_tools_checks
     end
 
     # not in initialize so upgrade can unlink the active keg before calling this
@@ -893,7 +893,7 @@ class FormulaInstaller
         sandbox.allow_write_xcode
         sandbox.deny_write_homebrew_repository
         sandbox.allow_write_cellar(formula)
-        Keg::TOP_LEVEL_DIRECTORIES.each do |dir|
+        Keg::KEG_LINK_DIRECTORIES.each do |dir|
           sandbox.allow_write_path "#{HOMEBREW_PREFIX}/#{dir}"
         end
         sandbox.exec(*args)

Library/Homebrew/install.rb

@@ -17,39 +17,36 @@ module Homebrew
       end
     end
 
-    def check_writable_install_location
+    def attempt_directory_creation
-      if HOMEBREW_CELLAR.exist? && !HOMEBREW_CELLAR.writable_real?
+      Keg::MUST_BE_WRITABLE_DIRECTORIES.each do |dir|
-        raise "Cannot write to #{HOMEBREW_CELLAR}"
+        begin
+          FileUtils.mkdir_p(dir) unless dir.exist?
+        rescue
+          nil
+        end
       end
-      prefix_writable = HOMEBREW_PREFIX.writable_real? || HOMEBREW_PREFIX.to_s == "/usr/local"
-      raise "Cannot write to #{HOMEBREW_PREFIX}" unless prefix_writable
     end
 
-    def check_development_tools
+    def perform_development_tools_checks
-      checks = Diagnostic::Checks.new
+      fatal_checks(:fatal_development_tools_checks)
+    end
+
+    def perform_preinstall_checks
+      check_ppc
+      attempt_directory_creation
+      fatal_checks(:fatal_install_checks)
+    end
+
+    def fatal_checks(type)
+      @checks ||= Diagnostic::Checks.new
       failed = false
-      checks.fatal_development_tools_checks.each do |check|
+      @checks.public_send(type).each do |check|
-        out = checks.send(check)
+        out = @checks.public_send(check)
         next if out.nil?
         failed ||= true
         ofail out
       end
       exit 1 if failed
     end
-
-    def check_cellar
-      FileUtils.mkdir_p HOMEBREW_CELLAR unless File.exist? HOMEBREW_CELLAR
-    rescue
-      raise <<~EOS
-        Could not create #{HOMEBREW_CELLAR}
-        Check you have permission to write to #{HOMEBREW_CELLAR.parent}
-      EOS
-    end
-
-    def perform_preinstall_checks
-      check_ppc
-      check_writable_install_location
-      check_cellar
-    end
   end
 end

Library/Homebrew/keg.rb

@@ -1,4 +1,5 @@
 require "keg_relocate"
+require "language/python"
 require "lock_file"
 require "ostruct"
 
@@ -64,18 +65,41 @@ class Keg
   # locale-specific directories have the form language[_territory][.codeset][@modifier]
   LOCALEDIR_RX = %r{(locale|man)/([a-z]{2}|C|POSIX)(_[A-Z]{2})?(\.[a-zA-Z\-0-9]+(@.+)?)?}
   INFOFILE_RX = %r{info/([^.].*?\.info|dir)$}
-  TOP_LEVEL_DIRECTORIES = %w[bin etc include lib sbin share var Frameworks].freeze
+  KEG_LINK_DIRECTORIES = %w[
-  ALL_TOP_LEVEL_DIRECTORIES = (TOP_LEVEL_DIRECTORIES + %w[lib/pkgconfig share/locale share/man opt]).freeze
+    bin etc include lib sbin share var Frameworks
-  PRUNEABLE_DIRECTORIES = %w[
+  ].freeze
-    bin etc include lib sbin share opt Frameworks LinkedKegs var/homebrew/linked
+  # TODO: remove when brew-test-bot no longer uses this
-  ].map do |dir|
+  TOP_LEVEL_DIRECTORIES = KEG_LINK_DIRECTORIES
-    case dir
+  PRUNEABLE_DIRECTORIES = (
-    when "LinkedKegs"
+    KEG_LINK_DIRECTORIES - %w[var] + %w[var/homebrew/linked]
-      HOMEBREW_LIBRARY/dir
+  ).map { |dir| HOMEBREW_PREFIX/dir }
-    else
+
-      HOMEBREW_PREFIX/dir
+  # Keep relatively in sync with
-    end
+  # https://github.com/Homebrew/install/blob/master/install
-  end
+  MUST_EXIST_DIRECTORIES = (
+    (KEG_LINK_DIRECTORIES + %w[
+      opt
+    ]).map { |dir| HOMEBREW_PREFIX/dir }
+  ).freeze
+  MUST_BE_WRITABLE_DIRECTORIES = (
+    (KEG_LINK_DIRECTORIES + %w[
+      opt
+      etc/bash_completion.d lib/pkgconfig
+      share/aclocal share/doc share/info share/locale share/man
+      share/man/man1 share/man/man2 share/man/man3 share/man/man4
+      share/man/man5 share/man/man6 share/man/man7 share/man/man8
+      share/zsh share/zsh/site-functions
+      var/log
+      Caskroom
+    ]).map { |dir| HOMEBREW_PREFIX/dir } + [
+      HOMEBREW_CACHE,
+      HOMEBREW_CELLAR,
+      HOMEBREW_LOCKS,
+      HOMEBREW_LOGS,
+      HOMEBREW_REPOSITORY,
+      Language::Python.homebrew_site_packages,
+    ]
+  ).freeze
 
   # These paths relative to the keg's share directory should always be real
   # directories in the prefix, never symlinks.
@@ -291,7 +315,7 @@ class Keg
 
     dirs = []
 
-    TOP_LEVEL_DIRECTORIES.map { |d| path/d }.each do |dir|
+    KEG_LINK_DIRECTORIES.map { |d| path/d }.each do |dir|
       next unless dir.exist?
       dir.find do |src|
         dst = HOMEBREW_PREFIX + src.relative_path_from(path)

Library/Homebrew/lock_file.rb

@@ -5,7 +5,7 @@ class LockFile
 
   def initialize(name)
     @name = name.to_s
-    @path = HOMEBREW_LOCK_DIR/"#{@name}.lock"
+    @path = HOMEBREW_LOCKS/"#{@name}.lock"
     @lockfile = nil
   end
 

Library/Homebrew/test/cleanup_spec.rb

@@ -27,8 +27,8 @@ describe CleanupRefinement do
 end
 
 describe Homebrew::Cleanup do
-  let(:ds_store) { Pathname.new("#{HOMEBREW_PREFIX}/Library/.DS_Store") }
+  let(:ds_store) { Pathname.new("#{HOMEBREW_CELLAR}/.DS_Store") }
-  let(:lock_file) { Pathname.new("#{HOMEBREW_LOCK_DIR}/foo") }
+  let(:lock_file) { Pathname.new("#{HOMEBREW_LOCKS}/foo") }
 
   around do |example|
     begin

Library/Homebrew/test/diagnostic_spec.rb

@@ -29,64 +29,25 @@ describe Homebrew::Diagnostic::Checks do
     end
   end
 
-  specify "#check_access_homebrew_repository" do
+  specify "#check_access_directories" do
     begin
-      mode = HOMEBREW_REPOSITORY.stat.mode & 0777
+      dirs = [
-      HOMEBREW_REPOSITORY.chmod 0555
+        HOMEBREW_CACHE,
-
+        HOMEBREW_CELLAR,
-      expect(subject.check_access_homebrew_repository)
+        HOMEBREW_REPOSITORY,
-        .to match("#{HOMEBREW_REPOSITORY} is not writable.")
+        HOMEBREW_LOGS,
+        HOMEBREW_LOCKS,
+      ]
+      modes = {}
+      dirs.each do |dir|
+        modes[dir] = dir.stat.mode & 0777
+        dir.chmod 0555
+        expect(subject.check_access_directories).to match(dir.to_s)
+      end
     ensure
-      HOMEBREW_REPOSITORY.chmod mode
+      modes.each do |dir, mode|
-    end
+        dir.chmod mode
-  end
+      end
-
-  specify "#check_access_lock_dir" do
-    begin
-      prev_mode = HOMEBREW_LOCK_DIR.stat.mode
-      mode = HOMEBREW_LOCK_DIR.stat.mode & 0777
-      HOMEBREW_LOCK_DIR.chmod 0555
-      expect(HOMEBREW_LOCK_DIR.stat.mode).not_to eq(prev_mode)
-
-      expect(subject.check_access_lock_dir)
-        .to match("#{HOMEBREW_LOCK_DIR} isn't writable.")
-    ensure
-      HOMEBREW_LOCK_DIR.chmod mode
-    end
-  end
-
-  specify "#check_access_logs" do
-    begin
-      mode = HOMEBREW_LOGS.stat.mode & 0777
-      HOMEBREW_LOGS.chmod 0555
-
-      expect(subject.check_access_logs)
-        .to match("#{HOMEBREW_LOGS} isn't writable.")
-    ensure
-      HOMEBREW_LOGS.chmod mode
-    end
-  end
-
-  specify "#check_access_cache" do
-    begin
-      mode = HOMEBREW_CACHE.stat.mode & 0777
-      HOMEBREW_CACHE.chmod 0555
-      expect(subject.check_access_cache)
-        .to match("#{HOMEBREW_CACHE} isn't writable.")
-    ensure
-      HOMEBREW_CACHE.chmod mode
-    end
-  end
-
-  specify "#check_access_cellar" do
-    begin
-      mode = HOMEBREW_CELLAR.stat.mode & 0777
-      HOMEBREW_CELLAR.chmod 0555
-
-      expect(subject.check_access_cellar)
-        .to match("#{HOMEBREW_CELLAR} isn't writable.")
-    ensure
-      HOMEBREW_CELLAR.chmod mode
     end
   end
 

Library/Homebrew/test/spec_helper.rb

@@ -32,7 +32,7 @@ TEST_DIRECTORIES = [
   HOMEBREW_CACHE,
   HOMEBREW_CACHE_FORMULA,
   HOMEBREW_CELLAR,
-  HOMEBREW_LOCK_DIR,
+  HOMEBREW_LOCKS,
   HOMEBREW_LOGS,
   HOMEBREW_TEMP,
 ].freeze
@@ -134,13 +134,9 @@ RSpec.configure do |config|
 
       FileUtils.rm_rf [
         TEST_DIRECTORIES.map(&:children),
+        *Keg::MUST_EXIST_DIRECTORIES,
         HOMEBREW_LINKED_KEGS,
         HOMEBREW_PINNED_KEGS,
-        HOMEBREW_PREFIX/".git",
-        HOMEBREW_PREFIX/"bin",
-        HOMEBREW_PREFIX/"etc",
-        HOMEBREW_PREFIX/"share",
-        HOMEBREW_PREFIX/"opt",
         HOMEBREW_PREFIX/"Caskroom",
         HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-cask",
         HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bar",

Library/Homebrew/test/support/lib/config.rb

@@ -27,7 +27,7 @@ HOMEBREW_CACHE         = HOMEBREW_PREFIX.parent/"cache"
 HOMEBREW_CACHE_FORMULA = HOMEBREW_PREFIX.parent/"formula_cache"
 HOMEBREW_LINKED_KEGS   = HOMEBREW_PREFIX.parent/"linked"
 HOMEBREW_PINNED_KEGS   = HOMEBREW_PREFIX.parent/"pinned"
-HOMEBREW_LOCK_DIR      = HOMEBREW_PREFIX.parent/"locks"
+HOMEBREW_LOCKS         = HOMEBREW_PREFIX.parent/"locks"
 HOMEBREW_CELLAR        = HOMEBREW_PREFIX.parent/"cellar"
 HOMEBREW_LOGS          = HOMEBREW_PREFIX.parent/"logs"
 HOMEBREW_TEMP          = HOMEBREW_PREFIX.parent/"temp"

Library/Homebrew/update_migrator.rb

@@ -341,9 +341,7 @@ module UpdateMigrator
         EOS
       end
 
-      (Keg::ALL_TOP_LEVEL_DIRECTORIES + ["Cellar"]).each do |dir|
+      Keg::MUST_EXIST_DIRECTORIES.each { |dir| FileUtils.mkdir_p dir }
-        FileUtils.mkdir_p "#{HOMEBREW_PREFIX}/#{dir}"
-      end
 
       src = Pathname.new("#{new_homebrew_repository}/bin/brew")
       dst = Pathname.new("#{HOMEBREW_PREFIX}/bin/brew")

docs/Troubleshooting.md

@@ -11,7 +11,7 @@ Follow these steps to fix common problems:
 * Run `brew update` twice.
 * Run `brew doctor` and fix all the warnings (**outdated Xcode/CLT and unbrewed dylibs are very likely to cause problems**).
 * Check that **Command Line Tools for Xcode (CLT)** and **Xcode** are up to date.
-* If commands fail with permissions errors, check the permissions of `/usr/local`'s subdirectories. If you’re unsure what to do, you can run `cd /usr/local && sudo chown -R $(whoami) bin etc include lib sbin share var Frameworks`.
+* If commands fail with permissions errors, check the permissions of `/usr/local`'s subdirectories. If you’re unsure what to do, you can run `cd /usr/local && sudo chown -R $(whoami) bin etc include lib sbin share var opt Cellar Caskroom Frameworks`.
 * Read through the [Common Issues](Common-Issues.md).
 
 ## Check to see if the issue has been reported