dependabot-core icon indicating copy to clipboard operation
dependabot-core copied to clipboard

Monkey patch to add method default_gem? to Bundler LazySpecification

Open honeyankit opened this issue 9 months ago • 22 comments

Context

Added method default_gem? to Bundler::LazySpecification to fix the following issue:

updater | 2024/04/24 19:17:00 ERROR <job_819371770> Error processing active_model_serializers (Dependabot::SharedHelpers::HelperSubprocessFailed)
2024/04/24 19:17:00 ERROR <job_819371770> undefined method `default_gem?' for #<Bundler::LazySpecification:0x00007f9b5a13cae8 @name="feature-flags-client", @version=#<Gem::Version "1.1.0">, @dependencies=[<Gem::Dependency type=:runtime name="faraday" requirements=">= 0">, <Gem::Dependency type=:runtime name="faraday-retry" requirements=">= 0">, <Gem::Dependency type=:runtime name="monolith-twirp-features-core" requirements="~> 1.1.1">, <Gem::Dependency type=:runtime name="twirp" requirements="~> 1.10">], @required_ruby_version=#<Gem::Requirement:0x00007f9b6fd8de90 @requirements=[[">=", #<Gem::Version "0">]], @_sorted_requirements=[[">=", #<Gem::Version "0">]], @_tilde_requirements=[]>, @required_rubygems_version=#<Gem::Requirement:0x00007f9b6fd8dd78 @requirements=[[">=", #<Gem::Version "0">]], @_sorted_requirements=[[">=", #<Gem::Version "0">]], @_tilde_requirements=[]>, @platform="ruby", @source=#<Bundler::Source::Rubygems:0x1520 rubygems repository https://rubygems.pkg.github.com/github/ or installed locally>, @force_ruby_platform=false, @full_name="feature-flags-client-1.1.0", @use_exact_resolved_specifications=true>

        if spec.default_gem?
               ^^^^^^^^^^^^^
updater | 2024/04/24 19:17:00 ERROR <job_819371770> /home/dependabot/common/lib/dependabot/shared_helpers.rb:189:in `run_helper_subprocess'

honeyankit avatar May 03 '24 00:05 honeyankit

Thanks for getting this fix in! Please is there a way to get test coverage for this?

abdulapopoola avatar May 03 '24 01:05 abdulapopoola

Just want to reiterate that I think we should consider this a temporary solution.

bdragon avatar May 03 '24 22:05 bdragon

@abdulapopoola @bdragon I have added the tests in both the bundler v1 and v2 environment. The test runs in the respective bundler1 and 2 environment.

Note: I will deploy this PR after the re-review of my test cases

honeyankit avatar May 04 '24 01:05 honeyankit

Hmm. The same test is getting passed in my local environment bundler 1 but failing in CI pointing towards some environment mismatch.

docker run  --env "CI=true" --env "RAISE_ON_WARNINGS=true" --env "DEPENDABOT_TEST_ACCESS_TOKEN=*******" --env "SUITE_NAME=bundler1" --rm ghcr.io/dependabot/dependabot-updater-bundler bash -c "cd /home/dependabot/bundler && ./script/ci-test"

honeyankit avatar May 04 '24 02:05 honeyankit

Based on some of the discussion in this PR, I can't help but wonder if this is a case where dropping support for Bundler v1 would have saved a day+ of engineering time that we could have put toward better support of Bundler v2...

cc @jonjanego for visibility.

jeffwidman avatar May 04 '24 04:05 jeffwidman

