Skip to content
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

Stop running Bundler v1 #10104

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Jun 28, 2024

What are you trying to accomplish?

Stop running Bundler v1 internally, and thus dropping all Bundler v1 specific code, without dropping support for providing updates for Bundler v1 lockfiles.

Anything you want to highlight for special attention from reviewers?

To do this, I adapted native helpers to pass an explicitly requirement on the locked version of Bundler to the resolver. With that in place, the end user behavior should still be the same. Ironically, the changes caused some trouble in Bundler 2. I think it should be possible to reconcile that, but for now I only changed helpers when locked bundler version is v1, and left helpers unchanged otherwise. It seemed also a plus to make this less risky to not change anything for Bundler v2.

Also, I'm pretty sure @jurre will ask about the following so I'll explain. There's some stuff that may change in edge cases. For example, if v1 lockfile uses multiple global sources (something considered insecure), Dependabot will now start splitting the gems in the lockfile by the source they come from. I'm not sure how Bundler 1 deals with that, and since Dependabot currently does not modify the BUNDLED WITH section in the lockfile, it may cause some trouble.

I think the above is acceptable given Bundler 1 is very old, and only particular cases like that may be affected, but generally updates should still work fine!

How will you know you've accomplished your goal?

Bundler v1 users can still get Dependabot PRs, and at the same time we can get rid of all the Bundler v1 specific code.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@github-actions github-actions bot added the L: ruby:bundler RubyGems via bundler label Jun 28, 2024
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/drop-bundler-1 branch 3 times, most recently from b7b2634 to 3d94d61 Compare June 28, 2024 12:42
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review June 28, 2024 13:25
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner June 28, 2024 13:25
@deivid-rodriguez
Copy link
Contributor Author

Probably worth doing some realworld testing, but a green spec suite, both with bundler 1 and bundler 2, is reassuring!

Regarding spec changes, I only changed expectations when I considered the new behavior in Bundler 2 better. When it was not clear, I fixed things so that things still work as the work today.

@jurre
Copy link
Member

jurre commented Jun 28, 2024

Thanks David! Should we consider just dropping support for bundler v1 altogether instead?

@deivid-rodriguez
Copy link
Contributor Author

I don't know but this is ready, so shouldn't be necessary to make that decision now if you don't want to!

@jurre
Copy link
Member

jurre commented Jun 28, 2024

I'm mostly thinking if we just drop support, we don't need to worry about potential edge-cases or patch up any issues we find with how this works against real world bundler 1 projects.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jun 28, 2024

🤷‍♂️ Up to you! I'm just proposing this because the previous times dropping support was considered, the number of users was still not small enough to justify a hard drop. Since the main annoyance of supporting Bundler v1 is maintaining a doubled code base, I figured we can fix the main annoyance and let users keep gradually moving on without disruption.

@deivid-rodriguez
Copy link
Contributor Author

Maybe ship this and announce official Bundler v1 deprecation at the same time, announcing that while support is still there, internal changes my cause issues for some users that won't necessarily be addressed.

@jeffwidman jeffwidman requested a review from jurre June 28, 2024 20:37
@abdulapopoola
Copy link
Member

@amazimbe can you help deploy this?

@amazimbe amazimbe force-pushed the deivid-rodriguez/drop-bundler-1 branch from f20d337 to 7ade76e Compare July 2, 2024 16:06
@amazimbe
Copy link
Contributor

amazimbe commented Jul 2, 2024

@amazimbe can you help deploy this?

Yes, I will.

@amazimbe amazimbe force-pushed the deivid-rodriguez/drop-bundler-1 branch from 7ade76e to 510c4c7 Compare July 3, 2024 08:28
Copy link
Contributor

@thavaahariharangit thavaahariharangit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the discussion with Abdul

@amazimbe
Copy link
Contributor

amazimbe commented Jul 3, 2024

@jurre I'm not able to deploy this without your approval. Please take a look when you get a chance.

@jurre
Copy link
Member

jurre commented Jul 3, 2024

To be entirely honest, I feel a little unsure about this change, David captured my reservations well in the PR body.

I would much rather we just push to stop supporting Bundler v1 altogether, rather than have potentially broken support out there. My worry is that, yes it might work for people using a single source, but the potential support burden from people who are using multiple sources seems not worth the risk. If we're not committed to fully supporting Bundler 1, and to be candid, I am not, I'd rather we just drop support entirely and communicate that clearly to our customers.

As mentioned, Bundler v1 is very old, usage is low, the upgrade path is really good these days. Why should we put ourselves in this awkward situation?

cc @jonjanego can we sync up soon-ish and decide if we can move forward with dropping support for Bundler v1, rather than having potentially incompatible support?

