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

Proposal to allow defining different lockfile name not based on Gemfile name #3238

Open
jacobbednarz opened this issue Nov 7, 2018 · 22 comments

Comments

@jacobbednarz
Copy link

We use Bundler on a quite a large Rails installation and one of the pain points we're looking to eliminate is that of major updates. I won't bore you with the details however we essentially are working to the point where we can run multiple versions of Rails in CI and various parts of our infrastructure using a single Gemfile. The Gemfile looks like this:

if ENV['RAILS_NEXT'] == '1'
  gem 'rails', github: 'rails/rails', branch: 'master'
else
  gem 'rails', '~> 5.2'
end

During our CI/deployment populate the RAILS_NEXT with the respective value and it will run the app or test suite using the expected gem versions. The issue with this is that bundler expects the lock file to be the name of the Gemfile appended with .lock. This is problematic for us as we would like this to be dynamic based on some level of environment input and to work around this, we have a vendored bundler version that patches this method to allow the lock file name to be appended differently.

# from lib/bundler/shared_helpers.rb

def default_lockfile
  gemfile = default_gemfile

  case gemfile.basename.to_s
  when "gems.rb" then Pathname.new(gemfile.sub(/.rb$/, ".locked"))
  else
    if ENV['RAILS_NEXT'] == '1'
      Pathname.new("#{gemfile}_next.lock")
    else
      Pathname.new("#{gemfile}.lock")
    end
  end.untaint
end

This patch is quite specific to us and the way we intend to run our systems however I wanted to open a discussion (and perhaps a code proposal) to allow an environment variable (similar to BUNDLE_GEMFILE) whereby we could define what the lock file name could be. I'd propose an environment variable BUNDLE_GEMFILE_LOCKFILE to be made accessible and (my example) usage would end up being:

RAILS_NEXT=1 BUNDLE_GEMFILE_LOCKFILE=Gemfile_next.lock bundle ...

This would achieve using a single Gemfile but dynamically outputting the dependencies to a separate lockfile which could later be referenced for installation/vendoring/etc.

We would default this environment variable to the existing Gemfile.lock (or gems.lock when it's ready for release).

cc @rafaelfranca @eileencodes as from previous conversations both Shopify and GitHub also have custom implementations of this.

@colby-swandale
Copy link
Member

colby-swandale commented Nov 7, 2018

This seems like a good feature to have. I think the only thing that we need to be aware of is loading the correct Bundler version works given the Gemfile.lock with the env var.

@eileencodes
Copy link
Contributor

Yes we do this at GitHub and it's been a huge pain for us. In order to do it correctly for all our systems we had to vendor and hack bundler. I'd love for it to be a supported feature so we don't need to do that anymore.

@deivid-rodriguez
Copy link
Member

I normally use two different gemfiles for this kind of situations (well, three actually), and then set the BUNDLE_GEMFILE to pick the one I want to use.

Gemfile

gem 'rails', '~> 5.2'

eval_gemfile "Gemfile.common"

Gemfile.next

gem 'rails', github: 'rails/rails', branch: 'master'

eval_gemfile "Gemfile.common"

@rafaelfranca
Copy link
Contributor

I experimented with David's approach for a while. While it is great that bundler already supports this way of having two different set of gems while sharing some other gems it gets very annoying when you have a gem that used to be in the Gemfile.common but you need to override it. You have to remember to remove from the .common and duplicate in each Gemfile otherwise you either get a warning or use the wrong versions because the same gem is defined twice.

But the proposed solution is also not ideal. Instead of one environment variable you have now to set two. This can cause problem when you forgot to set one of them and you will be booting with a different lock that will conflict with the gems required by the Gemfile.

The ideal solution in my opinion would have the approach pointed by David, that only require one option being passed, but with gems being allowed to be redefined without errors or warnings. I understand allowing redefining could introduce problems in applications so I think that is not something we can change easily.

@jacobbednarz
Copy link
Author

jacobbednarz commented Nov 7, 2018

I normally use two different gemfiles for this kind of situations (well, three actually), and then set the BUNDLE_GEMFILE to pick the one I want to use.

We initially seen this approach as the best course since it was already supported by Bundler however similar to @rafaelfranca, it becomes an issue when you need to decide if a gem is common or needs a specific override. It also made debugging a little more difficult since you needed to remember which Gemfile and setup is was using. While this seems like a non-issue, the size and age of our application lends itself to quite a bit of complexity and sometimes it's difficult to ascertain direct vs implied dependencies. By maintaining a single Gemfile, it's one less hoop to jump through.

But the proposed solution is also not ideal. Instead of one environment variable you have now to set two. This can cause problem when you forgot to set one of them and you will be booting with a different lock that will conflict with the gems required by the Gemfile.

I agree here and while it's not perfect, it will allow us (and a few other big Rails apps) to get back onto the mainline of Bundler without managing our own patched versions which I think is a big win in itself. We mitigate the forgotten environment variable data by populating it across the board automatically in CI and locally a centralised bundle script that defines these based on whether or not you tell it that you want to run the "next" version. Not sure if that works for all though.

The ideal solution in my opinion would have the approach pointed by David, that only require one option being passed, but with gems being allowed to be redefined without errors or warnings. I understand allowing redefining could introduce problems in applications so I think that is not something we can change easily.

I do like this idea but as you suggest, this might not be as straight forward as it seems and could introduce some hidden gotchas.

Overall, I don't think this is something that will impact alot of people (could be wrong though!) so I think the risk here is pretty minimal and the people that will be using this are using far jankier approaches to make it work so they will be happy it's less to maintain.

@eileencodes
Copy link
Contributor

The other thing is that we're assuming that we only ever have 2 lock files. In many cases we (GitHub) have 3. When we were doing the Rails upgrade we would have the main lock file Gemfile.lock, the one for the version that we'd recently gotten green Gemfile_rails51.lock and the one for the version we were going to Gemfile_rails52.lock. To prevent regressions once a build was green we'd swap it out. So the 4.0 gemfile/build became 4.1 while we worked on 4.2, when 4.2 was green we deleted 4.1 and added 5.0. This was so we could keep working on the next version and keep regressions from being introduced. Now that we're on 5.2.1 we have just the Gemfile.lock and Gemfile_next.lock for Rails upgrades. However we're doing a new project that requires us swapping out a major gem that is not rails so we have a third gemfile for that project. For that reason, I don't think we should limit this feature to just Gemfile.lock and Gemfile.next. There should be a simple way to swap out your gemfile regardless of the reason you're doing it.

@deivid-rodriguez
Copy link
Member

I understand that there are little annoyances with the eval_gemfile approach, such as:

  • Remembering to remove duplications when promoting gems from the common set of gems to the final gemfiles.
  • Having to search on two or more files when looking for how the dependency on a specific gem is defined.
  • Having to tweak your editor to recognize files such as Gemfile.common or Gemfile.next as ruby code.

The bundler CLI has gotten many improvements lately for automatically editing gemfiles. Probably an some alias combination of bundle add and bundle remove (setting the proper target gemfile for each of them) could do the trick for the first problem :)

@jacobbednarz
Copy link
Author

Until there is better handling of multiple gem definitions which don't result in warnings or errors, eval_gemfile isn't a viable option for us. It would require tooling to support that workflow in which case we may as well stick with our existing vendored version that handles it without any extra tooling.

There should be a simple way to swap out your gemfile regardless of the reason you're doing it.

@eileencodes just to confirm on this comment, would my proposal of the two environment variables (existing BUNDLE_GEMFILE and proposed BUNDLE_GEMFILE_LOCK) suffice? If I understand you correctly it should be just want to make sure I'm not missing anything here.

@rafaelfranca
Copy link
Contributor

Would not both solutions require tooling?

Yours would requiring tooling to support two environment variables and changes to bundler.

The common Gemfile requires tooling to support BUNDLE_GEMFILE environment only (that most of the CI systems already support, this is how most of the gems run their tests with many different versions of rails in travis-ci) and bundler and commands like bundle exec already support out of box.

Not saying that the common Gemfile is the ideal solution, but in my opinion is better than two distinct env vars that introduce the risk of one of them not matching the other.

@deivid-rodriguez
Copy link
Member

I fully agree with @rafaelfranca.

Regarding how to "silence" warnings or errors, I was thinking of a config setting where the last duplicated gem dependency would overwrite any previous definition, instead of erroring (if the duplicated definitions don't match) or warn (if the definitions match). I'm just not sure if the little annoyances of the eval_gemfile approach warrant the introduction of such a feature.

@jacobbednarz
Copy link
Author

Yours would requiring tooling to support two environment variables and changes to bundler.
...
Not saying that the common Gemfile is the ideal solution, but in my opinion is better than two distinct env vars that introduce the risk of one of them not matching the other.

Our setup is hidden behind a single environment flag, RAILS_NEXT and our scripts follow a pattern similar to GitHub's scripts to rule them all whereby we create a single entry point for most operations (we don't use things like bundle exec directly generally). Using this pattern, we only need to define a single environment variable and the rest is taken care of without too much thought using checks like this:

if [[ "${RAILS_NEXT}" = "1" ]]; then
  BUNDLE_GEMFILE_LOCKFILE="Gemfile_next.lock" bundle install
else
  bundle install
fi

Which minimises the risk of mismatching. This applies to setup, running the server, CI, etc. The joy of this is that it's very low friction for developers of all levels and streamlines these processes where people may want to try the new version of a gem but don't quite know the ins and outs of applications and their setups.

Using multiple Gemfiles and relying on eval_gemfile is going to make understanding the dependencies and how they fit together a bit more difficult when it's not a straight forward scenario such as one or two Gemfiles which @eileencodes mentioned is a case for GitHub at the moment and I can foresee at least two gems we'll also need to do this with as it will be a long running upgrade.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Nov 10, 2018

You could do the same using multiple gemfiles with

if [[ "${RAILS_NEXT}" = "1" ]]; then
  BUNDLE_GEMFILE="Gemfile_next" bundle install
else
  bundle install
fi

I'm not sure I fully understand the additional complexity of the more complicated scenarios you need to support, but intuitively both approaches seem to have a similar level of complexity to me. For example,

Gemfile

gem 'rails', '~> 5.2'
gem 'devise', '~> 4.0'

eval_gemfile "Gemfile.common"

Gemfile.edge

gem 'rails', github: 'rails/rails', branch: 'master'
gem 'devise', github: 'plataformatec/devise', branch: 'master'

eval_gemfile "Gemfile.common"

Gemfile.next_rails

gem 'rails', github: 'rails/rails', branch: 'master'
gem 'devise', '~> 4.0'

eval_gemfile "Gemfile.common"

Gemfile.next_devise

gem 'rails', '~> 5.2'
gem 'devise', github: 'plataformatec/devise', branch: 'master'

eval_gemfile "Gemfile.common"

That's 4 different gemfiles, but with your single gemfile approach you would need six different environment variables: RAILS_NEXT, DEVISE_NEXT, plus 4 more (one for each lock file).

@jacobbednarz
Copy link
Author

I've been thinking about this the last couple of days and I realised we got so caught up discussing the eval_gemfile approach and pushing that as the solution that we've missed a couple of things that lead to why I initially wanted a separate lock file name.

  • It doesn't solve the requirement of having a different lock file. We compile our gems and bundle at deploy time and sync those source files to a number of places for reuse. Without a custom lock file name, we're going to be consistently replacing the same file (in this case Gemfile.lock) with different versions of our Gemfile due to it coming out with the same name.
  • Related to the first point, we checksum our .ruby-version, Gemfile and Gemfile.lock to form a unique hash that our assets are bundled with in CI and it only recomputes them if the hash doesn't match (gem or ruby version change). We don't currently evaluate the files, just sha256 checksum so the eval_gemfile call will never happen which will result in the files not changing as intending.

For this, I still think adding the ability to output the lock file to a desired location is the path I think we should pursue here.

@rafaelfranca
Copy link
Contributor

I think I'm missing something. The eval_gemfile does generate a different lockfile for each gemfile. If you run bundle install it will generate a Gemfile.lock. If you run BUNDLE_GEMFILE="Gemfile_next" bundle install it will generate a Gemfile_next.lock file.

That being said I think it satisfy the first buttle point.

For the second point is it not possible to add the Gemfile.common to your checksum calculation? That way, there is no need to execute the eval_gemfile

@jacobbednarz
Copy link
Author

I think I'm missing something. The eval_gemfile does generate a different lockfile for each gemfile. If you run bundle install it will generate a Gemfile.lock. If you run BUNDLE_GEMFILE="Gemfile_next" bundle install it will generate a Gemfile_next.lock file.

No, you haven't missed anything. I've still just got a patched version locally which skews this a little 🤦‍♂️ Thanks for confirming that!

For the second point is it not possible to add the Gemfile.common to your checksum calculation? That way, there is no need to execute the eval_gemfile

The calculation would need to know which one it was building otherwise one Gemfile change could impact the others and cause extra compilation steps that won't be needed. An option here would be to export the BUNDLE_GEMFILE as a part of the CI environment and ensure that is persisted somewhere to build the filename? I.e. ${BUNDLE_GEMFILE}.lock 🤔

Have we got any further ideas on the declaration of multiple gems and how that would be handled?

@jacobbednarz
Copy link
Author

Regarding how to "silence" warnings or errors, I was thinking of a config setting where the last duplicated gem dependency would overwrite any previous definition, instead of erroring (if the duplicated definitions don't match) or warn (if the definitions match).

This is interesting but I think it would also be beneficial to have it the other way around too. You define your overrides upfront instead of taking on the latest defined. It would allow you to have stable fallbacks but overrides for newer/unstable builds too.

@deivid-rodriguez
Copy link
Member

I might be also misunderstanding something but I'd say the ${BUNDLE_GEMFILE}.lock file should contain all the information you need to get a checksum for caching. I'd say including either ${BUNDLE_GEMFILE} or Gemfile.common would be redundant?

@jacobbednarz
Copy link
Author

@deivid-rodriguez I'm in the process of trying to update our build/CI infrastructure relying on BUNDLE_GEMFILE now and will see how it goes. It's a bit of pain to swap over but it might be enough to get us off the custom fork for now. I'm still not entirely sold on the debuggability/dep ordering of the eval_gemfile stuff though after having a minor run in with it this afternoon...

@Edouard-chin
Copy link

Edouard-chin commented Nov 20, 2018

As a FYI, many of our apps at Shopify needs to be dualbooted, we needed a solution to make things easier for our developers so we recently built bootboot
This bundler plugin is meant to remove the overhead of dualbooting your app when you go with the "Gemfile Gemfile.lock Gemfile_next.lock" approach as well as making sure that both lockfiles are always in sync whenever someone adds/remove/update a gem.

Sadly, the dualbooting part currently can't work when bundler is frozen (which is most likely the case in your production environment) because of what I think might™ be an issue on bundler rubygems/bundler#6795 We can hopefully try to fix it soon.

In the meantime bootboot can still be usefull if you want to ensure that both your lockfiles are always in sync

@jacobbednarz
Copy link
Author

Thanks @Edouard-chin, appreciate it! Will definitely check it out!

@jacobbednarz
Copy link
Author

Big hat tip to @Edouard-chin and the folks at Shopify for bootboot, it's been working flawlessly since we jumped onto it and it's taken care of the keeping things in sync issue. 🍻

@Edouard-chin
Copy link

Thanks @jacobbednarz , very happy that it helps you ! Also happy new year :)

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

No branches or pull requests

7 participants