Merge pull request #2560 from reitermarkus/PATH-refactoring

Refactor PATH generation.

Library/Homebrew/PATH.rb

@@ -0,0 +1,74 @@
+class PATH
+  include Enumerable
+  extend Forwardable
+
+  def_delegator :@paths, :each
+
+  def initialize(*paths)
+    @paths = parse(*paths)
+  end
+
+  def prepend(*paths)
+    @paths = parse(*paths, *@paths)
+    self
+  end
+
+  def append(*paths)
+    @paths = parse(*@paths, *paths)
+    self
+  end
+
+  def insert(index, *paths)
+    @paths = parse(*@paths.insert(index, *paths))
+    self
+  end
+
+  def select(&block)
+    self.class.new(@paths.select(&block))
+  end
+
+  def reject(&block)
+    self.class.new(@paths.reject(&block))
+  end
+
+  def to_ary
+    @paths
+  end
+  alias to_a to_ary
+
+  def to_str
+    @paths.join(File::PATH_SEPARATOR)
+  end
+  alias to_s to_str
+
+  def ==(other)
+    if other.respond_to?(:to_ary)
+      return true if to_ary == other.to_ary
+    end
+
+    if other.respond_to?(:to_str)
+      return true if to_str == other.to_str
+    end
+
+    false
+  end
+
+  def empty?
+    @paths.empty?
+  end
+
+  def existing
+    existing_path = select(&File.method(:directory?))
+    # return nil instead of empty PATH, to unset environment variables
+    existing_path unless existing_path.empty?
+  end
+
+  private
+
+  def parse(*paths)
+    paths.flatten
+         .compact
+         .flat_map { |p| Pathname.new(p).to_path.split(File::PATH_SEPARATOR) }
+         .uniq
+  end
+end

Library/Homebrew/brew.rb

@@ -12,9 +12,9 @@ require "pathname"
 HOMEBREW_LIBRARY_PATH = Pathname.new(__FILE__).realpath.parent
 $:.unshift(HOMEBREW_LIBRARY_PATH.to_s)
 require "global"
+require "tap"
 
 if ARGV == %w[--version] || ARGV == %w[-v]
-  require "tap"
   puts "Homebrew #{HOMEBREW_VERSION}"
   puts "Homebrew/homebrew-core #{CoreTap.instance.version_string}"
   exit 0
@@ -47,13 +47,15 @@ begin
     end
   end
 
+  path = PATH.new(ENV["PATH"])
+
   # Add contributed commands to PATH before checking.
-  Dir["#{HOMEBREW_LIBRARY}/Taps/*/*/cmd"].each do |tap_cmd_dir|
+  path.append(Pathname.glob(Tap::TAP_DIRECTORY/"*/*/cmd"))
-    ENV["PATH"] += "#{File::PATH_SEPARATOR}#{tap_cmd_dir}"
-  end
 
   # Add SCM wrappers.
-  ENV["PATH"] += "#{File::PATH_SEPARATOR}#{HOMEBREW_SHIMS_PATH}/scm"
+  path.append(HOMEBREW_SHIMS_PATH/"scm")
+
+  ENV["PATH"] = path
 
   if cmd
     internal_cmd = require? HOMEBREW_LIBRARY_PATH.join("cmd", cmd)

Library/Homebrew/cmd/sh.rb

@@ -23,7 +23,7 @@ module Homebrew
     ENV.setup_build_environment
     if superenv?
       # superenv stopped adding brew's bin but generally users will want it
-      ENV["PATH"] = ENV["PATH"].split(File::PATH_SEPARATOR).insert(1, "#{HOMEBREW_PREFIX}/bin").join(File::PATH_SEPARATOR)
+      ENV["PATH"] = PATH.new(ENV["PATH"]).insert(1, HOMEBREW_PREFIX/"bin")
     end
     ENV["PS1"] = 'brew \[\033[1;32m\]\w\[\033[0m\]$ '
     ENV["VERBOSE"] = "1"

Library/Homebrew/diagnostic.rb

@@ -100,8 +100,7 @@ module Homebrew
 
       # See https://github.com/Homebrew/legacy-homebrew/pull/9986
       def check_path_for_trailing_slashes
-        all_paths = ENV["PATH"].split(File::PATH_SEPARATOR)
+        bad_paths = PATH.new(ENV["PATH"]).select { |p| p.end_with?("/") }
-        bad_paths = all_paths.select { |p| p[-1..-1] == "/" }
         return if bad_paths.empty?
 
         inject_file_list bad_paths, <<-EOS.undent