If we do decide we want to move forward with this change, we should IMO at the very least test the scenario where we're using multiple sources with Bundler 1 and see how this implementation handles that, so we can update our documentation to reflect the limitation if necessary.

PS: @deivid-rodriguez I thoroughly appreciate your work on this, I absolutely see you're trying to move us to a better place, and I can see the benefits of this approach ❤️

@deivid-rodriguez
Copy link
Contributor Author

No worries @jurre, I understand being careful, and that's precisely why I proposed this as a way to unblock dropping support, since it has been postponed a few times precisely to avoid disrupting users. But I do understand that this will still be disrupting.

How about this alternative approach:

If Dependabot finds a Bundler v1 lockfile, then it ignores all dependencies, except for Bundler itself, and proposes a PR to upgrade Bundler. Once the PR is merged, users can get PRs for the other dependencies again.

@jurre
Copy link
Member

jurre commented Jul 3, 2024

Let me talk to Jon about dropping support altogether, and let's also thoroughly test this approach on some real world v1 repo's.

I noticed some v2 fixtures with multiple sources actually use the v1 format, for example this one. I'm actually not quite sure what that means for our test suite here, but either way it'd be good to double check how this actually behaves. @amazimbe is going to run through some examples in a bit.

@deivid-rodriguez
Copy link
Contributor Author

Awesome, let me know if you need help!

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jul 8, 2024

I was curious and I tried this, and I can confirm the behavior:

  • Dependabot will split sources in lockfile for security if used to upgrade a lockfile with merged sources.
  • Bundler v1 will either merge them back (if run in non frozen mode), or fail (if run in frozen mode).

Note that this is not a new issue though, it already happened for 2.x lockfiles when Dependabot migrated to the version of Bundler that started splitting sources. See for example: #4095.

I think the best course of action would be what I proposed earlier:

  • If Dependabot finds a Bundler v1 lockfile, then it ignores all dependencies, except for Bundler itself, and proposes a PR to upgrade Bundler.
  • Once that PR is merged, users can get PRs for the other dependencies again.

@amazimbe
Copy link
Contributor

I run this against a repo with :
bundler version 1.17.3
ruby 2.7.0

but it did not update the bundler version.

@deivid-rodriguez
Copy link
Contributor Author

Yes, that's correct. Currently Dependabot never updates the Bundler version, and that's what will create issues here. Dependabot will upgrade the lockfile to "v2 format" but will keep "BUNDLED WITH 1.17.3" in it. And that will cause issues in edge cases like the multiple sources situation we were discussing.

Hence my suggestion of actually adding support in Dependabot for upgrading the version of Bundler in the lockfile. Then we can completely stop creating PRs for Bundler v1 lockfiles, except for upgrading the Bundler version itself, and once users merge that PR and upgrade to Bundler v2, they get their Dependabot PRs back.

@amazimbe
Copy link
Contributor

Yes, that's correct. Currently Dependabot never updates the Bundler version, and that's what will create issues here. Dependabot will upgrade the lockfile to "v2 format" but will keep "BUNDLED WITH 1.17.3" in it. And that will cause issues in edge cases like the multiple sources situation we were discussing.

Hence my suggestion of actually adding support in Dependabot for upgrading the version of Bundler in the lockfile. Then we can completely stop creating PRs for Bundler v1 lockfiles, except for upgrading the Bundler version itself, and once users merge that PR and upgrade to Bundler v2, they get their Dependabot PRs back.

In my case the lockfile stays in v1 format

@deivid-rodriguez
Copy link
Contributor Author

In my case the lockfile stays in v1 format

So, it proposes correct updates and does not update the format? Then that sounds perfect 😅. I have to admit I tried Bundler directly but not Dependabot as in this PR, so maybe you are right and this is fine after all!

@deivid-rodriguez
Copy link
Contributor Author

Oh, but it seems that repo does not need any updates? Of course if Bundler won't create any PRs it won't change the lockfile format. I think we'd need to test on a repository that actually needs updates. I suggest downgrading some gem manually and committing the updated Gemfile/Gemfile.lock file, and then trying again.

@abdulapopoola
Copy link
Member

abdulapopoola commented Jul 11, 2024

Tagging @jurre and @deivid-rodriguez for final approvals. If there are no more questions, then @amazimbe can get it deployed.

Thanks.

@jurre
Copy link
Member

jurre commented Jul 11, 2024

@amazimbe can you share the before and after lockfiles from those updates where we can see the updated dependency but the format staying the same?

@amazimbe
Copy link
Contributor

