Fix Ruby version conflict error detection for new Bundler versions #4820

deivid-rodriguez · 2022-03-09T19:31:08Z

Hello!

I'm having some issues running specs locally, and I won't be able to finish this work (and possibly write some specs) today, but I want to open a WIP PR if that's ok, so that I don't lose the context and that I can check whether the current version of my patch is ok with your CI.

On some of my repositories I'm getting mysterious Ruby version conflict errors that make dependabot updates fail. See for example: https://github.com/activeadmin/arbre/security/dependabot/9, which shows this error:

Bundler::VersionConflict with message: Bundler found conflicting requirements for the Ruby version:
  In Gemfile:
    Ruby (~> 2.6.7.0)

    rails (~> 7.0.2) was resolved to 7.0.2.2, which depends on
      actionpack (= 7.0.2.2) was resolved to 7.0.2.2, which depends on
        Ruby (>= 2.7.0)

However, my library does not impose any Ruby version requirements, so I wasn't sure where this was coming from.

After investigation, I noticed that dependabot is automatically adding this Ruby version constraint to the resolution when updating libraries.

After further investigation, I noticed that dependabot also wants to retry without this constraint, in case adding it fails. It does this by checking the content of the specific error message given by Bundler in this case. In particular

Bundler could not find compatible versions for gem "ruby\0"

However, in rubygems/bundler#6647 (released with Bundler 2.1.0.pre.1), this message was changed to

Bundler found conflicting requirements for the Ruby\0 version

That means that when using recent Bundler versions, the error message not longer matches, so dependabot leaves the extra Ruby version constraint there, and update fails.

This PR fixes the issue by also checking for the new error message in order to remove the extra Ruby version constraint.

I tried dependabot-script and it worked just fine with this change, but I want to check full CI too.

deivid-rodriguez · 2022-03-09T20:09:46Z

I noticed that when Bundler v1 and Bundler v2 behavior differ, you extract interface methods that then get their own version specific implementation at bundler/helper/v1 and bundler/helper/v2. I was not sure it was worth doing that here, but I'm happy to do it if that's the way to go!

jurre · 2022-03-10T07:39:08Z

I noticed that when Bundler v1 and Bundler v2 behavior differ, you extract interface methods that then get their own version specific implementation at bundler/helper/v1 and bundler/helper/v2. I was not sure it was worth doing that here, but I'm happy to do it if that's the way to go!

No this is fine! Those extracted interface methods are programs that we run as standalone CLIs and parse the results back to the main process. This is mostly used for when we use bundler as a library in order to avoid mismatches between the bundler used to manage dependabot-core itself and the one used to perform updates

As for running locally, are you using bin/docker_dev_shell? If not I'd recommend trying that route, running the project stand-alone is very hard and not well supported

Also, thanks for contributing!

deivid-rodriguez · 2022-03-10T07:46:14Z

No this is fine! Those extracted interface methods are programs that we run as standalone CLIs and parse the results back to the main process. This is mostly used for when we use bundler as a library in order to avoid mismatches between the bundler used to manage dependabot-core itself and the one used to perform updates

Ah, I see!

As for running locally, are you using bin/docker_dev_shell? If not I'd recommend trying that route, running the project stand-alone is very hard and not well supported

No, indeed I tried running what's in the script/ci-test directly. I actually got it to work, at least for bundler 2, with a minor modification that I will contribute next. But I will use bin/docker_dev_shell from now on!

Also, thanks for contributing!

You're welcome, thanks to you for dependabot :)

deivid-rodriguez · 2022-03-10T10:11:48Z

Turns out that while my changes fix my use case, they break a different use case. The difference between our use cases is the presence of a lockfile. In my case, there's a Gemfile.lock file so dependabot should not add any extra magic about Ruby version requirements. If there's no lockfile though, it's totally reasonable that dependabot tries to make sure resolution supports the oldest Ruby supported by the library, and that's the spec this is breaking.

I'll dig deeper.

deivid-rodriguez · 2022-03-10T12:43:38Z

I fixed the broken spec in the Bundler 2 suite, and added a spec for this fix. But now I have a broken spec in the Bundler 1 suite 😬. Will keep digging.

deivid-rodriguez · 2022-03-10T14:17:34Z

I think the current failure is some flaky failure, but other than that seems green.

However, I need to look closer at the consequences of doing this. It fixes my use case but I think it might break other cases.

I imagine now if a library provides a Gemfile + Gemfile.lockfile + gemspec, then dependabot could potentially change a gemspec dependency to a range that no longer resolves on the minimum ruby supported, and that would be bad since it would mean unintentionally dropping old rubies support (although it would most likely be detected by CI running on the minimum ruby since the new lockfile would no longer be installable there).