Library/Homebrew/extend/ENV/shared.rb

@@ -1,6 +1,7 @@
 require "formula"
 require "compilers"
 require "development_tools"
+require "PATH"
 
 # Homebrew extends Ruby's `ENV` to make our code more readable.
 # Implemented in {SharedEnvExtension} and either {Superenv} or
@@ -80,7 +81,7 @@ module SharedEnvExtension
   end
 
   def append_path(key, path)
-    append key, path, File::PATH_SEPARATOR if File.directory? path
+    self[key] = PATH.new(self[key]).append(path)
   end
 
   # Prepends a directory to `PATH`.
@@ -92,7 +93,7 @@ module SharedEnvExtension
   # (e.g. <pre>ENV.prepend_path "PATH", which("emacs").dirname</pre>)
   def prepend_path(key, path)
     return if %w[/usr/bin /bin /usr/sbin /sbin].include? path.to_s
-    prepend key, path, File::PATH_SEPARATOR if File.directory? path
+    self[key] = PATH.new(self[key]).prepend(path)
   end
 
   def prepend_create_path(key, path)
@@ -196,22 +197,23 @@ module SharedEnvExtension
 
   # @private
   def userpaths!
-    paths = self["PATH"].split(File::PATH_SEPARATOR)
+    path = PATH.new(self["PATH"]).select do |p|
       # put Superenv.bin and opt path at the first
-    new_paths = paths.select { |p| p.start_with?("#{HOMEBREW_REPOSITORY}/Library/ENV", "#{HOMEBREW_PREFIX}/opt") }
+      p.start_with?("#{HOMEBREW_REPOSITORY}/Library/ENV", "#{HOMEBREW_PREFIX}/opt")
-    # XXX hot fix to prefer brewed stuff (e.g. python) over /usr/bin.
+    end
-    new_paths << "#{HOMEBREW_PREFIX}/bin"
+    path.append(HOMEBREW_PREFIX/"bin") # XXX hot fix to prefer brewed stuff (e.g. python) over /usr/bin.
-    # reset of self["PATH"]
+    path.append(self["PATH"]) # reset of self["PATH"]
-    new_paths += paths
+    path.append(
       # user paths
-    new_paths += ORIGINAL_PATHS.map do |p|
+      ORIGINAL_PATHS.map do |p|
         begin
           p.realpath.to_s
         rescue
           nil
         end
-    end - %w[/usr/X11/bin /opt/X11/bin]
+      end - %w[/usr/X11/bin /opt/X11/bin],
-    self["PATH"] = new_paths.uniq.join(File::PATH_SEPARATOR)
+    )
+    self["PATH"] = path
   end
 
   def fortran
@@ -244,7 +246,7 @@ module SharedEnvExtension
     else
       if (gfortran = which("gfortran", (HOMEBREW_PREFIX/"bin").to_s))
         ohai "Using Homebrew-provided fortran compiler."
-      elsif (gfortran = which("gfortran", ORIGINAL_PATHS.join(File::PATH_SEPARATOR)))
+      elsif (gfortran = which("gfortran", PATH.new(ORIGINAL_PATHS)))
         ohai "Using a fortran compiler found at #{gfortran}."
       end
       if gfortran

Library/Homebrew/extend/ENV/std.rb

@@ -57,12 +57,12 @@ module Stdenv
   end
 
   def determine_pkg_config_libdir
-    paths = []
+    PATH.new(
-    paths << "#{HOMEBREW_PREFIX}/lib/pkgconfig"
+      HOMEBREW_PREFIX/"lib/pkgconfig",
-    paths << "#{HOMEBREW_PREFIX}/share/pkgconfig"
+      HOMEBREW_PREFIX/"share/pkgconfig",
-    paths += homebrew_extra_pkg_config_paths
+      homebrew_extra_pkg_config_paths,
-    paths << "/usr/lib/pkgconfig"
+      "/usr/lib/pkgconfig",
-    paths.select { |d| File.directory? d }.join(File::PATH_SEPARATOR)
+    ).existing
   end
 
   # Removes the MAKEFLAGS environment variable, causing make to use a single job.

Library/Homebrew/extend/ENV/super.rb

@@ -101,29 +101,28 @@ module Superenv
   end
 
   def determine_path