I've had a call with @jurre regarding this PR and some of what I wrote earlier has turns out to be incorrect. To avoid confusion, I'll summarise the current state of play in this comment.

Before running this CLI against my bundler v1 repo the Gemfile.lock was like this:

GEM
  remote: https://rubygems.org/
  remote: https://xxxx:xxxx@rubygems.pkg.github.com/xxxx/
  specs:
    connection_pool (2.4.1)
    hola (0.0.0)
    io-console (0.7.2)
    irb (1.14.0)
      rdoc (>= 4.0.0)
      reline (>= 0.4.2)
    psych (5.1.2)
      stringio
    rack (3.1.6)
    rack-protection (3.0.6)
      rack
    rdoc (6.7.0)
      psych (>= 4.0.0)
    redis (5.2.0)
      redis-client (>= 0.22.0)
    redis-client (0.22.2)
      connection_pool
    reline (0.5.9)
      io-console (~> 0.5)
    sidekiq (6.0.1)
      connection_pool (>= 2.2.2)
      rack (>= 2.0.0)
      rack-protection (>= 2.0.0)
      redis (>= 4.1.0)
    stringio (3.1.1)

PLATFORMS
  ruby

DEPENDENCIES
  hola (= 0.0.0)!
  irb
  sidekiq (= 6.0.1)

BUNDLED WITH
   1.17.3

After running this CLI against my bundler v1 repo, the output contained:

GEM
    remote: https://rubygems.pkg.github.com/xxxx/
    specs:
       hola (0.0.0)

GEM
    remote: https://rubygems.org/
    specs:
    concurrent-ruby (1.3.3)
    connection_pool (2.4.1)
    io-console (0.7.2)
    irb (1.14.0)
        rdoc (>= 4.0.0)
        reline (>= 0.4.2)
    logger (1.6.0)
    psych (5.1.2)
        stringio
    rack (3.1.7)
    rdoc (6.7.0)
        psych (>= 4.0.0)
    redis-client (0.22.2)
        connection_pool
    reline (0.5.9)
        io-console (~> 0.5)
    sidekiq (7.3.0)
        concurrent-ruby (< 2)
        connection_pool (>= 2.3.0)
        logger
        rack (>= 2.2.4)
        redis-client (>= 0.22.2)
    stringio (3.1.1)

PLATFORMS
    ruby

DEPENDENCIES
    hola (= 0.0.0)!
    irb
    sidekiq (= 7.3.0)

BUNDLED WITH
    1.17.3

This is indeed in V2 format.

We then run bundle _1.17.3_ update in the v1 repo and the new Gemfile.lock reverted back to the original i.e.


GEM
  remote: https://rubygems.org/
  remote: https://xxxx:xxxx@rubygems.pkg.github.com/xxxx/
  specs:
    connection_pool (2.4.1)
    hola (0.0.0)
    io-console (0.7.2)
    irb (1.14.0)
      rdoc (>= 4.0.0)
      reline (>= 0.4.2)
    psych (5.1.2)
      stringio
    rack (3.1.7)
    rack-protection (3.0.6)
      rack
    rdoc (6.7.0)
      psych (>= 4.0.0)
    redis (5.2.0)
      redis-client (>= 0.22.0)
    redis-client (0.22.2)
      connection_pool
    reline (0.5.9)
      io-console (~> 0.5)
    sidekiq (6.0.1)
      connection_pool (>= 2.2.2)
      rack (>= 2.0.0)
      rack-protection (>= 2.0.0)
      redis (>= 4.1.0)
    stringio (3.1.1)

PLATFORMS
  ruby
  unknown

DEPENDENCIES
  hola (= 0.0.0)!
  irb
  sidekiq (= 6.0.1)

BUNDLED WITH
   1.17.3

To conclude, I believe we have 3 main options:

  1. Remove support for bundler v1 completely as bundler v1 is more than 6 years old. But as @deivid-rodriguez mentioned earlier, this has been tried before but we haven't actually done it due to the number of v1 users still being significant.
  2. As proposed by @deivid-rodriguez, make changes to Dependabot so that it only creates a PR to upgrade bundler if it encounters a bundler v1 lockfile. This would be like a forced bundler upgrade.
  3. Merge this as is and document the flip flopping when users manually run bundle update to avoid confusion. From what I've seen, this would lose the updates made by dependabot every time bundle update is run so it could be confusing and perhaps ineffective.

@amazimbe
Copy link
Contributor

On hold awaiting input from product about how we would like to proceed.

@deivid-rodriguez
Copy link
Contributor Author

No longer relevant 🎉. Thanks all :)

@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/drop-bundler-1 branch October 18, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: ruby:bundler RubyGems via bundler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants