Bundler 2: [Prerelease] Add version resolver #3319

feelepxyz · 2021-03-22T17:54:41Z

Add native helpers for bundler 2 version resolver.

Added two pending specs as they require the LatestVersionFinder.

TODO

Add specs when bundler 2 version is specified
Add specs when updating gems that require a specific major version of bundler, e.g. guard-bundler

feelepxyz · 2021-03-23T11:24:25Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

+
+          let(:dependency_files) { project_dependency_files("bundler2/bundler_specified") }
+
+          it "returns nil as resolution returns the bundler version installed by core" do


Ref: https://github.com/dependabot/dependabot-core/pull/3319/files#diff-8619130a1efd74403d2096bc77c13c862073216cecadb1ae2e00ec4a62070675R34

This is a bummer, it doesn't seem like we can currently update bundler at all.

When running a dry-run to update a gemfile with bundler v1 it tries to go to v2 even though the project has been bundled with v1, it looks like it falls through here atm: https://github.com/dependabot/dependabot-core/blob/main/bundler/helpers/v1/lib/functions/version_resolver.rb#L24 because bundler isn't listed as one of the gems in the definition.

When running the v2 helpers, bundler is listed as a gem but we return nil if the gem name is bundler, I think this is the best thing for now as the "resolved version" is core's bundler version, not the latest resolvable version.

feelepxyz · 2021-03-23T11:25:23Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

+
+        let(:dependency_files) { project_dependency_files("bundler2/requires_bundler") }
+
+        pending "resolves version" do


These specs require the LatestVersionFinder and all the dependency source native helpers to be added so left these as pending until we have that merged in.

Should be able to remove pending once this is in #3327

I'll do this downstream in a separate PR 👍

feelepxyz · 2021-03-23T11:25:35Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

          let(:dependency_files) do
            project_dependency_files("bundler1/imports_gemspec_version_clash_old_required_ruby_no_lockfile")
          end
-          let(:gemfile_fixture_name) { "imports_gemspec_version_clash" }


feelepxyz · 2021-03-23T11:26:48Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

+          end
+          let(:current_version) { "3.0.1" }
+
+          it "raises a DependencyFileNotResolvable error" do


Previously it looked like bundler v1 ignores the minimum ruby version in the gemspec (see spec above), I think this new behaviour is correct as the currently installed version of statesman doesn't satisfy the ruby version in the project gemspec.

feelepxyz · 2021-03-23T11:27:53Z

bundler/spec/dependabot/bundler/update_checker_spec.rb

@@ -2074,98 +2074,4 @@
      it { is_expected.to eq(false) }
    end
  end
-
-  context "with bundler 2 support enabled" do


All these specs fail because the v2 version resolver is used and returns errors.

feelepxyz · 2021-03-23T11:28:59Z

bundler/spec/dependabot/bundler/file_updater_spec.rb

@@ -1708,29 +1708,4 @@
      end
    end
  end
-
-  context "with bundler 2 support enabled" do


These specs fail because the v2 version resolver is used and returns errors.

feelepxyz · 2021-03-23T11:33:34Z

bundler/helpers/v2/monkey_patches/definition_ruby_version_patch.rb

@@ -8,11 +8,11 @@ def index
      if ruby_version
        requested_version = ruby_version.to_gem_version_with_patchlevel
        sources.metadata_source.specs <<
-          Gem::Specification.new("ruby\0", requested_version)
+          Gem::Specification.new("Ruby\0", requested_version)


Looks like it changed here: rubygems/rubygems@4e950f5

feelepxyz · 2021-03-24T14:24:51Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

+        end
+      end
+
+      context "when bundled with v2 and requesting a version that requires bundler v1", :bundler_v2_only do


@jurre looks like this spec is not actually raising, not sure there's anything wrong with that though so probably just a bogus test case that can be removed.

feelepxyz · 2021-03-24T14:25:09Z

bundler/spec/dependabot/bundler/update_checker/version_resolver_spec.rb

+        let(:dependency_files) { project_dependency_files("bundler2/requires_bundler") }
+
+        pending "resolves version" do
+          expect(subject.version).to eq(Gem::Version.new("3.0.0"))


This should be expect(subject[:version]).to eq(Gem::Version.new("3.0.0"))

Cheers 👍