It seems like a very rare case, but I want to double check the new behavior.

jurre · 2022-03-10T17:04:45Z

I think the current failure is some flaky failure, but other than that seems green.

Yes, we unfortunately have a few flaky tests in our bundler suite, one of which is fixed here

jurre · 2022-03-11T13:06:43Z

@deivid-rodriguez is this ready for review?

deivid-rodriguez · 2022-03-11T15:47:35Z

Still a WIP and I'm also not even sure this makes sense in general, or just for my use case. In my libraries I use a lockfile_only versioning strategy, so it definitely makes sense in that context. But I'm not sure if ignoring this ruby requirement dependabot adds is too aggressive for other cases.

So, not ready for a review, but happy to get feedback about the idea.

deivid-rodriguez · 2022-03-18T14:30:02Z

Alright, so I had another look at this and my conclusion is that this is a bug introduced when Bundler 2 was added. I did my best to explain the context, and the relevant commits/PRs both in the dependabot and bundler side. The explanation is at a2def54.

However, I tried this PR against my repository and while it did successfully generate a PR (see activeadmin/activeadmin#7400), it updated the Gemfile which I guess should be "forbidden" given that we use the lockfile-only versioning strategy in our configuration. And I think that issue has been introduced by this PR, because an analogous and recent PR did not have this issue (see activeadmin/activeadmin#7390).

So... I need to investigate that latter issue more.

It doesn't seem to break any tests, including the one added with the fix (which I'm keeping). Also the regexp matching of the error message no longer matches Bundler 2 error message and that hasn't caused any issues. So it seems that this code is not necessary.

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.

deivid-rodriguez · 2022-03-18T16:50:45Z

Alright I figured that last issue. The problem is that I was generating my activeadmin PRs using this branch + https://github.com/dependabot/dependabot-script.

However, dependabot-script does not support the lockfile-only versioning strategy since it's missing a condition here:

https://github.com/dependabot/dependabot-script/blob/4330ff7043b6fe2bb009005e2f5b0ca9985f32f2/generic-update-script.rb#L182-L190

as opposed to the in-repo dry-run script which does check the proper condition at line 680

dependabot-core/bin/dry-run.rb

Lines 679 to 688 in d0d849a

    
           requirements_to_unlock = 
        
             if $options[:lockfile_only] || !checker.requirements_unlocked_or_can_be? 
        
               if checker.can_update?(requirements_to_unlock: :none) then :none 
        
               else :update_not_possible 
        
               end 
        
             elsif checker.can_update?(requirements_to_unlock: :own) then :own 
        
             elsif checker.can_update?(requirements_to_unlock: :all) then :all 
        
             else :update_not_possible 
        
             end

So, not an issue related to this PR, so I'm marking this as ready!

jeffwidman · 2022-03-21T18:21:14Z

So, not an issue related to this PR, so I'm marking this as ready!

@deivid-rodriguez While outside the scope of this PR, it sounds like this is a real issue that needs addressing. Are you planning to tackle this issue in a separate PR or file an issue so all your research work doesn't get forgotten about?

jurre · 2022-03-21T18:41:47Z

Maybe we should just retire dependabot-script, no one is really maintaining it and it really only serves as an example of how one might use dependabot-core. I wish the code from our dry-run script was a little more composable/reusable and folks could just use that instead, but alas.

deivid-rodriguez · 2022-03-21T19:04:31Z

I'm not sure what are the differences between the two, but they look very similar. I guess (from naming) one difference is that dry-run.rb is not supposed to actually create PR's.

I guess it makes sense to merge them and eventually retire dependabot-script. I'm happy to contribute my fix to dependabot-script to start reducing the delta between the two scripts and open an issue to eventually merge them. How about that?

jeffwidman · 2023-01-23T20:27:26Z

I filed dependabot/dependabot-script#891 so we don't lose track of ☝️.

There's a larger-scope conversation about melding dry-run/dependabot-script or possibly migrating to the CLI but until we get a chance to tackle that, the above issue at least notes the divergence in functionality.

deivid-rodriguez force-pushed the fix-ruby-error-detection branch 2 times, most recently from e2afd61 to d4c4c74 Compare March 9, 2022 19:43

deivid-rodriguez mentioned this pull request Mar 10, 2022

Update ci-test script to use bundler 2.3.8 #4824

Merged

deivid-rodriguez force-pushed the fix-ruby-error-detection branch from d4c4c74 to b5749ea Compare March 10, 2022 11:44

