From ad6843972fd53cc140f39abb800f9a64ff272994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 25 Oct 2022 15:21:26 +0200 Subject: [PATCH] Add a warning in an edge case of using `gemspec` DSL If a Gemfile duplicates a development dependency also defined in a local gemspec with a different requirement, the requirement in the local gemspec will be silently ignored. This surprised me. I think we should either: * Make sure both requirements are considered, like it happens for runtime dependencies (I added a spec to illustrate the current behavior here). * Add a warning that the requirement in the gemspec will be ignored. I think the former is slightly preferable, but it may cause some bundle's that previously resolve to no longer resolver. I went with the latter but the more I think about it, the more this seems like it should behave like the former. --- bundler/lib/bundler/dsl.rb | 41 +++++++++++--------- bundler/spec/commands/install_spec.rb | 54 +++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 18 deletions(-) diff --git a/bundler/lib/bundler/dsl.rb b/bundler/lib/bundler/dsl.rb index 03c80a408cc4..76e2f73fff59 100644 --- a/bundler/lib/bundler/dsl.rb +++ b/bundler/lib/bundler/dsl.rb @@ -102,38 +102,43 @@ def gem(name, *args) # if there's already a dependency with this name we try to prefer one if current = @dependencies.find {|d| d.name == dep.name } - deleted_dep = @dependencies.delete(current) if current.type == :development + if current.requirement != dep.requirement + current_requirement_open = current.requirements_list.include?(">= 0") - unless deleted_dep - if current.requirement != dep.requirement - return if dep.type == :development + if current.type == :development + @dependencies.delete(current) + unless current_requirement_open || dep.type == :development + Bundler.ui.warn "A gemspec development dependency (#{dep.name}, #{current.requirement}) is being overridden by a Gemfile dependency (#{dep.name}, #{dep.requirement}).\n" \ + "This behaviour may change in the future. Please remove either of them, or make sure they both have the same requirement\n" \ + end + else update_prompt = "" if File.basename(@gemfile) == Injector::INJECTED_GEMS - if dep.requirements_list.include?(">= 0") && !current.requirements_list.include?(">= 0") + if dep.requirements_list.include?(">= 0") && !current_requirement_open update_prompt = ". Gem already added" else update_prompt = ". If you want to update the gem version, run `bundle update #{current.name}`" - update_prompt += ". You may also need to change the version requirement specified in the Gemfile if it's too restrictive." unless current.requirements_list.include?(">= 0") + update_prompt += ". You may also need to change the version requirement specified in the Gemfile if it's too restrictive." unless current_requirement_open end end raise GemfileError, "You cannot specify the same gem twice with different version requirements.\n" \ - "You specified: #{current.name} (#{current.requirement}) and #{dep.name} (#{dep.requirement})" \ - "#{update_prompt}" - elsif current.source != dep.source - return if dep.type == :development - raise GemfileError, "You cannot specify the same gem twice coming from different sources.\n" \ - "You specified that #{dep.name} (#{dep.requirement}) should come from " \ - "#{current.source || "an unspecified source"} and #{dep.source}\n" - else - Bundler.ui.warn "Your Gemfile lists the gem #{current.name} (#{current.requirement}) more than once.\n" \ - "You should probably keep only one of them.\n" \ - "Remove any duplicate entries and specify the gem only once.\n" \ - "While it's not a problem now, it could cause errors if you change the version of one of them later." + "You specified: #{current.name} (#{current.requirement}) and #{dep.name} (#{dep.requirement})" \ + "#{update_prompt}" end + elsif current.source != dep.source + return if dep.type == :development + raise GemfileError, "You cannot specify the same gem twice coming from different sources.\n" \ + "You specified that #{dep.name} (#{dep.requirement}) should come from " \ + "#{current.source || "an unspecified source"} and #{dep.source}\n" + elsif current.type != :development && dep.type != :development + Bundler.ui.warn "Your Gemfile lists the gem #{current.name} (#{current.requirement}) more than once.\n" \ + "You should probably keep only one of them.\n" \ + "Remove any duplicate entries and specify the gem only once.\n" \ + "While it's not a problem now, it could cause errors if you change the version of one of them later." end end diff --git a/bundler/spec/commands/install_spec.rb b/bundler/spec/commands/install_spec.rb index c2f55befc395..4456af257258 100644 --- a/bundler/spec/commands/install_spec.rb +++ b/bundler/spec/commands/install_spec.rb @@ -430,6 +430,60 @@ expect(the_bundle).to include_gems("my-private-gem 1.0") end + it "throws a warning if a gem is added once in Gemfile and also inside a gemspec as a development dependency, with different requirements" do + build_lib "my-gem", :path => bundled_app do |s| + s.add_development_dependency "rubocop", "~> 1.36.0" + end + + build_repo4 do + build_gem "rubocop", "1.36.0" + build_gem "rubocop", "1.37.1" + end + + gemfile <<~G + source "#{file_uri_for(gem_repo4)}" + + gemspec + + gem "rubocop", group: :development + G + + bundle :install + + expect(err).to include("A gemspec development dependency (rubocop, ~> 1.36.0) is being overridden by a Gemfile dependency (rubocop, >= 0).") + expect(err).to include("This behaviour may change in the future. Please remove either of them, or make sure they both have the same requirement") + + # This is not the best behavior I believe, it would be better if both + # requirements are considered if they are compatible, and a version + # satisfying both is chosen. But not sure about changing it right now, so + # I went with a warning for the time being. + expect(the_bundle).to include_gems("rubocop 1.37.1") + end + + it "considers both dependencies for resolution if a gem is added once in Gemfile and also inside a local gemspec as a runtime dependency, with different requirements" do + build_lib "my-gem", :path => bundled_app do |s| + s.add_dependency "rubocop", "~> 1.36.0" + end + + build_repo4 do + build_gem "rubocop", "1.36.0" + build_gem "rubocop", "1.37.1" + end + + gemfile <<~G + source "#{file_uri_for(gem_repo4)}" + + gemspec + + gem "rubocop" + G + + bundle :install + + expect(err).to be_empty + expect(the_bundle).to include_gems("rubocop 1.36.0") + end + it "throws an error if a gem is added twice in Gemfile when version of one dependency is not specified" do install_gemfile <<-G, :raise_on_error => false source "#{file_uri_for(gem_repo2)}"