-
-
Notifications
You must be signed in to change notification settings - Fork 2k
[RubygemsIntegration] Add support for new activate_bin_path binstubs #4341
Conversation
(caused by rubygems/rubygems#1527) |
93e1b7c
to
fcaab35
Compare
@indirect r? |
@homu r+ |
📌 Commit fcaab35 has been approved by |
⚡ Test exempted - status |
[RubygemsIntegration] Add support for new activate_bin_path binstubs This ought to fix the build! \c @indirect
|
||
redefine_method(gem_class, :bin_path) do |name, *args| | ||
exec_name = args.first | ||
return ENV["BUNDLE_BIN_PATH"] if exec_name == "bundle" |
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 like the new Gem.activate_bin_path
will be missing out the chance to hit this early return and breaks this feature on bundle-exec's man page:
make sure that it's still possible to shell out to bundle from inside a command invoked by bundle exec (using $BUNDLE_BIN_PATH)
Not sure what would be the best way to fix it :-/
/cc @segiddins @indirect
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 like you were right :) We're about to make this change on this PR #4992. I ran across this while researching a write up of a fresh commit message. It's been a long and winding road to re-discover what you'd found in March.
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.
@chrismo I'm feeling a bit guilty now for not being able to help fixing the issue a few months back.. But it was the first time I was reporting an issue with Rubygems and Bundler. There was a tendency to think I was missing something in the code and not fully understand the details. But thanks so much for the work on #4992. Looking forward to the release!
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.
No worries, there are a lot of moving parts (as I continue to learn) and it can be a tough decision to know what matters and what will just be fine. At least you recorded this comment which helps confirm what we're seeing. :)
@taoza please open a new issue with steps to reproduce the bug you're seeing |
@segiddins Working on it :) |
This ought to fix the build!
\c @indirect