-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix GEM_PATH regression in 1.13. #4992
Fix GEM_PATH regression in 1.13. #4992
Conversation
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.
looks good, but we definitely need test coverage for this
@@ -81,6 +82,38 @@ def kernel_load(file, *args) | |||
abort "#{e.class}: #{e.message}\n #{backtrace.join("\n ")}" | |||
end | |||
|
|||
# This is pretty damned esoteric. Since the dawn of Bundler, when |
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.
please remove the damned
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.
Yes, sorry - it was late at night.
# the weirdness right here, with this method and its appropriate | ||
# 27-to-1 ratio of doc lines to code lines. | ||
def fixup_gem_path_env | ||
ENV['GEM_PATH'] = nil if Bundler.settings[:disable_shared_gems] |
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.
double quotes
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.
Yeah, I forgot to run rubocop, though I knew this wasn't done yet - no tests.
yeah, tests for the bundle exec, and tests for the disable_shared_gems putting all gems into the --path, which said tests would have prevented this change in the first place. |
@chrismo awesomeeeeee thank you for tracking this down 🙇 I'm very excited to get this fixed once we have tests. :) |
5b19c05
to
8f3dd8f
Compare
The comments were a bit rambly and late-night stream of consciousness. Re-wrote them to try and be clearer (and committed the bug unfixed at the moment whilest I still work out tests). Still need to research |
# this needs to be empty string to cause | ||
# PathSupport.split_gem_path to only load up the | ||
# Bundler --path setting as the GEM_PATH. | ||
env["GEM_PATH"] = '' |
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.
double quotes
This isn't working. I still can't get any spec within the bundler codebase itself to reliably recreate the problem, because it's a nested bundle exec that is the original problem ... and that's just a bit too bizarre for my brain to figure out. The spec_helper calls
placing the local exe/bundle in the PATH, thwarting any attempt to recreate a nested bundle exec accurately which IRL needs to be hitting the Rubygems generated bin entry:
So. I at least created this bundler-case based on the original #4592 bug report. This accurately shows the nested bundle exec problem in 1.12.5 and the fix in 1.13.1. But the code HERE in this PR (#4992) - while it fixes #4974, it regresses the nested bundle exec fix (#4592). The reason is also due to the nature of the 1.12.5 change to use Kernel.load (same process) instead of Kernel.exec (a new process). There's so much global state (and a lot of it memoized) that's setup in So so. IMO, #4974 is a much worse problem than #4592. #4974 keeps I'd rather release a 1.13.2 with #4974 fixed and intentionally regress #4592 until we can figure out how to properly support the nested |
proactive code review - I'm being a little sloppy on this PR, I'll rebase and remove the version change that snuck in and everything before for realz. Just pushing for comment on the substantive parts. |
def configure_gem_path_override | ||
if Bundler.settings[:disable_shared_gems] | ||
# TODO MODO hate this spooky at a distance global state voodoo | ||
ENV["DO_NOT_LIMIT_GEM_PATH"] = "true" |
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.
any reason this couldn't just check if ENV["GEM_PATH"].nil?
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.
this configure_gem_path_override
method is now called before require "bundler/setup"
... prior commit had me attempting to undo something that was done inside bundler/setup after it was called ... had to change the approach since it was too late at that point.
bundle :install, :system_bundler => true, :path => "vendor/bundler" | ||
end | ||
|
||
it "works with disable_shared_gems" do |
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.
needs a more descriptive name
Samuel contributed this patch to simulate a system bundler.
0482075
to
d08c778
Compare
Ok, I think this ready, presuming CI passes. |
@segiddins all your requested changes appear to be addressed in the final commit, even though they're still shown in the PR... is that normal? this seems sub-optimal. :/ |
dunno - prior versions of github, before the code review stuff, used to collapse comments on old commits. i'm not used to the new review flow ... maybe @segiddins needs to "sign off" on his comments in some way? |
yeah, I guess so. well, I'm happy with this, so @homu r+. |
shoot. i also built a bundler-cases for this - and it's still failing. argh. looking ... |
heh. well, in that case... @homu r- |
📌 Commit d08c778 has been approved by |
…ismo Fix GEM_PATH regression in 1.13. This is a bit esoteric. There's two separate pieces to understand first. (1) When `:disable_shared_gems` is true, the `GEM_PATH` gets initialized to an empty string. This is done to make sure it's expanded to ONLY the Bundler --path setting, otherwise it expands to include the system path. (2) Prior to 1.12, the code here in bundle exec didn't use Kernel.load and didn't `require "bundler/setup"`, which meant the path to gems was never involved. In 1.12, bundle exec was overhauled for various reasons to use Kernel.load, and `require "bundler/setup"` is now invoked, which created a bug. In cases like `--deployment` where `disable_shared_gems` is true, Bundler couldn't find itself, because Bundler never lives in the `--path` but only in system gems. We fixed this (the bundle exec bug) in 1.13.0 by changing GEM_PATH to be initialized to nil instead of empty string in all cases. But it created another bug. We've reverted the change so that GEM_PATH is now back to being initialized, but still need to patch up GEM_PATH right here to allow bundle exec to continue to work. Fix #4974 - [x] Needs tests
✌️ @chrismo can now approve this pull request |
💔 Test failed - status |
654f80b
to
db83b3c
Compare
tl;dr `--deployment` will copy _all_ gems to the `--path` now and a proper fix is in place for nested bundle execs when `--path` is set and RubyGems >=2.6.2 is being used. Fixes rubygems#4974. There's two problems blended in here. Let's trace the events here from the beginning, shall we? First off, from probably the first versions of Bundler, when `disable_shared_gems` is true, the `GEM_PATH` gets internally overridden and initialized to an empty string. This is done to make sure that later in the `Bundler.setup` process it's expanded to ONLY the Bundler `--path` setting, otherwise it expands to include the system path. In 1.12, `bundle exec` was changed to use `Kernel.load` in some cases, and that new code path calls `require "bundler/setup"`. Later, issue rubygems#4592 was filed, showing that in cases like `--deployment` where `disable_shared_gems` is true, Bundler couldn't find itself, because Bundler never lives in the `--path` but only in system gems. And as it would later be discovered, was not a problem with RubyGems 2.6.1, but was a problem with >=2.6.2 (and older RubyGems it would seem, though those weren't as thoroughly investigated). We fixed rubygems#4592 (see PR rubygems#4701) in 1.13.0 by changing the oooold code initializing `GEM_PATH` to be initialized to `nil` instead of empty string in all `disable_shared_gems` cases. But it created another bug, filed as rubygems#4974, wherein system gems were no longer copied to the `--path` when `disable_shared_gems` is true. In this commit here (rubygems#4992) we've reverted the change so that `GEM_PATH` is now back to being initialized to an empty string instead of `nil`. That fixes rubygems#4974, but regresses rubygems#4592, and we needed a new way to fix it. After a few tortured attempts at this, I ran across issue rubygems#4602, a similar report of nested bundle execs being broken, rubygems#4602 itself an extension of rubygems#4381 reporting the same problem. It brought to light the role the Rubygems version played in the problem. When the `bundler` gem is installed and the wrapper is generated for any gem executables, the contents of this wrapper are determined by the Rubygems version. Up until RubyGems 2.6.1 the last line of this wrapper calls `Gem.bin_path`. Bundler replaces the Rubygems implementation of `Gem.bin_path` with its own, and has for a long time made a special exception for the Bundler gem itself, short-circuiting with the contents of a special ENV variable called `BUNDLE_BIN_PATH`. In Rubygems 2.6.2, `bin_path` was superseded by a new `Gem.activate_bin_path` method which did what `bin_path` did but also activated the gem. Bundler 1.13 added support for this, but it didn't include the same short-circuit for bundler itself. (Alert user @taoza even noticed this here rubygems@fcaab35#r56665282). This commit also includes that short circuit for Rubygems >=2.6.2 now and nested bundle exec should continue to work. Thx to @miros for filing rubygems#4974 and isolating the problem in 1.12, @segiddins for many contributions and better ideas, and everyone else for their patience :)
bah, keep forgetting that one test I'd added that's still failing. fixing, new commit shortly |
tl;dr `--deployment` will copy _all_ gems to the `--path` now and a proper fix is in place for nested bundle execs when `--path` is set and RubyGems >=2.6.2 is being used. Fixes rubygems#4974. There's two problems blended in here. Let's trace the events here from the beginning, shall we? First off, from probably the first versions of Bundler, when `disable_shared_gems` is true, the `GEM_PATH` gets internally overridden and initialized to an empty string. This is done to make sure that later in the `Bundler.setup` process it's expanded to ONLY the Bundler `--path` setting, otherwise it expands to include the system gems path. In 1.12, `bundle exec` was changed to use `Kernel.load` in some cases, and that new code path calls `require "bundler/setup"`. Later, issue rubygems#4592 was filed, showing that in cases like `--deployment` where `disable_shared_gems` is true, Bundler couldn't find itself, because Bundler never lives in the `--path` but only in system gems. And as it would later be discovered, was not a problem with RubyGems 2.6.1, but was a problem with >=2.6.2 (and older RubyGems it would seem, though those weren't as thoroughly investigated). We fixed rubygems#4592 (see PR rubygems#4701) in 1.13.0 by changing the oooold code initializing `GEM_PATH` to be initialized to `nil` instead of empty string in all `disable_shared_gems` cases. But it created another bug, filed as rubygems#4974, wherein system gems were no longer copied to the `--path` when `disable_shared_gems` was true. In this commit here (rubygems#4992) we've reverted the change so that `GEM_PATH` is now back to being initialized to an empty string instead of `nil`. That fixes rubygems#4974, but regresses rubygems#4592, and we needed a new way to fix it. After a few tortured attempts at this, I ran across issue rubygems#4602, a similar report of nested bundle execs being broken, rubygems#4602 itself an extension of rubygems#4381 reporting the same problem. It brought to light the role the Rubygems version played in the problem. When the `bundler` gem is installed and the wrapper is generated for any gem executables, the contents of this wrapper are determined by the Rubygems version. Up until RubyGems 2.6.1 the last line of this wrapper calls `Gem.bin_path`. Bundler replaces the Rubygems implementation of `Gem.bin_path` with its own, and has for a long time made a special exception for the Bundler gem itself, short-circuiting with the contents of a special ENV variable called `BUNDLE_BIN_PATH`. In Rubygems 2.6.2, `bin_path` was superseded by a new `Gem.activate_bin_path` method which did what `bin_path` did but also activated the gem. Bundler 1.13 added support for this, but it didn't include the same short-circuit for bundler itself. (Alert user @taoza even noticed this here rubygems@fcaab35#r56665282). This commit also includes that short circuit for Rubygems >=2.6.2 now and nested bundle exec should continue to work. Thx to @miros for filing rubygems#4974 and isolating the problem in 1.12, @segiddins for many contributions and better ideas, and everyone else for their bug reports and patience :)
db83b3c
to
4ab674f
Compare
@homu retry |
@homu r+ |
📌 Commit 4ab674f has been approved by |
@homu retry |
…ismo Fix GEM_PATH regression in 1.13. _nth time is a charm ... here's the latest latest latest commit message_: Fix disable_shared_gems bug & keep nested exec tl;dr `--deployment` will copy _all_ gems to the `--path` now and a proper fix is in place for nested bundle execs when `--path` is set and RubyGems >=2.6.2 is being used. Fixes #4974. There's two problems blended in here. Let's trace the events here from the beginning, shall we? First off, from probably the first versions of Bundler, when `disable_shared_gems` is true, the `GEM_PATH` gets internally overridden and initialized to an empty string. This is done to make sure that later in the `Bundler.setup` process it's expanded to ONLY the Bundler `--path` setting, otherwise it expands to include the system path. In 1.12, `bundle exec` was changed to use `Kernel.load` in some cases, and that new code path calls `require "bundler/setup"`. Later, issue #4592 was filed, showing that in cases like `--deployment` where `disable_shared_gems` is true, Bundler couldn't find itself, because Bundler never lives in the `--path` but only in system gems. And as it would later be discovered, was not a problem with RubyGems 2.6.1, but was a problem with >=2.6.2 (and older RubyGems it would seem, though those weren't as thoroughly investigated). We fixed #4592 (see PR #4701) in 1.13.0 by changing the oooold code initializing `GEM_PATH` to be initialized to `nil` instead of empty string in all `disable_shared_gems` cases. But it created another bug, filed as #4974, wherein system gems were no longer copied to the `--path` when `disable_shared_gems` is true. In this commit here (#4992) we've reverted the change so that `GEM_PATH` is now back to being initialized to an empty string instead of `nil`. That fixes #4974, but regresses #4592, and we needed a new way to fix it. After a few tortured attempts at this, I ran across issue #4602, a similar report of nested bundle execs being broken, #4602 itself an extension of #4381 reporting the same problem. It brought to light the role the Rubygems version played in the problem. When the `bundler` gem is installed and the wrapper is generated for any gem executables, the contents of this wrapper are determined by the Rubygems version. Up until RubyGems 2.6.1 the last line of this wrapper calls `Gem.bin_path`. Bundler replaces the Rubygems implementation of `Gem.bin_path` with its own, and has for a long time made a special exception for the Bundler gem itself, short-circuiting with the contents of a special ENV variable called `BUNDLE_BIN_PATH`. In Rubygems 2.6.2, `bin_path` was superseded by a new `Gem.activate_bin_path` method which did what `bin_path` did but also activated the gem. Bundler 1.13 added support for this, but it didn't include the same short-circuit for bundler itself. (Alert user @taoza even noticed this here fcaab35#r56665282). This commit also includes that short circuit for Rubygems >=2.6.2 now and nested bundle exec should continue to work.
☀️ Test successful - status |
whew. and don't come back :) |
…ismo Fix GEM_PATH regression in 1.13. _nth time is a charm ... here's the latest latest latest commit message_: Fix disable_shared_gems bug & keep nested exec tl;dr `--deployment` will copy _all_ gems to the `--path` now and a proper fix is in place for nested bundle execs when `--path` is set and RubyGems >=2.6.2 is being used. Fixes #4974. There's two problems blended in here. Let's trace the events here from the beginning, shall we? First off, from probably the first versions of Bundler, when `disable_shared_gems` is true, the `GEM_PATH` gets internally overridden and initialized to an empty string. This is done to make sure that later in the `Bundler.setup` process it's expanded to ONLY the Bundler `--path` setting, otherwise it expands to include the system path. In 1.12, `bundle exec` was changed to use `Kernel.load` in some cases, and that new code path calls `require "bundler/setup"`. Later, issue #4592 was filed, showing that in cases like `--deployment` where `disable_shared_gems` is true, Bundler couldn't find itself, because Bundler never lives in the `--path` but only in system gems. And as it would later be discovered, was not a problem with RubyGems 2.6.1, but was a problem with >=2.6.2 (and older RubyGems it would seem, though those weren't as thoroughly investigated). We fixed #4592 (see PR #4701) in 1.13.0 by changing the oooold code initializing `GEM_PATH` to be initialized to `nil` instead of empty string in all `disable_shared_gems` cases. But it created another bug, filed as #4974, wherein system gems were no longer copied to the `--path` when `disable_shared_gems` is true. In this commit here (#4992) we've reverted the change so that `GEM_PATH` is now back to being initialized to an empty string instead of `nil`. That fixes #4974, but regresses #4592, and we needed a new way to fix it. After a few tortured attempts at this, I ran across issue #4602, a similar report of nested bundle execs being broken, #4602 itself an extension of #4381 reporting the same problem. It brought to light the role the Rubygems version played in the problem. When the `bundler` gem is installed and the wrapper is generated for any gem executables, the contents of this wrapper are determined by the Rubygems version. Up until RubyGems 2.6.1 the last line of this wrapper calls `Gem.bin_path`. Bundler replaces the Rubygems implementation of `Gem.bin_path` with its own, and has for a long time made a special exception for the Bundler gem itself, short-circuiting with the contents of a special ENV variable called `BUNDLE_BIN_PATH`. In Rubygems 2.6.2, `bin_path` was superseded by a new `Gem.activate_bin_path` method which did what `bin_path` did but also activated the gem. Bundler 1.13 added support for this, but it didn't include the same short-circuit for bundler itself. (Alert user @taoza even noticed this here fcaab35#r56665282). This commit also includes that short circuit for Rubygems >=2.6.2 now and nested bundle exec should continue to work.
* 'master' of github.com:bundler/bundler: (163 commits) remove always-failing 1.8.7 build Rename to force_ruby_platform Fail fast in the specs when running from a path with special characters Change changelog on rubygems#4974 Version 1.13.2 with changelog Auto merge of rubygems#4990 - bundler:seg-realworld-flex, r=segiddins Auto merge of rubygems#4922 - JuanitoFatas:fix/4914-gemfile-engine-symbol-and-string, r=segiddins Auto merge of rubygems#4983 - bundler:seg-redefine-method-visibility, r=indirect Auto merge of rubygems#4994 - bundler:seg-definition-init-perf, r=segiddins Auto merge of rubygems#4971 - bundler:seg-pare-metadata-error, r=indirect Auto merge of rubygems#4998 - srbaker:patch-1, r=segiddins Auto merge of rubygems#4968 - bundler:seg-exec-load-docs, r=indirect Auto merge of rubygems#5002 - alepore:fix-colors, r=indirect Auto merge of rubygems#5015 - m1k3:fix-settings-no-config, r=segiddins Auto merge of rubygems#4992 - chrismo:fix_disable_shared_gems_gem_path, r=chrismo Auto merge of rubygems#5008 - bundler:seg-lazy-specification-materialize-platform, r=indirect Auto merge of rubygems#5014 - bundler:seg-auto-install-bool-key, r=indirect Auto merge of rubygems#4928 - bundler:seg-update-compact-index, r=segiddins Improve IRB behavior in generated bin/console Add a spec for only_ruby_platform ...
nth time is a charm ... here's the latest latest latest commit message:
Fix disable_shared_gems bug & keep nested exec
tl;dr
--deployment
will copy all gems to the--path
now and aproper fix is in place for nested bundle execs when
--path
is set andRubyGems >=2.6.2 is being used.
Fixes #4974.
There's two problems blended in here. Let's trace the events here from
the beginning, shall we?
First off, from probably the first versions of Bundler, when
disable_shared_gems
is true, theGEM_PATH
gets internally overriddenand initialized to an empty string. This is done to make sure that later
in the
Bundler.setup
process it's expanded to ONLY the Bundler--path
setting, otherwise it expands to include the system path.
In 1.12,
bundle exec
was changed to useKernel.load
in some cases,and that new code path calls
require "bundler/setup"
.Later, issue #4592 was filed, showing that in cases like
--deployment
where
disable_shared_gems
is true, Bundler couldn't find itself,because Bundler never lives in the
--path
but only in system gems. Andas it would later be discovered, was not a problem with RubyGems 2.6.1,
but was a problem with >=2.6.2 (and older RubyGems it would seem, though
those weren't as thoroughly investigated).
We fixed #4592 (see PR #4701) in 1.13.0 by changing the oooold code
initializing
GEM_PATH
to be initialized tonil
instead of emptystring in all
disable_shared_gems
cases. But it created another bug,filed as #4974, wherein system gems were no longer copied to the
--path
whendisable_shared_gems
is true. In this commit here (#4992)we've reverted the change so that
GEM_PATH
is now back to beinginitialized to an empty string instead of
nil
.That fixes #4974, but regresses #4592, and we needed a new way to fix
it.
After a few tortured attempts at this, I ran across issue #4602, a
similar report of nested bundle execs being broken, #4602 itself an
extension of #4381 reporting the same problem. It brought to light the
role the Rubygems version played in the problem.
When the
bundler
gem is installed and the wrapper is generated for anygem executables, the contents of this wrapper are determined by the
Rubygems version. Up until RubyGems 2.6.1 the last line of this wrapper
calls
Gem.bin_path
. Bundler replaces the Rubygems implementation ofGem.bin_path
with its own, and has for a long time made a specialexception for the Bundler gem itself, short-circuiting with the contents
of a special ENV variable called
BUNDLE_BIN_PATH
.In Rubygems 2.6.2,
bin_path
was superseded by a newGem.activate_bin_path
method which did whatbin_path
did but alsoactivated the gem. Bundler 1.13 added support for this, but it didn't
include the same short-circuit for bundler itself. (Alert user @taoza
even noticed this here
fcaab35#r56665282).
This commit also includes that short circuit for Rubygems >=2.6.2 now
and nested bundle exec should continue to work.