New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monkey patch to add method default_gem? to Bundler LazySpecification #9660
base: main
Are you sure you want to change the base?
Conversation
Thanks for getting this fix in! Please is there a way to get test coverage for this? |
Just want to reiterate that I think we should consider this a temporary solution. |
9945c8d
to
7677564
Compare
@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 |
Hmm. The same test is getting passed in my local environment bundler 1 but failing in CI pointing towards some environment mismatch.
|
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. |
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. |
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? |
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 |
And usage is actually significant cc: @jonjanego |
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 |
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. |
Agreed! I proposed a clarification of the policy at rubygems/rubygems#7633.
Yeah, same as before 😞. 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 😄) 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 #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 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. |
https://github.com/rubygems/rubygems/blob/master/bundler/doc/POLICIES.md?plain=1#L16:
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. |
We will broadcast it through next version's changelog 👍. 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.
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 :)
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 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 🙂 |
4542ec0
to
9f08ced
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks like it will do the job for now, I would love to see some more information on where the method is called from, it's somewhat surprising we are hitting this path but it doesn't seem to be a widespread issue in Bundler, or people would have likely complained?
I was also wondering if we should be trying to fix/patch the calling code rather than adding back the method, but I know this is blocking some other work and I don't see any practical issues with this approach, so 👍
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. |
I created an upstream fix for this at rubygems/rubygems#7659. |
@deivid-rodriguez - could you let us know when that version is cut / and the changelog is published? |
Sure, I'll try publish it this week. |
Co-authored-by: Bryan Dragon <25506+bdragon@users.noreply.github.com>
9f08ced
to
aecaa41
Compare
Context
Added method
default_gem?
toBundler::LazySpecification
to fix the following issue: