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

RFC: Bundler Version Locking #29

Merged
merged 7 commits into from
Jan 24, 2022

Conversation

duckinator
Copy link
Member

@duckinator duckinator commented Sep 1, 2020

NOTE: This RFC is proposed as an alternative to the "Bundler version switcher", and assumes that is being removed first. Whether or not to remove it should be discussed elsewhere.

This RFC no longer assumes BundlerVersionFinder is removed, and there is a proof-of-concept (see #29 (comment)) implemented without removing it, but they may be redundant in some cases.

Rendered

@duckinator duckinator force-pushed the bundler-version-locking branch from 6459b08 to e8abfd7 Compare September 1, 2020 19:31
@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Sep 2, 2020

This proposal sounds neat. FWIW, adding this behaviour to the current version switcher has already been proposed and have a couple of thumbs up: rubygems/rubygems#3318. In my opinion, it makes sense and should've been there from the beginning.

Something I'd also like to see is the possibility of locking the bundler version to ensure that everybody uses the same version, even if the dependency is not explicit in the Gemfile. Something like bundle config set --local lock_bundler true, or whatever. What happens now essentially, only without the user-friendliness improvements of auto-installing and "trampolining" if the locked version is not present.

@duckinator
Copy link
Member Author

duckinator commented Sep 6, 2020

Honestly, I'm against it being a completely separate feature like that. The whole point of this proposal was to make it more user-friendly and build on existing functionality. By having it be a separate thing, it removes the ability for users to rely on prior knowledge, and feels as though it undermines a significant part of the proposal.

If the version switcher is just removed, with no alternative, you get the Bundler 1.x behavior back. You can then just put Bundler in your gem file and run bundle install && bundle install to get close to the same functionality (first pass ensures the right Bundler version, the second pass runs the whole install with the right version). Why make it more complex than necessary?

Also: wouldn't making it a config variable require committing the Bundler config file? iirc, that's the same place credentials go right now, isn't it? (I vaguely recall an issue about that.)

@deivid-rodriguez
Copy link
Member

Absolutely, I reread your proposal and essentially already covers this. We should probably clarify the behavior in presence of a lockfile (even if it's consistent with the current behavior, but probably doens't hurt anyways). If I understand your proposal correctly, it's this:

  • If there's no gem "bundler", "<requirement>" in the Gemfile, then the latest version on the user's system gets run, regardless of presence of a lockfile.
  • If there's gem "bundler", "<requirement>" in the Gemfile, then:
    • The latest available version of bundler matching the requirement is used if there's no lockfile.
    • The locked version of bundler is used if there's a lockfile.

Would that be it?

@duckinator
Copy link
Member Author

@deivid-rodriguez yes, exactly.

@simi
Copy link
Member

simi commented Sep 9, 2020

👍 This sounds really great! 👍 The only problem I see in here is how to safely switch in between bundler versions. I'm not sure initially proposed Kernel.exec("bundle", "_#{required_version}_", *args) is safe enough.

🤔 In theory bundler version could be duplicated in lockfile since BUNDLED WITH will be still present. But it is possible to use open constraint like gem bundler '~> 2.0' and BUNDLED WITH will have locked exact version like 2.0.2 thus it is not duplicated for those cases and it is still useful to keep both constraints in lockfile. 👍

@duckinator
Copy link
Member Author

duckinator commented Sep 20, 2020

@simi yeah, I'm not fully sold on the Kernel.exec("bundle", "_#{required_version}_", *args), it was meant more as an example of what end-user behavior is expected. I'm not sure how the implementation should be approached.

I still need to go through and revise this a bit based on this discussion; I'll try to get to that soon.

@duckinator
Copy link
Member Author

Okay, took a lot longer than I'd have liked, but I'm finally getting back to this today.

Proof of concept

Assuming this script is saved as ./bundle-bootstrap:

#!/usr/bin/env ruby

require 'bundler'

bundler_dep = Bundler.load.dependencies.find { |d| d.name == 'bundler' }
if bundler_dep
  result = Gem.install(bundler_dep).find { |g| g.name == 'bundler' }
  Kernel.exec("bundle", "_#{result.version}_", *ARGV)
end

# If integrated into Bundler, this next line would be removed and we'd just continue as normal.
Kernel.exec("bundle", *ARGV)

Then you can do this:

$ ls -a
./  ../  bundle-bootstrap  Gemfile
$ cat Gemfile 
source "https://rubygems.org"

gem 'bundler', '~> 2.0.0'
gem 'json'
$ bundle --version
Bundler version 2.1.4
$ ./bundle-bootstrap install
Fetching bundler-2.0.2.gem
Fetching gem metadata from https://rubygems.org/.
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using bundler 2.0.2
Fetching json 2.3.1
Installing json 2.3.1 with native extensions
Bundle complete! 2 Gemfile dependencies, 2 gems now installed.
Bundled gems are installed into `./.bundle/gems`
$ bundle --version
Bundler version 2.0.2
$

Safety of Kernel.exec

I'm unsure if Kernel.exec() is safe enough. If it's not, I'm also not sure what to use instead (still looking into that.)

It has some pretty substantial weirdness in Ruby — C has a variety of of exec*() functions that could get around this, but Ruby seems to present multiple variants through Kernel.exec based on some kind of magic.

Kernel.exec and Process.exec both seem to wind up in the C function rb_f_exec(), which handles things that are run in the shell and things that are not. See https://github.com/ruby/ruby/blob/0829f1470469e2c8b74df97d44514b49f4c5f39e/process.c#L2936-L2966

I know we don't want to run things through the shell, and I'm unsure about using the PATH. So I'm not sure what to use instead.

@duckinator
Copy link
Member Author

After a quick romp through the C code, I've confirmed that as long as you provide at least one argument and avoid the first variant described in the docs, Ruby will never use the shell to run things passed to Kernel.exec.It seems if there's no arguments passed, it will use the shell, no matter what.

@simi given that^, what're your thoughts on Kernel.exec? We'd always be passing at least one argument (the _#{version}_), so we can guarantee it won't use shell execution. Are there other aspects of Kernel.exec you're concerned about?

@duckinator
Copy link
Member Author

I've cleaned everything up a bit, and removed the assumption that BundlerVersionFinder is removed — it turns out the approach I came up with can coexist with it. They may be redundant in some cases, however. I'm not sure.

@deivid-rodriguez @simi could you take another look at this when you have some time?

@duckinator
Copy link
Member Author

I made a rough implementation of it here: rubygems/rubygems#4076

Still needs tests and such, though.

@simi
Copy link
Member

simi commented Nov 29, 2020

@simi given that^, what're your thoughts on Kernel.exec? We'd always be passing at least one argument (the _#{version}_), so we can guarantee it won't use shell execution. Are there other aspects of Kernel.exec you're concerned about?

If I understand it well, this should automate the following scenario:

  1. run bundle install with wrong version
  2. get informed wrong bundler version is used
  3. run bundle _LOCKED_VERSION_ install
  4. all good

I think the key in here is to run both bundle commands within same environment (same shell, env vars, ...).

But see this case:

MY_ENV=a ruby -e 'ENV["MY_ENV"]="b";Kernel.exec("ruby", "-e", "print ENV[\"MY_ENV\"]")'
b

It is not running in the original environment (with changed env var in here). Could that be potential security problem?

Also there is hardcoded bundle command, but it is not required anywhere to keep this called bundle. One of the usecases of using different name is mentioned at https://github.com/rubygems/rubygems/blob/6bc9889e9555731d5e12329ceaa2680c7228e07d/bundler/doc/development/SETUP.md#bundler-development-setup.

Those are my initial thoughts.

🤔 Wouldn't be possible to introduce bundle binstub being able to require proper bundler version initially to avoid any need for version switching after bundler load?

@duckinator
Copy link
Member Author

@simi yes, that is the workflow it automates, but there's some subtle differences — mainly that it installs bundler first and avoids installing anything besides Bundler using the wrong Bundler version).

I'm unsure if the changed environment has security implications. Will need to look into that more. Good catch. 👍

The bundle command is hard-coded in what I wrote, but I suspect there's a way to reliably get the name used for the bundle executable (possibly $0?). I'd need to look into that.

I think the binstub should be able to handle everything. If it can, that's probably the better approach, since I it would avoid both problems you mentioned.

@duckinator
Copy link
Member Author

duckinator commented Nov 30, 2020

I made a crude proof-of-concept using a modified version of the bundler binstub I have on my system. The way it reads the Gemfile is absolutely not the correct approach, but I think it proves that it's theoretically possible to use the binstub to do what would be needed for this RFC.

Here's the modified binstub:

#!/usr/bin/ruby
#
# This file was generated by RubyGems.
#
# The application 'bundler' is installed as part of a gem, and
# this file is here to facilitate running it.
#

require 'rubygems'

version = ">= 0.a"

str = ARGV.first
if str
  str = str.b[/\A_(.*)_\z/, 1]
end

if str and Gem::Version.correct?(str)
  version = str
  ARGV.shift
else
  # TODO: Use RubyGems to find this information, not this trash fire.
  class KludgeBucket
    attr_accessor :bundler_version

    def initialize
      eval open('Gemfile').read
    end

    def gem(name, version=nil, *)
      if name == 'bundler' && !version.nil?
        @bundler_version = version
      end
    end

    def method_missing(*); end
  end
  bundler_version = KludgeBucket.new.bundler_version

  if bundler_version
    # TODO: Figure out how to avoid needing to manually specify user_install here.
    #       It's in my ~/.gemrc, but it doesn't work here, unsurprisingly.
    result = Gem.install("bundler", bundler_version, {user_install: true}).find { |g| g.name == 'bundler' }
    version = "#{bundler_version}"
  end
end

if Gem.respond_to?(:activate_bin_path)
load Gem.activate_bin_path('bundler', 'bundle', version)
else
gem "bundler", version
load Gem.bin_path("bundler", "bundle", version)
end

And here's an example of using it (where that code is saved as ./xbundle):

$ bundle --version
Bundler version 2.1.4
$ cat Gemfile
source "https://rubygems.org"

gem 'bundler', '~> 2.0.0'
gem 'json'
$ ./xbundle install
Fetching bundler-2.0.2.gem
Fetching gem metadata from https://rubygems.org/.
Using bundler 2.0.2
Fetching json 2.3.1
Installing json 2.3.1 with native extensions
Bundle complete! 2 Gemfile dependencies, 2 gems now installed.
Bundled gems are installed into `./.bundle/gems`
$

(EDIT: Realized I was using a modified version of Bundler; uninstalled that and used 2.1.4 to confirm the code from rubygems/rubygems#4076 wasn't the thing making it work.)

…ementation suggestions since it's clear more discussion needs to happen.
@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Dec 11, 2020

Hi! I was planning to come back to this after the rubygems 3.2.0 + bundler 2.2.0 release, so here I am.

First some thoughts after the release. I broke some things for people. It was unintentional, but it happened :(. It turns out bundler is very subject to breaking people's workflows when we make changes. I'm sure previous maintainers will agree with me and I can confirm it. Sometimes we simply fail to evaluate how breaking the changes we make will be, and other times we choose to call some potentially breaking things "bug fixes" in favour of distributing fixed behaviour in exchange for breaking some edge cases relying on the wrong behaviour, because we decide that it's very unlikely that those edge cases happen.

So given that, I believe the best way to ensure that bundler keeps working for a given application is to make sure that all developers and environments of the application use the exact same code, bit by bit that already worked once. That means always choosing the BUNDLED WITH version in the lockfile, no matter there's a gem "bundler" constraint in the Gemfile or not.

So that would be my suggestion to modify this RFC.

Regarding the implementation, the earliest we can do this, the better of course. So binstub seems like a great place. However, I'm not sure people will like that we silently install stuff under the hood when running things like bundle exec. So maybe it's better to restrict this to bundle install only (maybe bundle update, and bundle lock too, but bundle install seems good for an initial version), and delay a bit the moment where we Kernel.exec to the appropriate version?

@duckinator
Copy link
Member Author

@deivid-rodriguez my understanding is that your suggestion is basically: If BUNDLED WITH is specified in Gemfile.lock, use that value; otherwise, look for a gem "bundler" constraint. Is that correct?

Assuming my understanding of your suggestion is correct, the problem I have is this: from my perspective, putting gem "bundler", some_version_constraint into your Gemfile is the way you opt-in to this feature. Doing that is explicitly requesting a specific version constraint. Having it prefer the automatically-generated BUNDLED WITH value unconditionally is incredibly confusing to me, and effectively undermines the entire point of this RFC. The whole thing that prompted this RFC was that BUNDLED WITH, as it works now, feels way too magical and there's no real point in a separate thing.

If people want to specify it in their Gemfile and want an exact Bundler version (as opposed to a range), they can just pin it there.

The way that makes the most sense to me, and takes into account the BUNDLED WITH behavior while allowing gem "bundler" to take priority, is something along the lines of:

  1. Determine which version to use:
    a. If using the bundle _<VERSION>_ ... invocation, use that version. Go to step 2. (Or 4? Unsure which is better.)
    b. If there is no gem "bundler" constraint and no BUNDLED WITH version, go to step 5.
    c. If BUNDLED WITH is available and there is no gem "bundler" constraint, print a message suggesting pinning the Bundler version in the Gemfile (so it's explicitly under user control), then use that value. Go to step 2.
    d. If there is a gem "bundler" constraint, use that value. Go to step 2.
  2. If running bundle install, install the version found in step 1.
  3. If running any command except bundle install and the version found in step 1 is not available, print a warning.
  4. Switch to the version found in step 1, if it's available.
  5. Continue as normal.

I think this strikes a good balance of keeping backwards-compatibility while adding new functionality and opening the window to removing the reliance on BUNDLED WITH in a future major version bump.

What're your thoughts, @deivid-rodriguez?

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Dec 14, 2020

Yeah, I now strongly believe that BUNDLED WITH should be the version preferred for all bundler commands (if available), and the version installed by bundle install.

I also felt somewhat strongly about this before, but I decided to give in since other people disagreed. However, after the last bundler release I feel even more strongly about it.

The point of the lockfile is a having a way to lock your dependencies, all of them, even if not explicitly specified in the Gemfile. And bundler is one dependency after all, so it should be locked just like the rest.

As per why locking dependencies, it's about determinism. I don't want my collegue and I banging our heads against the wall because a command is doing something for me and something different for my collegue when we are apparently using the same code. Turns out we're not because I was using bundler 2.2.0 and my collegue bundler 2.1.4. I don't want CI of people starting to fail because we released bundler 2.2 with a bug.

When we released bundler 2.2, we got this issue: "Bundler is not respecting the lockfile". And they were right (it was due to using an old rubygems version, current rubygems uses the exact version in the BUNDLED WITH if available). If I commit my lock file to source control, I expect my dependencies to be locked, so I disagree with the statement

It feels too magical, because the user isn't actually asking for it.

In my opinion, the PR you created is a very good idea (if changed to install the BUNDLED WITH version) because it brings usability to the current version switcher, because users can get the proper version available just by running bundle install (which they are already running).

@duckinator
Copy link
Member Author

duckinator commented Dec 14, 2020

Okay, talked to @deivid-rodriguez and there's been a lot of miscommunication/confusion on the bundler version switcher in general, but I think we've sorted out what the expectations are. The key moment in that discussion for me was when it became clear that some of the behavior I was originally bothered by was a bug, not a feature.

The conclusion we came to is to do something along the lines of:

  1. If there's a lockfile with BUNDLED WITH, install it if needed and re-exec.
  2. Resolve dependencies.
  3. If the resolved bundler version doesn't match the running one, install it if needed and re-exec.

I think my PR (rubygems/rubygems#4076) handles steps 2-3, but still needs some refinement and to implement step 1.

I'm gonna call it a day for now, but it feels like we're making good progress on this. ^.^

@duckinator
Copy link
Member Author

duckinator commented Jan 16, 2021

I'm currently rewriting my RFC, and will want feedback when I'm finished with that.

However, to revisit this:

If I commit my lock file to source control, I expect my dependencies to be locked, so I disagree with the statement

It feels too magical, because the user isn't actually asking for it.

I think this boils down to the concept of implicit vs explicit dependencies. The things in your Gemfile are explicit dependencies, but "the version of Bundler I used at the time I ran this command" is an implicit dependency.

My general interpretation of this is that explicit dependencies should override implicit dependencies, but there's some nuance I missed initially:

If they conflict, the explicit dependency should override the constraint. If they overlap, the implicit dependency should refine the constraint.

Turns out that's exactly what the updated idea in my last comment does.

@duckinator duckinator force-pushed the bundler-version-locking branch 3 times, most recently from ef12137 to 3b8ac25 Compare January 16, 2021 05:45

There are many times where locking your Bundler version is useful.
The existence of `BundlerVersionFinder` shows that, but the initial attempt
caused innumerable problems. This is an attempt to resolve those problems.
Copy link
Member

Choose a reason for hiding this comment

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

Let's expand on the motivation section. Some draft with ideas from my side:

Bundler is used by many people in many different scenarios. Experience has proven that releasing changes in bundler can result if breaking behaviour for people for whom the current version is working just fine. Even if no backwards incompatibiltiy was intended.

These problems could be avoided by making sure the dependency on bundler for people is locked just like all the other dependencies. After all, one of the main purposes of bundler is to lock your dependencies so that all developers and environments of an application run the exact same code, thus avoiding "works on my machine" issues, and breakage because a third party dependency was released.

This proposal expands this philosophy to the bundler dependency itself. Bundler already records the bundler version that generated a given lockfile in the lockfile itself, so by always running that version we make sure that developers always get a deterministic working version when working on a project.

Developers can still upgrade bundler, and we will of course encourage them to do so, but we don't want everyone to silently and automatically be updated just because we released a new version. This is also a quality of life improvement for the bundler maintainers team, because we won't get a lot of issues at the same time right after we release, but instead issues will be reported on a more distributed manner as people choose to upgrade. This will reduce our chances to get burnt out and overwhelmed with work.

@deivid-rodriguez
Copy link
Member

And honestly, the proposal does not provide any real world scenario, where this is useful. The "Motivation" section is very vague.

Agreed. I made a comment with some suggestion to expand that section.

@voxik
Copy link

voxik commented Jan 18, 2021

We tend to remove usage of Bundler,

How does this proposal change that?

Some test suites are using Bundler in really elaborated way, that makes it hard to remove the Bundler. Now there will be not just hard to remove Bundler dependency, but this allows to depend on quirks of some specific version of Bundler.

We tend to remove version restrictions and hence we work with upstreams to expand the version matrix and fix incompatibilities.

You're still free to do that, right? I'm guessing if you see a lockfile you will remove it,right? In that case, this proposal changes noting for you.

This is not just about lockfile, many times it is actually "easier" to recreate the Gemfile altogether just to remove all possible dependencies of some specific versions. This adds one additional worry.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 18, 2021

Thanks for explaining your concerns.

It seems clear to me that while developing bundler we shouldn't focus on making it easier to remove bundler. I am convinced that this is a good move, and unless I get a lot of push back from other maintainers, this will happen. In principle, your concerns doesn't look too bad to me, but obviously you know best what will cause issues for you, so I apologize for creating some overhead for you. Hopefully all the other times when I tried to make your work easier can compensate for this one 😅.

@voxik
Copy link

voxik commented Jan 18, 2021

Could you please consider to provide a way to disable this feature? Of course I'd prefer something like operating_system.rb, where the feature could be disabled once and is kept close to the code. Of course Env variable is probably second best option. But setting Env variable systemwide is not the best way to go.

@duckinator
Copy link
Member Author

@voxik I'm not opposed to adding a way to disable this. FYI, however, my RubyGems/Bundler work is pretty sporadic because I'm trying to find a house right now. I probably won't get back to this RFC until at least January 25th, if not sometime February.

@zofrex
Copy link

zofrex commented Jun 8, 2021

David pointed me at this thread as this proposal seems like it would solve some challenges I'm facing. Perhaps my needs/wants could help flesh out the motivation section? Something like:


Motivations: Security

Scanning

It is not currently possible to determine from the source code of a project which version of Bundler it will run.

This is in contrast to every other Ruby dependency – every gem a project depends on has the exact version captured in the Gemfile.lock file, and the Ruby version is recorded both in the Gemfile and often a .ruby-version file.

This makes it possible to quickly and easily 'scan' a project for security issues with their dependencies. Tools like bundler-audit (and many, many others) can warn developers if any of their dependencies have known security issues, and doing so is so easy that Github can do this at scale. This makes it very easy to alert developers of issues with their dependencies so they update them faster, shrinking the window during which they are vulnerable to exploitation.

All of their dependencies, that is, except Bundler. bundler-audit does not, currently, check the Bundler version – and if you read that discussion you can see the complexity of attempting to do so.

Bundler has security vulnerabilities, just like any gem, and it can sometimes be extremely important to ensure that a vulnerable version is not in use. For example, multiple bugs have occurred over the years that affect dependency resolution from private vs public sources. Any organisation using private repositories would find it very useful to be able to statically check that none of their repositories are using versions of Bundler vulnerable to dependency confusion attacks – currently the only way to check what version of Bundler is being used is to run it.

Guaranteeing that the version of Bundler specified in the lockfile is the version that will be run makes it possible to statically assess projects' Bundler versions, bringing it in-line with all the other gems in use.

Locking Bundler version

Ensuring the specified version of Bundler is used enables the static scanning use-case detailed above, but it is also a useful feature directly, as well.

If a vulnerability is found in Bundler and a patch is released, it's important to make sure everyone and everything is using that latest version.

For build machines, production systems, and other systems that are controlled and defined by code this is already possible, but having this functionality built-in to Bundler would simplify the process.

Currently this could be achieved by, for example, adding an explicit "gem install bundler:x.y.z" in a build script. With the Bundler version controlled by the lockfile, updating Bundler becomes the same as updating any other gem, no changes to build scripts (pipeline definitions, Dockerfile, etc) required.

It would also mean that developers would immediately be using the updated version too, which is something not so easily achievable today. While production machines are of course the most important to secure, a successful dependency confusion attack resulting in remote code execution on a developer's machine is still an undesirable scenario.

With the Bundler version controlled by the lockfile, developers would be moved on to the patched version of Bundler as soon as they pull down the updated lockfile.


I hope this is useful. If it is, please feel free to copy/paste all or part of it as is useful, or re-jig it if you prefer.

@duckinator
Copy link
Member Author

I probably won't get back to this RFC until at least January 25th, if not sometime February.

Okay maybe it took a bit longer. 😂

Added a lot more to the Motivation section, based on what @deivid-rodriguez and @zofrex said previously!

@duckinator
Copy link
Member Author

duckinator commented Jul 16, 2021

And honestly, the proposal does not provide any real world scenario, where this is useful. The "Motivation" section is very vague.

I'll address this one first, since it's separate from the rest:

I initially had very limited time I could set aside for this, so I focused on fleshing out the implementation details and getting to a proof-of-concept as quickly as I could.

Hopefully the expanded Motivation section provides more context.


Honestly, it seems that this will make life of Fedora packagers harder then it used to be. We tend to remove usage of Bundler, we tend to remove version restrictions and hence we work with upstreams to expand the version matrix and fix incompatibilities.

Now there will be not just hard to remove Bundler dependency, but this allows to depend on quirks of some specific version of Bundler.

This is not just about lockfile, many times it is actually "easier" to recreate the Gemfile altogether just to remove all possible dependencies of some specific versions. This adds one additional worry.

This all seems to assume that codebases relying on quirks of specific Bundler versions is a problem this will create, but I'm rather confused by that implication since part of the motivation for this RFC is that very problem already existing in the wild.

@ojab
Copy link

ojab commented Aug 24, 2021

And honestly, the proposal does not provide any real world scenario, where this is useful. The "Motivation" section is very vague.

Separate gem sources broke bundler-2.1 a little rubygems/rubygems#4381 (comment). If production runs bundler-2.1 and developer do bundle with bundler-2.2.25 (or any version with auto-update to separate sources) — it would be broken.

Locking bundler version would prevent that.

@duckinator
Copy link
Member Author

@simi it looks like Bundler.original_exec(...) should address the problem you mentioned about the environment:

https://github.com/rubygems/rubygems/blob/9c88db949d7480ebea4cae29dfad1a5ad6db8cd1/bundler/lib/bundler.rb#L422-L425

@duckinator
Copy link
Member Author

$ MY_ENV=a ruby -e 'ENV["MY_ENV"]="b";Kernel.exec("ruby", "-e", "puts ENV[\"MY_ENV\"]")'
b
$ MY_ENV=a ruby -e 'require "bundler";ENV["MY_ENV"]="b";Bundler.original_exec("ruby", "-e", "puts ENV[\"MY_ENV\"]")'
a

@eregon
Copy link

eregon commented Dec 15, 2021

To clarify, the Bundler version that might be installed on the fly will be installed like any other gem in the Gemfile.lock, right?
What's important is that bundler caching still works with this, so when there is a cached vendor/bundle we can already find that bundler version in there.

@deivid-rodriguez
Copy link
Member

No, the idea is that the version installed on the fly will be installed alongside the running bundler version. As of today, bundler is always installed globally, it never gets installed to vendor/bundle, even if it's a direct dependency in the Gemfile. I don't think it makes sense to install to vendor/bundle because by the time bundler configures vendor/bundle to be the path to look for gems, bundler is obviously already running, so the copy in vendor/bundle can never be actually used.

By the way, I just noticed yesterday that the current PR might actually be doing what you suggest, but if that's the case it's unintended 😅, I'll verify that and fix it.

@deivid-rodriguez
Copy link
Member

Caching is based off the Gemfile.lock file, right? This doesn't modify the lockfile, so I don't think it should affect catching?

@deivid-rodriguez
Copy link
Member

The "caching" for this feature would be: if we are running the exact bundler version if the BUNDLED WITH section, it means that version is already installed and running, so we can go ahead and skip everything.

@eregon
Copy link

eregon commented Dec 15, 2021

The problem is then e.g. for ruby/setup-ruby (and other CIs) we'll end up downloading that specific bundler version every time, even if the cache already exists.
I'd guess this will lead to "too many requests" to rubygems.org for downloading bundler so many times, and it can never be cached.

Also there is the case where the default gem home is not writable, but yet bundle install with a path (e.g. vendor/bundle) should work just fine, without requiring to write to the global gem home.

I don't think it makes sense to install to vendor/bundle because by the time bundler configures vendor/bundle to be the path to look for gems, bundler is obviously already running, so the copy in vendor/bundle can never be actually used.

I'd think it's not a problem, we'll have some other version of Bundler running initially anyway and then execve() to the right one, after it is installed. So the initial bundler can and likely already does look at the bundler path, should use that to find all available bundler versions, and if it's already there should just use it.

@eregon
Copy link

eregon commented Dec 15, 2021

To make that last sentence clearer, in https://github.com/duckinator/rfcs/blob/bundler-version-locking/text/0000-bundler-version-locking.md#example-1
From the beginning until (... rest of output from installing Bundler 2.0.2 ...) I'd think the initial Bundler (2.1.4) is running, and from (or just after) Using bundler 2.0.2 then Bundler 2.0.2 is running.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Dec 15, 2021

The problem is then e.g. for ruby/setup-ruby (and other CIs) we'll end up downloading that specific bundler version every time, even if the cache already exists.
I'd guess this will lead to "too many requests" to rubygems.org for downloading bundler so many times, and it can never be cached.

For just one single gem install? Any uncached bundle install does many many more requests that this and it has always worked fine in GitHub Actions, just slower. You're even also installing bundler manually in ruby/setup-ruby when it's not a default gem and it hasn't caused issues, right? https://github.com/ruby/setup-ruby/blob/a6f22865941e122a37e097fbded3dd0b54c39207/bundler.js#L94.

Also there is the case where the default gem home is not writable, but yet bundle install with a path (e.g. vendor/bundle) should work just fine, without requiring to write to the global gem home.

Sure, this will be handled smoothly.

I'd think it's not a problem, we'll have some other version of Bundler running initially anyway and then execve() to the right one, after it is installed. So the initial bundler can and likely already does look at the bundler path, should use that to find all available bundler versions, and if it's already there should just use it.

This is a good point, actually! I think the current implementation is doing just that and although unintended in the first place, it seems indeed much better! I will verify this.

@eregon
Copy link

eregon commented Dec 15, 2021

You're correct that for non-head Rubies which ship with Bundler 2, setup-ruby currently does gem install bundler -v "~> 2" (or the version in BUNDLED WITH if there is one), cache or not.
I do remember issues with too many requests but that was for the new RubyGems.org API and those have been fixed.
I forgot the exact reason why latest Bundler 2 is always installed instead of just trying the one shipped with Ruby (which was how it was done initially IIRC) but I think it was basically the easiest way to deal with bugs in older Bundler versions.

I think conceptually it's nice to treat bundler "like any other gem in the Gemfile/Gemfile.lock", except that of course if the initial bundler doesn't have the right version we should install only that bundler, exec to it and install the rest of the gems with that bundler.

@deivid-rodriguez
Copy link
Member

I forgot the exact reason why latest Bundler 2 is always installed instead of just trying the one shipped with Ruby (which was how it was done initially IIRC) but I think it was basically the easiest way to deal with bugs in older Bundler versions.

Oh right, I misread that code, you actually do this in more cases than I thought, which confirms it shouldn't be an issue even if we went with the initial approach.

I'll still have a look at your suggestion since it seems nice anyways!

@deivid-rodriguez
Copy link
Member

Actually, I think you implemented at some point in ruby/setup-ruby the exact logic I'm implementing here: ruby/setup-ruby#123. So once we ship this feature (even in its current version of installing always globally), I think you should be able to gradually remove all that code.

@eregon
Copy link

eregon commented Dec 15, 2021

Yes, once this land we wouldn't install bundler manually anymore in setup-ruby for Rubies which ship with a recent enough RubyGems/Bundler (which has this functionality).
Not sure regarding -head versions but probably worth trying to just use the same logic for those.
(Older Rubies would still need to e.g. run with Bundler 1)

@duckinator
Copy link
Member Author

As a note: the functionality for this wound up getting merged before the RFC. (Whoops.)

See rubygems/rubygems#4076.

Not sure how to handle this RFC now, honestly.

@indirect
Copy link
Member

We should just merge it. :)

@indirect indirect merged commit 6a85878 into rubygems:master Jan 24, 2022
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.

8 participants