-    paths = [Superenv.bin]
+    path = PATH.new(Superenv.bin)
 
     # Formula dependencies can override standard tools.
-    paths += deps.map { |d| d.opt_bin.to_s }
+    path.append(deps.map(&:opt_bin))
-
+    path.append(homebrew_extra_paths)
-    paths += homebrew_extra_paths
+    path.append("/usr/bin", "/bin", "/usr/sbin", "/sbin")
-    paths += %w[/usr/bin /bin /usr/sbin /sbin]
 
     # Homebrew's apple-gcc42 will be outside the PATH in superenv,
     # so xcrun may not be able to find it
     begin
       case homebrew_cc
       when "gcc-4.2"
-        paths << Formulary.factory("apple-gcc42").opt_bin
+        path.append(Formulary.factory("apple-gcc42").opt_bin)
       when GNU_GCC_REGEXP
-        paths << gcc_version_formula($&).opt_bin
+        path.append(gcc_version_formula($&).opt_bin)
       end
     rescue FormulaUnavailableError
       # Don't fail and don't add these formulae to the path if they don't exist.
       nil
     end
 
-    paths.to_path_s
+    path.existing
   end
 
   def homebrew_extra_pkg_config_paths
@@ -131,15 +130,17 @@ module Superenv
   end
 
   def determine_pkg_config_path
-    paths  = deps.map { |d| "#{d.opt_lib}/pkgconfig" }
+    PATH.new(
-    paths += deps.map { |d| "#{d.opt_share}/pkgconfig" }
+      deps.map { |d| d.opt_lib/"pkgconfig" },
-    paths.to_path_s
+      deps.map { |d| d.opt_share/"pkgconfig" },
+    ).existing
   end
 
   def determine_pkg_config_libdir
-    paths = %w[/usr/lib/pkgconfig]
+    PATH.new(
-    paths += homebrew_extra_pkg_config_paths
+      "/usr/lib/pkgconfig",
-    paths.to_path_s
+      homebrew_extra_pkg_config_paths,
+    ).existing
   end
 
   def homebrew_extra_aclocal_paths
@@ -147,10 +148,11 @@ module Superenv
   end
 
   def determine_aclocal_path
-    paths = keg_only_deps.map { |d| "#{d.opt_share}/aclocal" }
+    PATH.new(
-    paths << "#{HOMEBREW_PREFIX}/share/aclocal"
+      keg_only_deps.map { |d| d.opt_share/"aclocal" },
-    paths += homebrew_extra_aclocal_paths
+      HOMEBREW_PREFIX/"share/aclocal",
-    paths.to_path_s
+      homebrew_extra_aclocal_paths,
+    ).existing
   end
 
   def homebrew_extra_isystem_paths
@@ -158,13 +160,14 @@ module Superenv
   end
 
   def determine_isystem_paths
-    paths = ["#{HOMEBREW_PREFIX}/include"]
+    PATH.new(
-    paths += homebrew_extra_isystem_paths
+      HOMEBREW_PREFIX/"include",
-    paths.to_path_s
+      homebrew_extra_isystem_paths,
+    ).existing
   end
 
   def determine_include_paths
-    keg_only_deps.map { |d| d.opt_include.to_s }.to_path_s
+    PATH.new(keg_only_deps.map(&:opt_include)).existing
   end
 
   def homebrew_extra_library_paths
@@ -172,10 +175,11 @@ module Superenv
   end
 
   def determine_library_paths
-    paths = keg_only_deps.map { |d| d.opt_lib.to_s }
+    PATH.new(
-    paths << "#{HOMEBREW_PREFIX}/lib"
+      keg_only_deps.map(&:opt_lib),
-    paths += homebrew_extra_library_paths
+      HOMEBREW_PREFIX/"lib",
-    paths.to_path_s
+      homebrew_extra_library_paths,
+    ).existing
   end
 
   def determine_dependencies
@@ -183,9 +187,10 @@ module Superenv
   end
 
   def determine_cmake_prefix_path
-    paths = keg_only_deps.map { |d| d.opt_prefix.to_s }
+    PATH.new(
-    paths << HOMEBREW_PREFIX.to_s
+      keg_only_deps.map(&:opt_prefix),
-    paths.to_path_s
+      HOMEBREW_PREFIX.to_s,
+    ).existing
   end
 
   def homebrew_extra_cmake_include_paths
@@ -193,9 +198,7 @@ module Superenv
   end
 
   def determine_cmake_include_path