deivid-rodriguez force-pushed the fix-ruby-error-detection branch 2 times, most recently from 74ca25f to 2c04439 Compare March 10, 2022 13:54

deivid-rodriguez mentioned this pull request Mar 10, 2022

Bump rubocop from 1.25.1 to 1.26.0 activeadmin/arbre#333

Merged

deivid-rodriguez force-pushed the fix-ruby-error-detection branch 3 times, most recently from 49c863e to a2def54 Compare March 18, 2022 13:34

deivid-rodriguez mentioned this pull request Mar 18, 2022

Bump rails from 7.0.2.2 to 7.0.2.3 activeadmin/activeadmin#7400

Closed

deivid-rodriguez added 2 commits March 18, 2022 16:06

Revert b692568

2462acb

It doesn't seem to break any tests, including the one added with the fix (which I'm keeping). Also the regexp matching of the error message no longer matches Bundler 2 error message and that hasn't caused any issues. So it seems that this code is not necessary.

deivid-rodriguez force-pushed the fix-ruby-error-detection branch from a2def54 to e9a759b Compare March 18, 2022 15:06

deivid-rodriguez marked this pull request as ready for review March 18, 2022 16:50

deivid-rodriguez requested a review from a team as a code owner March 18, 2022 16:50

jurre approved these changes Mar 21, 2022

View reviewed changes

jurre merged commit ea8fbaf into dependabot:main Mar 21, 2022

deivid-rodriguez deleted the fix-ruby-error-detection branch March 21, 2022 10:18

jurre mentioned this pull request Mar 23, 2022

v0.180.1 #4898

Merged

deivid-rodriguez mentioned this pull request Apr 12, 2022

Dependabot for Ruby libraries should behave just like for dependabot for applications #4987

Open

jeffwidman mentioned this pull request Jan 23, 2023

dependabot-script does not support the lockfile-only versioning strategy dependabot/dependabot-script#891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ruby version conflict error detection for new Bundler versions #4820

Fix Ruby version conflict error detection for new Bundler versions #4820

deivid-rodriguez commented Mar 9, 2022 •

edited

Loading

deivid-rodriguez commented Mar 9, 2022

jurre commented Mar 10, 2022 •

edited

Loading

deivid-rodriguez commented Mar 10, 2022

deivid-rodriguez commented Mar 10, 2022 •

edited

Loading

deivid-rodriguez commented Mar 10, 2022

deivid-rodriguez commented Mar 10, 2022 •

edited

Loading

jurre commented Mar 10, 2022 •

edited

Loading

jurre commented Mar 11, 2022

deivid-rodriguez commented Mar 11, 2022

deivid-rodriguez commented Mar 18, 2022

deivid-rodriguez commented Mar 18, 2022

jeffwidman commented Mar 21, 2022 •

edited

Loading

jurre commented Mar 21, 2022

deivid-rodriguez commented Mar 21, 2022 •

edited

Loading

jeffwidman commented Jan 23, 2023

Fix Ruby version conflict error detection for new Bundler versions #4820

Fix Ruby version conflict error detection for new Bundler versions #4820

Conversation

deivid-rodriguez commented Mar 9, 2022 • edited Loading

deivid-rodriguez commented Mar 9, 2022

jurre commented Mar 10, 2022 • edited Loading

deivid-rodriguez commented Mar 10, 2022

deivid-rodriguez commented Mar 10, 2022 • edited Loading

deivid-rodriguez commented Mar 10, 2022

deivid-rodriguez commented Mar 10, 2022 • edited Loading

jurre commented Mar 10, 2022 • edited Loading

jurre commented Mar 11, 2022

deivid-rodriguez commented Mar 11, 2022

deivid-rodriguez commented Mar 18, 2022

deivid-rodriguez commented Mar 18, 2022

jeffwidman commented Mar 21, 2022 • edited Loading

jurre commented Mar 21, 2022

deivid-rodriguez commented Mar 21, 2022 • edited Loading

jeffwidman commented Jan 23, 2023

deivid-rodriguez commented Mar 9, 2022 •

edited

Loading

jurre commented Mar 10, 2022 •

edited

Loading

deivid-rodriguez commented Mar 10, 2022 •

edited

Loading

deivid-rodriguez commented Mar 10, 2022 •

edited

Loading

jurre commented Mar 10, 2022 •

edited

Loading

jeffwidman commented Mar 21, 2022 •

edited

Loading

deivid-rodriguez commented Mar 21, 2022 •

edited

Loading