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

[deivid-rodriguez]

Sorry for the long ticket, sometimes I feel I am too verbose, but I tried my best to justify my proposal

Context

I commit Gemfile.lock files to source control when developing Ruby libraries. This is somewhat controversial and not widely adopted, but it's currently the official recommendation of the Bundler team, and other package managers also agree (https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/).

Anyways, that currently means that I get security advisories and dependabot PRs to warn me about vulnerabilities in my Gemfile.lock files and to update them.

However, sometimes dependabot is unable to create PRs for some of these security advisories, because of some special behavior it has for libraries.

The special behavior is that if it's updating a Gemfile.lock file inside a library, dependabot will parse the required_ruby_version field in the gemspec file, and use the lower bound for it as the required ruby version for the resolution. That means that sometimes it will not be able to update a Gemfile.lock file to a secure version because the secure version no longer supports the oldest Ruby version supported by the library.

An example of this is https://github.com/formtastic/formtastic/security/dependabot/15:

Dependabot is only able to update to nokogiri-1.12.5, because it adds a requirement on "Ruby 2.5" to the resolution (the gemspec includes required_ruby_version >= 2.5), and 1.12.5 is the newest nokogiri version that still supports Ruby 2.5.

Why it currently works like this?

This feature was added a very long time ago with 9d08fc2. I think the reason is the following:

• As you probably know, Gemfile and Gemfile.lock files in libraries are not used at all by end users of the libraries, it's the Gemfile and Gemfile.lock files in end applications using the library that get considered. The only thing actually meaningful for end users are the dependency requirements in the gemspec file of the library.

• Gemfile and Gemfile.lock files are important though for developers of the library, since they usually determine the exact set of dependencies and their versions the library is tested against. It's normally impractical to test a library against all possible combinations of its dependencies, so Gemfile and Gemfile.lock files help choosing what's worth testing against.

• I think adding this extra requirement on the Ruby version is intended to help making sure that the set of dependencies the library is tested against is compatible with the oldest Ruby version supported by the library. And I think that's intended to avoid introducing changes that unintentionally drop support for old Rubies.

Why I think it should be changed?

I think there are a few reasons to change this:

• Simplicity. We can get rid of a lot of "library specific code". Code that's also very dependent on Bundler internals and likely to have issues, like the one fixed by Fix Ruby version conflict error detection for new Bundler versions #4820.
• Consistency. Currently dependabot does this, but only "sometimes". For example, if it finds any issues caused by the extra ruby requirement, it will retry without it. This was introduced by fcd5682.
• Not necessary most of the times. Most libraries test against all major Rubies they support, and if they have to choose, they definitely test against the oldest supported Ruby. So even if a PR created by dependabot due to this change made the library incompatible with that Ruby version, it will be cached by CI.
• Clarity. As can be seen in the above screenshot, it's currently very hard to figure out why Dependabot could not update the dependency, because this extra requirement on the Ruby version is added "silently".

And most importantly

• Security. Security issues are most important for production environments, but can also affect developer environments. By leaving Gemfile.lock files out there with insecure versions of gems, we leave room for those issues hitting developer environments.

If the above makes sense, I'm happy to create a PR for this.

Thanks for reading!

[jurre]

Thanks for the detailed issue @deivid-rodriguez! Overall I think you make a good point, but I am wondering if we don't end up breaking users setups when doing this, if someone is using the lower bound ruby version as specified in the Gemfile, and dependabot now performs this update, all of a sudden we've broken their development environment in a way they wouldn't have if they'd performed the update manually.

I wonder if we should instead update the ruby requirement in the Gemfile in this case, but it might also be confusing and not what users want 🤔

All in all, I am not sure what the right thing to do is, pinging other folks on the team to gather their thoughts.

[jurre]

One improvement we could make if we do decide to stick with the current approach is to attempt explaining why that's the latest version that could be installed. We already do this for cases when there's a transitive dependency conflict, and might be able to do the same thing here. Not saying that this is the right thing to do, just dumping my thoughts

[brrygrdn]

I think there are a few reasons to change this

@deivid-rodriguez I agree with the rationale, especially Simplicity/Consistency as I think those are general improvement targets we should aim for 👍🏻

I am wondering if we don't end up breaking users setups when doing this

This is a concern I share, I think there are two aspects to separate out:

1. What is the objectively correct - or least surprising - behaviour for Dependabot as a library?
2. Assuming we do make a change, how do we ship a behavioural change like this to the service?

I think @jurre suggestion above that we attempt to explain that the latest version is pinned by the gemspec is optimised for solving the second and is likely something we can deliver more easily.

If we were to optimise for (1) by changing this behaviour I think we'd need to look at:

• How frequently do we encounter this kind of pinning in the wild? If it's a sufficiently small percentage we might have more appetite to just change it?
• If we were to change the behaviour, is there anything we should add to the PR to make it obvious we have ignored required_ruby_version?

[brrygrdn]

🤔 We could isolate the old/new behaviour behind a configuration value and graduate the improved behaviour as optional at first and then deprecate it by flipping the configuration flag to an opt in to the old behaviour

[jurre]

Yeah good call on how to go about rolling out such a change @brrygrdn

🤔 We could isolate the old/new behaviour behind a configuration value and graduate the improved behaviour as optional at first and then deprecate it by flipping the configuration flag to an opt in to the old behaviour

Would really love to avoid this sort of configuration option as it's overly specific and I'd hope we can instead identify the Right Thing To Do™

[deivid-rodriguez]

Hei, thanks for your responses!

Just to clarify the current and proposed behavior.

• The required_ruby_version in the gemspec won't be ignored, it will still be respected, before and after this proposal.
• If there's a ruby directive in the Gemfile, it will still be respected, before and after this proposal.

The difference between the current and proposed behavior is that if you have, say, required_ruby_version = ">= 2.5.0" in your gemspec file, and no ruby directive in your Gemfile. dependabot currently adds the following on top of your Gemfile

ruby "~> 2.5.0"

So, dependabot is currently forcing the resolution to be valid on Ruby 2.5, while with the new behavior, dependabot would find a resolution valid on some Ruby >= 2.5.0.

Could this proposal break some developer setup? I guess, yeah. If a developer of a gem is developing using the oldest ruby the gem supports, and have Gemfile (and possibly Gemfile.lock) files reflecting that, dependabot could create a PR that breaks that user's setup. But only if the PR is merged, and I think the great majority of the gems out there test in CI against the oldest Ruby they support. So I think it's an assumable risk, and in any case it might help the gem author realize that her CI is missing some coverage.

[jurre]

Thanks David, I guess my main concern is users getting upset that Dependabot is opening a PR that breaks the assumptions of their setup, when it could have figured out the "right" thing to do (although "right" is dubious in this case, I totally see your point).

I'm worried that if users have a setup like this, but they don't have CI set up for their oldest ruby version and they now release an incompatible version for the ruby version they're supposedly supporting, they might end up being frustrated with Dependabot's behavior.

I wonder if in this case we should bump the required_ruby_version directive in the users gemspec to a range that is valid together with the update? This might be complex, but at least we're being clear about what accepting the updated PR means in that case?

I'm not necessarily convinced that what I'm saying is "right" here, I'm mostly just thinking out loud to make sure we end up making a well thought-out decision, I really appreciate the perspective you're bringing!

[deivid-rodriguez]

I totally understand your concern @jurre.