Up until the Bundler 2 upgrade, when in the context of updating a library, dependabot would first try locking the Ruby version according to the minimum ruby requirement specified in the gemspec (if any). If that fail, dependabot would retry without any ruby version locking by detecting a specific error message raised by Bundler. This feature was introduced by fcd5682. However, when the upgrade to Bundler 2 happened, the related spec started failing, because the string to look for within Bundler's error message was not updated to match what Bundler 2 was raising (error message was changed upstream at rubygems/bundler#6647). Instead, the spec was updated to match the new result (see dependabot#3319 (comment)). In the spec, dependabot is updating a Gemfile including ```ruby gem 'statesman', "~> 3.0.0" ``` in combination with a gemspec including ```ruby required_ruby_version ">= 1.9.3" ``` Statesman 3.0.0 has a Ruby ">= 2.2" requirement, so it does not support Ruby 1.9.3 already. The original dependabot behaviour was to ignore the preexisting mismatch and move on. That makes sense to me, there's some reasons why a library main have this situation. In my case, I have several Gemfiles testing different major versions of my dependencies (Rails, in particular), and some of them don't support the oldest Ruby supported by my gem (my Rails 7 gemfile does not support my oldest supported Ruby, 2.6). On a more pragmatic point of view, the old behavior didn't cause any reported issues that I know of, while the new behavior did bite my particular case. So this commit changes the expectation to what it used to be and updates the strings to look for in error messages to support both Bundler 1 and Bundler 2 error messages.

Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them.

Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them, and then fixes the failures. One test was straightforward change from method to hash value. But the other was a logical change from expecting a raised error to expecting no error. However, this has been true since this code was originally committed (once #3327 was merged to make the test work): https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250 I considered removing it, but thought it didn't hurt to continue to keep it in to ensure no regressions... it is a `bundler 2` test, so still valid even after we drop support for `bundler 1`.

Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` marker, but that accidentally got overlooked. So this removes the `pending` marker, and then fixes the failures: 1. The first was a straightforward change from method to hash value. 2. The second wasn't raising the expected error... However, this has been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250) (once #3327 was merged to make the test work). So I deleted the test as it added no value.

feelepxyz added 4 commits March 22, 2021 17:25

Bundler 2: Add version resolver native helper

55460c7

Fix required ruby version monkey patch

ffa5288

Remove bundler 2 specs from update checker and file updater

02e27c6

Add bundler 2 specific tets

eff6206

feelepxyz changed the title ~~WIP: Bundler 2 version resolver~~ Bundler 2: [Prerelease] Add version resolver Mar 23, 2021

feelepxyz marked this pull request as ready for review March 23, 2021 11:23

feelepxyz requested a review from a team as a code owner March 23, 2021 11:23

feelepxyz commented Mar 23, 2021

View reviewed changes

brrygrdn mentioned this pull request Mar 23, 2021

Bundler 2: [Prerelease] Implement v2 native functions for LatestVersionFinder #3327

Merged

1 task

jurre approved these changes Mar 24, 2021

View reviewed changes

Merge branch 'main' into feelepxyz/bundler2-version-resolver

6a68837

jurre merged commit 495b7ab into main Mar 24, 2021

jurre deleted the feelepxyz/bundler2-version-resolver branch March 24, 2021 14:18

feelepxyz commented Mar 24, 2021

View reviewed changes

thepwagner mentioned this pull request Mar 25, 2021

v0.138.4 #3354

Merged

jeffwidman mentioned this pull request Nov 22, 2022

Re-enable two pending tests #6194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundler 2: [Prerelease] Add version resolver #3319

Bundler 2: [Prerelease] Add version resolver #3319

feelepxyz commented Mar 22, 2021 •

edited

Loading

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021 •

edited

Loading

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021

jurre Mar 24, 2021

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021

feelepxyz Mar 23, 2021

feelepxyz Mar 24, 2021

feelepxyz Mar 24, 2021

jurre Mar 24, 2021


		let(:dependency_files) { project_dependency_files("bundler2/bundler_specified") }

		it "returns nil as resolution returns the bundler version installed by core" do


		let(:dependency_files) { project_dependency_files("bundler2/requires_bundler") }

		pending "resolves version" do

Bundler 2: [Prerelease] Add version resolver #3319

Bundler 2: [Prerelease] Add version resolver #3319

Conversation

feelepxyz commented Mar 22, 2021 • edited Loading

TODO

Choose a reason for hiding this comment

feelepxyz Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feelepxyz commented Mar 22, 2021 •

edited

Loading

feelepxyz Mar 23, 2021 •

edited

Loading