I support phasing out support for bundler v1 (I'd love to see stats on what percentage of jobs are still targeting v1). That said, in this case I believe the exception is being raised for bundler v2.

bdragon avatar May 04 '24 04:05 bdragon

I actually thought this exception was Bundler 1 specific, and no longer happening in Bundler v2. If happening with the latest version of Bundler, then it seems more worth looking into it upstream. Is there a public repo using Bundler v2 that reproduces the issue?

deivid-rodriguez avatar May 06 '24 07:05 deivid-rodriguez

Do we have usage stats on distribution of bundler 1 vs. bundler 2 jobs?

Bundler v1 appears to not be "formally EOL", but by the transitive nature of its requirements, it's effectively EOL. Per https://bundler.io/guides/bundler_2_upgrade.html v2 requires a minimum ruby version of 2.3.0 - which is already EOL. Bundler v1 requires Ruby 1 - which is well and truly dead.

Unless we are seeing a significant number of jobs of bundler v1 still running, I'm all in favor of dropping support going forward, or at the very least, cease including it in our test cases for future releases

jonjanego avatar May 06 '24 14:05 jonjanego

We do have some metrics here.

And usage is actually significant

cc: @jonjanego

abdulapopoola avatar May 06 '24 15:05 abdulapopoola

Bundler v1 appears to not be "formally EOL", but by the transitive nature of its requirements, it's effectively EOL. Per https://bundler.io/guides/bundler_2_upgrade.html v2 requires a minimum ruby version of 2.3.0 - which is already EOL. Bundler v1 requires Ruby 1 - which is well and truly dead.

I can confirm that Bundler v1 is completely EOL. Last time we looked into dropping Bundler v1 support in Dependabot, there was still a non neglectable number of jobs running Bundler v1, but maybe people have finally moved on now and it's finally time to drop it :pray:

deivid-rodriguez avatar May 07 '24 13:05 deivid-rodriguez

Bundler v1 appears to not be "formally EOL", but by the transitive nature of its requirements, it's effectively EOL. Per https://bundler.io/guides/bundler_2_upgrade.html v2 requires a minimum ruby version of 2.3.0 - which is already EOL. Bundler v1 requires Ruby 1 - which is well and truly dead.

I can confirm that Bundler v1 is completely EOL. Last time we looked into dropping Bundler v1 support in Dependabot, there was still a non neglectable number of jobs running Bundler v1, but maybe people have finally moved on now and it's finally time to drop it 🙏

Do you know of an official source from the bundler team stating it's EOL? Just for our reference

jonjanego avatar May 07 '24 14:05 jonjanego

You can check Bundler policies here.

We should generalize them now that we've settled on "following Ruby". Last time we updated them it was 2022, and we only supported Bundler 2.3, and exceptionally, 2.2 and 2.1.

Basically, the currently policy is, we only support the latest minor version (2.5 right now), and exceptionally, other versions included in Ruby versions that are not EOL'd. That means Bundler 2.4 (shipped with Ruby 3.2), and Bundler 2.3 shipped with Ruby 3.1 as per https://www.ruby-lang.org/en/downloads/branches/.

deivid-rodriguez avatar May 07 '24 15:05 deivid-rodriguez

You can check Bundler policies here.

We should generalize them now that we've settled on "following Ruby". Last time we updated them it was 2022, and we only supported Bundler 2.3, and exceptionally, 2.2 and 2.1.

Basically, the currently policy is, we only support the latest minor version (2.5 right now), and exceptionally, other versions included in Ruby versions that are not EOL'd. That means Bundler 2.4 (shipped with Ruby 3.2), and Bundler 2.3 shipped with Ruby 3.1 as per https://www.ruby-lang.org/en/downloads/branches/.

Thanks for the pointers!

I think that updating the policies to be explicit about it would be helpful to us (for 'what to support'), as well as to the community so there's no ambiguity.

We do still see a fair bit of update jobs running on bundler v1, so something I'm concerned about is breaking people's workflows; but perhaps we could use this as a forcing function for people to upgrade.

jonjanego avatar May 07 '24 16:05 jonjanego

I think that updating the policies to be explicit about it would be helpful to us (for 'what to support'), as well as to the community so there's no ambiguity.

Agreed! I proposed a clarification of the policy at https://github.com/rubygems/rubygems/pull/7633.

We do still see a fair bit of update jobs running on bundler v1, so something I'm concerned about is breaking people's workflows; but perhaps we could use this as a forcing function for people to upgrade.

Yeah, same as before :disappointed:. I guess you can reevaluate when you next upgrade Ruby (I guess around this time next year, you're now running the latest and greatest :smile:) or once it gets in the middle again (my understanding is that it last got in the middle when upgrading to Ruby 3.3 in https://github.com/dependabot/dependabot-core/pull/9597). But as upstream maintainer, I don't have a problem either if you decide to go ahead and do it right now. May be a bit disrupting, but it may indeed force some people to upgrade which is good for everyone!

deivid-rodriguez avatar May 07 '24 17:05 deivid-rodriguez

@deivid-rodriguez i see that your policy update PR got merged, nice! Perhaps it may be helpful to broadcast that policy proactively to make it clear what is/isn't supported.

I've done some digging and we're still seeing upwards of 10k+ update jobs for bundler v1 come thru each month, spread out across 1100+ orgs. However, this is less than 4% of all bundler updates. We'll have to evaluate the options for what we can do to minimize the negative impact to end users if we were to cease support.

jonjanego avatar May 08 '24 22:05 jonjanego

https://github.com/rubygems/rubygems/blob/master/bundler/doc/POLICIES.md?plain=1#L16:

Bundler tries for perfect backwards compatibility. That means that if something worked in version 1.x, it should continue to work in 1.y and 1.z. That thing may or may not continue to work in 2.x. We may not always get it right, and there may be extenuating circumstances that force us into choosing between different kinds of breakage, but compatibility is very important to us. Infrastructure should be as unsurprising as possible.

It sounds like Bundler 2 can be used for the Bundler 1 jobs? I realize some will undoubtedly break, but it might be a small enough percentage that we'd be okay with it... I suppose we could try feature flagging and see what happens.

jeffwidman avatar May 09 '24 03:05 jeffwidman

@deivid-rodriguez i see that your policy update PR got merged, nice! Perhaps it may be helpful to broadcast that policy proactively to make it clear what is/isn't supported.

We will broadcast it through next version's changelog :+1:. I'd say that's enough, we haven't got any issues opened about it since last update of the policy and now that we've settled on following Ruby, I think everything is more clear for the community.

I've done some digging and we're still seeing upwards of 10k+ update jobs for bundler v1 come thru each month, spread out across 1100+ orgs. However, this is less than 4% of all bundler updates. We'll have to evaluate the options for what we can do to minimize the negative impact to end users if we were to cease support.

If I recall correctly, last time I checked, maybe ~1 year ago or less, it was around 15% of jobs. I think things are going in the right direction :)

It sounds like Bundler 2 can be used for the Bundler 1 jobs? I realize some will undoubtedly break, but it might be a small enough percentage that we'd be okay with it... I suppose we could try feature flagging and see what happens.

That was exactly my plan! Bundler 2 was not really that breaking and Bundler 1.x lockfiles should still work just fine with Bundler 2. We'd need some hack to force Bundler 2 to be run, since by default Bundler 1 will run for lockfiles with "BUNDLED WITH" using Bundler 1, but other than that I think things should just work.

deivid-rodriguez avatar May 09 '24 09:05 deivid-rodriguez

I actually thought this exception was Bundler 1 specific, and no longer happening in Bundler v2. If happening with the latest version of Bundler, then it seems more worth looking into it upstream. Is there a public repo using Bundler v2 that reproduces the issue?

👋 @deivid-rodriguez I've observed that this is happening on bradfeehan/treehouse (public repo). In the cases I've seen, the common configuration is that gems are being vendored 🙂

landongrindheim avatar May 13 '24 16:05 landongrindheim

With the info shared by @landongrindheim I managed to reproduce this using only Bundler and it seems like a recent regression. I will investigate more and keep you posted.

deivid-rodriguez avatar May 14 '24 13:05 deivid-rodriguez

I created an upstream fix for this at https://github.com/rubygems/rubygems/pull/7659.

deivid-rodriguez avatar May 14 '24 18:05 deivid-rodriguez

We will broadcast it through next version's changelog 👍.

@deivid-rodriguez - could you let us know when that version is cut / and the changelog is published?

jonjanego avatar May 14 '24 18:05 jonjanego

Sure, I'll try publish it this week.

deivid-rodriguez avatar May 14 '24 18:05 deivid-rodriguez

Closing this PR as it has become obsolete in-front of below PR: https://github.com/dependabot/dependabot-core/pull/9807

honeyankit avatar May 24 '24 22:05 honeyankit

Police

GIgako19929 avatar Sep 13 '24 16:09 GIgako19929