The idea of bumping the required_ruby_version directive in the gemspec is interesting, but it seems to me that would be mixing two separate concerns in the same PR. Dropping support for an old ruby is one thing, bump the version of a dependency seems something unrelated. Mainly because the former actually affect end users using the dropped Ruby (who won't be able to use future releases), while the latter only affects developers of the gem.

To be honest, what bothers me the most about the current behavior is that dependabot creates a security alert but it's not able to offer a solution nor to properly explain what went wrong. I think improving the error message to explain that Dependabot tried to upgrade the dependency using the oldest supported Ruby, and that was not possible, and to recommend to drop the old Ruby in order to let dependabot upgrade the dependency would go a long way.

[jurre]

The idea of bumping the required_ruby_version directive in the gemspec is interesting, but it seems to me that would be mixing two separate concerns in the same PR. Dropping support for an old ruby is one thing, bump the version of a dependency seems something unrelated. Mainly because the former actually affect end users using the dropped Ruby (who won't be able to use future releases), while the latter only affects developers of the gem.

That's totally fair, yeah, although bumping to a version of a dependency that doesn't support their version of ruby also effects them in the same way, right?

This is probably what I would do manually when running into this situation though

To be honest, what bothers me the most about the current behavior is that dependabot creates a security alert but it's not able to offer a solution nor to properly explain what went wrong. I think improving the error message to explain that Dependabot tried to upgrade the dependency using the oldest supported Ruby, and that was not possible, and to recommend to drop the old Ruby in order to let dependabot upgrade the dependency would go a long way.

Yeah this is a much bigger problem, we've been able to tackle explaining some failed updates, but this one might be a good one to tackle

[deivid-rodriguez]

That's totally fair, yeah, although bumping to a version of a dependency that doesn't support their version of ruby also effects them in the same way, right?

Only if the gemspec is changed to not allow the previous version. If dependabot proposes changes only to Gemfile and/or Gemfile.lock files, those won't affect end users no matter what. And even if it changes the gemspec, that only results in support drop if the versioning strategy has been changed from the default, since widen, which is the default for Ruby libraries, does not result in bumping the lower bound for the dependency.

Now that I think about it, the current implementation does make sense to prevent the situation you're mentioning: avoid a dependency being bumped in the gemspec, causing a implicit support drop of the oldest Ruby. Perhaps it would be acceptable to accept my proposal for "development updates" (those that don't affect end users), but keep the current behavior in other situations.

[schinery]

Don't know if this is relevant to this conversation or a separate issue but I have two private GitHub package (rubygems.pkg.github.com) gems, both having spec.required_ruby_version = ">= 3.1" in their .gemspec.

One of the gems has ruby "3.1.1" defined in its Gemfile and works as expected when any new gems are found that need updating. However, the other gem doesn't have the Ruby version defined in the gem file, and when it finds any gems to update it throws a Bundler found conflicting requirements for the Ruby version error.

updater | ERROR <job_343082656> Error processing nokogiri (Dependabot::SharedHelpers::HelperSubprocessFailed)
updater | ERROR <job_343082656> Bundler found conflicting requirements for the Ruby version:
updater | <job_343082656>   In Gemfile:
updater | <job_343082656>     my_gem was resolved to 4.63.1, which depends on
updater | <job_343082656>       Ruby (>= 3.1)
updater | <job_343082656> 
updater | <job_343082656>     pry-byebug (~> 3.9) was resolved to 3.9.0, which depends on
updater | <job_343082656>       byebug (~> 11.0) was resolved to 11.1.3, which depends on
updater | <job_343082656>         Ruby (>= 2.4.0)
updater | <job_343082656> 
updater | <job_343082656>     faker (~> 2.20) was resolved to 2.20.0, which depends on
updater | <job_343082656>       Ruby (>= 2.5)
updater | <job_343082656> 
updater | <job_343082656>     i18n-tasks (~> 1.0) was resolved to 1.0.9, which depends on
updater | <job_343082656>       Ruby (< 4.0, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     net-ftp (~> 0.1) was resolved to 0.1.3, which depends on
updater | <job_343082656>       Ruby (>= 2.3.0)
updater | <job_343082656> 
updater | <job_343082656>     pry-rails (~> 0.3) was resolved to 0.3.9, which depends on
updater | <job_343082656>       Ruby (>= 1.9.1)
updater | <job_343082656> 
updater | <job_343082656>     rspec-rails (~> 5.1) was resolved to 5.1.1, which depends on
updater | <job_343082656>       Ruby (>= 2.2.0)
updater | <job_343082656> 
updater | <job_343082656>     my_rubocop (= 0.0.2) was resolved to 0.0.2, which depends on
updater | <job_343082656>       Ruby (>= 2.7)
updater | <job_343082656> 
updater | <job_343082656>     m (~> 1.5) was resolved to 1.6.0, which depends on
updater | <job_343082656>       Ruby (>= 1.9)
updater | <job_343082656> 
updater | <job_343082656>     minitest-macos-notification (~> 1.0) was resolved to 1.0.1, which depends on
updater | <job_343082656>       minitest (~> 5.14) was resolved to 5.15.0, which depends on
updater | <job_343082656>         Ruby (< 4.0, >= 2.2)
updater | <job_343082656> 
updater | <job_343082656>     minitest-reporters (~> 1.5) was resolved to 1.5.0, which depends on
updater | <job_343082656>       Ruby (>= 1.9.3)
updater | <job_343082656> 
updater | <job_343082656>     mocha (~> 1.11) was resolved to 1.13.0, which depends on
updater | <job_343082656>       Ruby (>= 1.8.7)
updater | <job_343082656> 
updater | <job_343082656>     spy (~> 1.0) was resolved to 1.0.2, which depends on
updater | <job_343082656>       Ruby (>= 2.1.0)
updater | <job_343082656> 
updater | <job_343082656>     with_model (~> 2.1) was resolved to 2.1.6, which depends on
updater | <job_343082656>       Ruby (>= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     Ruby
updater | <job_343082656> 
updater | <job_343082656> Bundler could not find compatible versions for gem "my_gem":
updater | <job_343082656>   In snapshot (Gemfile.lock):
updater | <job_343082656>     my_gem (= 4.63.1)
updater | <job_343082656> 
updater | <job_343082656>   In Gemfile:
updater | <job_343082656>     my_gem
updater | <job_343082656> 
updater | <job_343082656> Running `bundle update` will rebuild your snapshot from scratch, using only
updater | <job_343082656> the gems in your Gemfile, which may resolve the conflict.
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:121:in `run_helper_subprocess'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/native_helpers.rb:44:in `block in run_bundler_subprocess'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:382:in `block in with_original_env'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:698:in `with_env'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:382:in `with_original_env'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/native_helpers.rb:40:in `run_bundler_subprocess'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:68:in `block in build_updated_lockfile'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `block in in_a_temporary_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `chdir'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `in_a_temporary_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:37:in `in_a_temporary_repo_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:62:in `build_updated_lockfile'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:46:in `updated_lockfile_content'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater.rb:157:in `updated_lockfile_content'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater.rb:41:in `updated_dependency_files'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:678:in `generate_dependency_files_for'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:274:in `check_and_create_pull_request'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:82:in `check_and_create_pr_with_error_handling'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `block in run'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `each'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `run'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/update_files_job.rb:17:in `perform_job'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/base_job.rb:35:in `run'
updater | ERROR <job_343082656> bin/update_files.rb:22:in `<main>'

Again, I don't know if this is relevant and am happy to open a separate issue for this if it is not. I've also added ruby "3.1.1" to the gem that didn't have it to check if it is that which is causing the issue, just waiting for another gem bump to confirm.

Also worth noting the the gem without Ruby defined in the Gemfile did work at some point, and don't think anything dramatic has changed with it recently.

[deivid-rodriguez]

Interesting @schinery! It seems definitely related and I think it's worth looking into it and fixing your issue before proceeding with anything here. It might shed some light and what the best/right/expected behavior is.

One guess would be a conflict between your gem requirements (Ruby >= 3.1) and the Ruby version run by Dependabot internally (2.7)? Dependabot definitely has some solution for that since the gem with ruby in the Gemfile is working, but maybe it's not getting applied when there's no ruby directive.

Also, since you said this only stopped working recently, I wonder if #4820 affected this somehow...