-    paths = []
+    PATH.new(homebrew_extra_cmake_include_paths).existing
-    paths += homebrew_extra_cmake_include_paths
-    paths.to_path_s
   end
 
   def homebrew_extra_cmake_library_paths
@@ -203,9 +206,7 @@ module Superenv
   end
 
   def determine_cmake_library_path
-    paths = []
+    PATH.new(homebrew_extra_cmake_library_paths).existing
-    paths += homebrew_extra_cmake_library_paths
-    paths.to_path_s
   end
 
   def homebrew_extra_cmake_frameworks_paths
@@ -213,9 +214,10 @@ module Superenv
   end
 
   def determine_cmake_frameworks_path
-    paths = deps.map { |d| d.opt_frameworks.to_s }
+    PATH.new(
-    paths += homebrew_extra_cmake_frameworks_paths
+      deps.map(&:opt_frameworks),
-    paths.to_path_s
+      homebrew_extra_cmake_frameworks_paths,
+    ).existing
   end
 
   def determine_make_jobs
@@ -330,10 +332,4 @@ module Superenv
   end
 end
 
-class Array
-  def to_path_s
-    map(&:to_s).uniq.select { |s| File.directory? s }.join(File::PATH_SEPARATOR).chuzzle
-  end
-end
-
 require "extend/os/extend/ENV/super"

Library/Homebrew/global.rb

@@ -3,6 +3,7 @@ require "extend/fileutils"
 require "extend/pathname"
 require "extend/git_repository"
 require "extend/ARGV"
+require "PATH"
 require "extend/string"
 require "os"
 require "utils"
@@ -55,7 +56,7 @@ HOMEBREW_PULL_OR_COMMIT_URL_REGEX = %r[https://github\.com/([\w-]+)/([\w-]+)?/(?
 require "compat" unless ARGV.include?("--no-compat") || ENV["HOMEBREW_NO_COMPAT"]
 
 ENV["HOMEBREW_PATH"] ||= ENV["PATH"]
-ORIGINAL_PATHS = ENV["HOMEBREW_PATH"].split(File::PATH_SEPARATOR).map do |p|
+ORIGINAL_PATHS = PATH.new(ENV["HOMEBREW_PATH"]).map do |p|
   begin
     Pathname.new(p).expand_path
   rescue

Library/Homebrew/requirement.rb

@@ -96,7 +96,7 @@ class Requirement
     # PATH.
     parent = satisfied_result_parent
     return unless parent
-    return if ENV["PATH"].split(File::PATH_SEPARATOR).include?(parent.to_s)
+    return if PATH.new(ENV["PATH"]).include?(parent.to_s)
     ENV.append_path("PATH", parent)
   end
 
@@ -151,11 +151,11 @@ class Requirement
   end
 
   def which(cmd)
-    super(cmd, ORIGINAL_PATHS.join(File::PATH_SEPARATOR))
+    super(cmd, PATH.new(ORIGINAL_PATHS))
   end
 
   def which_all(cmd)
-    super(cmd, ORIGINAL_PATHS.join(File::PATH_SEPARATOR))
+    super(cmd, PATH.new(ORIGINAL_PATHS))
   end
 
   class << self

Library/Homebrew/test/PATH_spec.rb

@@ -0,0 +1,115 @@
+require "PATH"
+
+describe PATH do
+  describe "#initialize" do
+    it "can take multiple arguments" do
+      expect(described_class.new("/path1", "/path2")).to eq("/path1:/path2")
+    end
+
+    it "can parse a mix of arrays and arguments" do
+      expect(described_class.new(["/path1", "/path2"], "/path3")).to eq("/path1:/path2:/path3")
+    end
+
+    it "splits an existing PATH" do
+      expect(described_class.new("/path1:/path2")).to eq(["/path1", "/path2"])
+    end
+
+    it "removes duplicates" do
+      expect(described_class.new("/path1", "/path1")).to eq("/path1")
+    end
+  end
+
+  describe "#to_ary" do
+    it "returns a PATH array" do
+      expect(described_class.new("/path1", "/path2").to_ary).to eq(["/path1", "/path2"])
+    end
+  end
+
+  describe "#to_str" do
+    it "returns a PATH string" do
+      expect(described_class.new("/path1", "/path2").to_str).to eq("/path1:/path2")
+    end
+  end
+
+  describe "#prepend" do
+    it "prepends a path to a PATH" do
+      expect(described_class.new("/path1").prepend("/path2").to_str).to eq("/path2:/path1")
+    end
+
+    it "removes duplicates" do
+      expect(described_class.new("/path1").prepend("/path1").to_str).to eq("/path1")
+    end
+  end
+
+  describe "#append" do
+    it "prepends a path to a PATH" do
+      expect(described_class.new("/path1").append("/path2").to_str).to eq("/path1:/path2")
+    end
+
+    it "removes duplicates" do
+      expect(described_class.new("/path1").append("/path1").to_str).to eq("/path1")
+    end
+  end
+
+  describe "#insert" do
+    it "inserts a path at a given index" do
+      expect(described_class.new("/path1").insert(0, "/path2").to_str).to eq("/path2:/path1")
+    end
+
+    it "can insert multiple paths" do
+      expect(described_class.new("/path1").insert(0, "/path2", "/path3")).to eq("/path2:/path3:/path1")
+    end
+  end
+
+  describe "#include?" do
+    it "returns true if a path is included" do
+      path = described_class.new("/path1", "/path2")
+      expect(path).to include("/path1")
+      expect(path).to include("/path2")
+    end
+
+    it "returns false if a path is not included" do
+      expect(described_class.new("/path1")).not_to include("/path2")
+    end
+
+    it "returns false if the given string contains a separator" do
+      expect(described_class.new("/path1", "/path2")).not_to include("/path1:")
+    end
+  end
+
+  describe "#each" do
+    it "loops through each path" do
+      enum = described_class.new("/path1", "/path2").each
+
+      expect(enum.next).to eq("/path1")
+      expect(enum.next).to eq("/path2")
+    end
+  end
+
+  describe "#select" do
+    it "returns an object of the same class instead of an Array" do
+      expect(described_class.new.select { true }).to be_a(described_class)
+    end
+  end
+
+  describe "#reject" do
+    it "returns an object of the same class instead of an Array" do
+      expect(described_class.new.reject { true }).to be_a(described_class)
+    end
+  end
+
+  describe "#existing" do
+    it "returns a new PATH without non-existent paths" do
+      allow(File).to receive(:directory?).with("/path1").and_return(true)
+      allow(File).to receive(:directory?).with("/path2").and_return(false)
+
+      path = described_class.new("/path1", "/path2")
+      expect(path.existing.to_ary).to eq(["/path1"])
+      expect(path.to_ary).to eq(["/path1", "/path2"])
+    end
+
+    it "returns nil instead of an empty #{described_class}" do
+      expect(described_class.new.existing).to be nil
+    end
+  end
+end

Library/Homebrew/utils.rb

@@ -190,10 +190,10 @@ module Homebrew
     Gem::Specification.reset
 
     # Add Gem binary directory and (if missing) Ruby binary directory to PATH.
-    path = ENV["PATH"].split(File::PATH_SEPARATOR)
+    path = PATH.new(ENV["PATH"])
-    path.unshift(RUBY_BIN) if which("ruby") != RUBY_PATH
+    path.prepend(RUBY_BIN) if which("ruby") != RUBY_PATH
-    path.unshift(Gem.bindir)
+    path.prepend(Gem.bindir)
-    ENV["PATH"] = path.join(File::PATH_SEPARATOR)
+    ENV["PATH"] = path.existing
 
     if Gem::Specification.find_all_by_name(name, version).empty?
       ohai "Installing or updating '#{name}' gem"
@@ -293,7 +293,7 @@ def quiet_system(cmd, *args)
 end
 
 def which(cmd, path = ENV["PATH"])
-  path.split(File::PATH_SEPARATOR).each do |p|
+  PATH.new(path).each do |p|
     begin
       pcmd = File.expand_path(cmd, p)
     rescue ArgumentError
@@ -307,7 +307,7 @@ def which(cmd, path = ENV["PATH"])
 end
 
 def which_all(cmd, path = ENV["PATH"])
-  path.to_s.split(File::PATH_SEPARATOR).map do |p|
+  PATH.new(path).map do |p|
     begin
       pcmd = File.expand_path(cmd, p)
     rescue ArgumentError
@@ -416,7 +416,7 @@ def nostdout
 end
 
 def paths(env_path = ENV["PATH"])
-  @paths ||= env_path.split(File::PATH_SEPARATOR).collect do |p|
+  @paths ||= PATH.new(env_path).collect do |p|
     begin
       File.expand_path(p).chomp("/")
     rescue ArgumentError