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

Remove Bundler as a dependency #1255

Conversation

Muhammad-awaisMurtaza
Copy link
Contributor

@Muhammad-awaisMurtaza Muhammad-awaisMurtaza commented Mar 6, 2024

Motivation

We are going to use Svix for our webhooks and we have a Gemfile with bundler 2.4.21 and a ruby version 2.7.8.
But due to bundler (>= 2.2.10) in svix/ruby/gemfile.lock our build is failing on Heroku because >= tries to get the latest version of bundler which is currently 2.5.6 that requires ruby version 3.0 (which we don't have in our repo). So basically we need to restrict the bundler from updating to the latest version.

Solution

There are two options:

  • We can include the bundler dependency but with the ~> sign, not the >= (so that it will not jump to the latest version).
  • As we do not have any need to add bundler as a dependency we can remove it too.

I have implemented the 2nd one in my PR.

@Muhammad-awaisMurtaza Muhammad-awaisMurtaza requested a review from a team as a code owner March 6, 2024 09:53
@tasn
Copy link
Member

tasn commented Mar 6, 2024

Why did you close it?

@smizell
Copy link

smizell commented Mar 6, 2024

@tasn Joining in here :) This is Stephen from AnyRoad. We chatted a few weeks ago.

We're currently working on getting the Ruby client into our code. We had planned on starting a fork/PR in our own repo, but we opened this one here instead by mistake. The issue for us is that the >= requirement causes Heroku to try to use the latest version of Bundler installed, which doesn't work with our build. What we were going to propose is removing it as a dependency.

I'd be happy to open an issue to explain more or we can dig into getting this PR to a good state. What would be the best path forward?

@tasn
Copy link
Member

tasn commented Mar 6, 2024

Hey Stephen, thanks for the context, and good to hear from you again!

We are happy to fix whatever is suboptimal in the libs. I'm not sure about the exact specifics of Ruby, but just based on the name Bundler doesn't sound like it should be a dependency, it should only be a dev dependency, right? Maybe just open a PR to remove the dep? You tell me what's the right thing to do in Ruby-land. :)

@smizell
Copy link

smizell commented Mar 6, 2024

Great! This would let us do some testing in our deployed environment. Everything works great for us until we deploy, but Heroku's buildpack is set up in a way that causes the issue to come up.

In chatting with another dev here, it doesn't look like Bundler is needed as a dependency for this library, even as a dev dependency, so you could remove it altogether from the svix.gemspec file. Another fix would be to change the >= to >~ so it doesn't try to pick the latest version possible if you want to keep it as a dev dependency. In our opinion, we think the first option seems like the best for you all right now.

If this is a quick fix for you all, that would be great since you are familiar with your build and publish flows. But we're also happy to dig in and see about updating it! We'll look into opening a PR unless I hear from here in the mean time that you're updating it.

Thanks for the quick response!

@tasn
Copy link
Member

tasn commented Mar 6, 2024

We would be happy for y'all to contribute a fix just removing it from the gemspec! LMK once you open a PR, or let me know if you prefer us to do it. Either works, but I prefer you got the credit! :)

@svix-jplatte
Copy link
Member

svix-jplatte commented Mar 6, 2024

If removing it is indeed the correct solutoin, we can just reopen this PR ;)

@svix-jplatte svix-jplatte reopened this Mar 6, 2024
@svix-jplatte svix-jplatte changed the title Removed Bundler as a dependency Remove Bundler as a dependency Mar 6, 2024
@svix-jplatte svix-jplatte marked this pull request as ready for review March 6, 2024 15:18
@smizell
Copy link

smizell commented Mar 6, 2024

I think Bundler will also need to be removed from the svix.gemspec file.

@tasn
Copy link
Member

tasn commented Mar 6, 2024

While we are at it, do we need rake and rspec? Feels like more of the same?

@svix-jplatte
Copy link
Member

Well, we do have a Rakefile and now that I look at it.. It does a require "bundler/gem_tasks" as line 1. Though it's only one extra line after that, so maybe the whole file is useless?

@smizell
Copy link

smizell commented Mar 6, 2024

I think you want to keep Rake and RSpec as dev dependencies.

@smizell
Copy link

smizell commented Mar 6, 2024

And I think you need the Rakefile in order to build and publish your gem (based what I see in your GitHub Action workflow).

@smizell
Copy link

smizell commented Mar 6, 2024

@tasn @svix-jplatte Is there anything else you need here from us for this PR? Thanks for looking into this so quickly!

Copy link
Member

@svix-jplatte svix-jplatte left a comment

Choose a reason for hiding this comment

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

No further steps needed.

@svix-jplatte svix-jplatte merged commit 25cf751 into svix:main Mar 7, 2024
3 checks passed
@smizell
Copy link

smizell commented Mar 7, 2024

Amazing! Thanks so much. Do you have any thoughts on when you'll publish a new version to RubyGems that includes this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants