Merge pull request #3933 from rubygems/path_and_path_system_priority

Prioritize `path.system` over `path` when it makes sense

bundler/lib/bundler/settings.rb

@@ -63,30 +63,25 @@ class Settings
     ].freeze
 
     DEFAULT_CONFIG = {
-      :silence_deprecations => false,
-      :disable_version_check => true,
-      :prefer_patch => false,
-      :redirect => 5,
-      :retry => 3,
-      :timeout => 10,
+      "BUNDLE_SILENCE_DEPRECATIONS" => false,
+      "BUNDLE_DISABLE_VERSION_CHECK" => true,
+      "BUNDLE_PREFER_PATCH" => false,
+      "BUNDLE_REDIRECT" => 5,
+      "BUNDLE_RETRY" => 3,
+      "BUNDLE_TIMEOUT" => 10,
     }.freeze
 
     def initialize(root = nil)
       @root            = root
       @local_config    = load_config(local_config_file)
+      @env_config      = ENV.to_h.select {|key, _value| key =~ /\ABUNDLE_.+/ }
       @global_config   = load_config(global_config_file)
       @temporary       = {}
     end
 
     def [](name)
       key = key_for(name)
-      value = @temporary.fetch(key) do
-              @local_config.fetch(key) do
-              ENV.fetch(key) do
-              @global_config.fetch(key) do
-              DEFAULT_CONFIG.fetch(name) do
-                nil
-              end end end end end
+      value = configs.values.map {|config| config[key] }.compact.first
 
       converted_value(value, name)
     end
@@ -129,9 +124,7 @@ def set_global(key, value)
     end
 
     def all
-      env_keys = ENV.keys.grep(/\ABUNDLE_.+/)
-
-      keys = @temporary.keys | @global_config.keys | @local_config.keys | env_keys
+      keys = @temporary.keys | @global_config.keys | @local_config.keys | @env_config.keys
 
       keys.map do |key|
         key.sub(/^BUNDLE_/, "").gsub(/__/, ".").downcase
@@ -168,34 +161,32 @@ def gem_mirrors
 
     def locations(key)
       key = key_for(key)
-      locations = {}
-      locations[:temporary] = @temporary[key] if @temporary.key?(key)
-      locations[:local]  = @local_config[key] if @local_config.key?(key)
-      locations[:env]    = ENV[key] if ENV[key]
-      locations[:global] = @global_config[key] if @global_config.key?(key)
-      locations[:default] = DEFAULT_CONFIG[key] if DEFAULT_CONFIG.key?(key)
-      locations
+      configs.keys.inject({}) do |partial_locations, level|
+        value_on_level = configs[level][key]
+        partial_locations[level] = value_on_level unless value_on_level.nil?
+        partial_locations
+      end
     end
 
     def pretty_values_for(exposed_key)
       key = key_for(exposed_key)
 
       locations = []
 
-      if @temporary.key?(key)
-        locations << "Set for the current command: #{converted_value(@temporary[key], exposed_key).inspect}"
+      if value = @temporary[key]
+        locations << "Set for the current command: #{converted_value(value, exposed_key).inspect}"
       end
 
-      if @local_config.key?(key)
-        locations << "Set for your local app (#{local_config_file}): #{converted_value(@local_config[key], exposed_key).inspect}"
+      if value = @local_config[key]
+        locations << "Set for your local app (#{local_config_file}): #{converted_value(value, exposed_key).inspect}"
       end
 
-      if value = ENV[key]
+      if value = @env_config[key]
         locations << "Set via #{key}: #{converted_value(value, exposed_key).inspect}"
       end
 
-      if @global_config.key?(key)
-        locations << "Set for the current user (#{global_config_file}): #{converted_value(@global_config[key], exposed_key).inspect}"
+      if value = @global_config[key]
+        locations << "Set for the current user (#{global_config_file}): #{converted_value(value, exposed_key).inspect}"
       end
 
       return ["You have not configured a value for `#{exposed_key}`"] if locations.empty?
@@ -204,17 +195,19 @@ def pretty_values_for(exposed_key)
 
     # for legacy reasons, in Bundler 2, we do not respect :disable_shared_gems
     def path
-      key  = key_for(:path)
-      path = ENV[key] || @global_config[key]
-      if path && !@temporary.key?(key) && !@local_config.key?(key)
-        return Path.new(path, false, false)
+      configs.each do |_level, settings|
+        path = value_for("path", settings)
+        path_system = value_for("path.system", settings)
+        disabled_shared_gems = value_for("disable_shared_gems", settings)
+        next if path.nil? && path_system.nil? && disabled_shared_gems.nil?
+        system_path = path_system || (disabled_shared_gems == false)
+        return Path.new(path, system_path)
       end
 
-      system_path = self["path.system"] || (self[:disable_shared_gems] == false)
-      Path.new(self[:path], system_path, Bundler.feature_flag.default_install_uses_path?)
+      Path.new(nil, false)
     end
 
-    Path = Struct.new(:explicit_path, :system_path, :default_install_uses_path) do
+    Path = Struct.new(:explicit_path, :system_path) do
       def path
         path = base_path
         path = File.join(path, Bundler.ruby_scope) unless use_system_gems?
@@ -224,7 +217,7 @@ def path
       def use_system_gems?
         return true if system_path
         return false if explicit_path
-        !default_install_uses_path
+        !Bundler.feature_flag.default_install_uses_path?
       end
 
       def base_path
@@ -277,9 +270,9 @@ def app_cache_path
 
     def validate!
       all.each do |raw_key|
-        [@local_config, ENV, @global_config].each do |settings|
-          value = converted_value(settings[key_for(raw_key)], raw_key)
-          Validator.validate!(raw_key, value, settings.to_hash.dup)
+        [@local_config, @env_config, @global_config].each do |settings|
+          value = value_for(raw_key, settings)
+          Validator.validate!(raw_key, value, settings.dup)
         end
       end
     end
@@ -292,6 +285,20 @@ def key_for(key)
 
     private
 
+    def configs
+      {
+        :temporary => @temporary,
+        :local => @local_config,
+        :env => @env_config,
+        :global => @global_config,
+        :default => DEFAULT_CONFIG,
+      }
+    end
+
+    def value_for(name, config)
+      converted_value(config[key_for(name)], name)
+    end
+
     def parent_setting_for(name)
       split_specific_setting_for(name)[0]
     end

bundler/spec/install/path_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 RSpec.describe "bundle install" do
-  describe "with --path" do
+  describe "with path configured" do
     before :each do
       build_gem "rack", "1.0.0", :to_system => true do |s|
         s.write "lib/rack.rb", "puts 'FAIL'"
@@ -13,12 +13,20 @@
       G
     end
 
-    it "does not use available system gems with bundle --path vendor/bundle", :bundler => "< 3" do
+    it "does not use available system gems with `vendor/bundle" do
       bundle "config --local path vendor/bundle"
       bundle :install
       expect(the_bundle).to include_gems "rack 1.0.0"
     end
 
+    it "uses system gems with `path.system` configured with more priority than `path`" do
+      bundle "config --local path.system true"
+      bundle "config --global path vendor/bundle"
+      bundle :install
+      run "require 'rack'", :raise_on_error => false
+      expect(out).to include("FAIL")
+    end
+
     it "handles paths with regex characters in them" do
       dir = bundled_app("bun++dle")
       dir.mkpath
@@ -30,7 +38,7 @@
       dir.rmtree
     end
 
-    it "prints a warning to let the user know what has happened with bundle --path vendor/bundle" do
+    it "prints a message to let the user know where gems where installed" do
       bundle "config --local path vendor/bundle"
       bundle :install
       expect(out).to include("gems are installed into `./vendor/bundle`")