-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Remove version overwriting hacks #7062
Remove version overwriting hacks #7062
Conversation
af4e349
to
0ce7b55
Compare
8f1b215
to
f43d751
Compare
f43d751
to
9d5c8e6
Compare
788f7f4
to
a98d8ba
Compare
Ok, so I looked a bit more into this, and it turns out that this "version hack" is only needed for rubygems versions the doesn't provide the While debugging this, I added a few other changes that are not directly related to this, so I have extracted them to separate PRs (#7136, #7137, #7138, #7139). @segiddins What do you think about this? I really don't like changing the version of a loaded gem, if we can avoid it 😬... |
49c5beb
to
76a6e63
Compare
Sorry @segiddins. This is not ready for review. A general idea of whether you think this is a good idea is appreciated though. |
37a362c
to
44c6655
Compare
@segiddins This should be ready for a review now! |
44c6655
to
b134ca0
Compare
These version overwriting hacks bit me again in #7166, but I think, at least some of them can be fixed for all supported rubygems versions, so I'm back to working on this PR 😅. I'll re-request a review when I'm ready :) |
9802616
to
8e76547
Compare
8e76547
to
7911450
Compare
I'm so happy I finally managed to keep this part of the diff diff --git a/lib/bundler/version.rb b/lib/bundler/version.rb
index cfd630095..21eeeb0fd 100644
--- a/lib/bundler/version.rb
+++ b/lib/bundler/version.rb
@@ -1,23 +1,7 @@
# frozen_string_literal: false
module Bundler
- # We're doing this because we might write tests that deal
- # with other versions of bundler and we are unsure how to
- # handle this better.
- VERSION = "2.1.0.pre.1".freeze unless defined?(::Bundler::VERSION)
-
- def self.overwrite_loaded_gem_version
- begin
- require "rubygems"
- rescue LoadError
- return
- end
- return unless bundler_spec = Gem.loaded_specs["bundler"]
- return if bundler_spec.version == VERSION
- bundler_spec.version = Bundler::VERSION
- end
- private_class_method :overwrite_loaded_gem_version
- overwrite_loaded_gem_version
+ VERSION = "2.1.0.pre.1".freeze
def self.bundler_major_version
@bundler_major_version ||= VERSION.split(".").first.to_i while also keeping CI green for all supported rubygems & rubies 🎉. The price to pay is the admittedly ugly style of using absolute paths with autoloads, but I think it's definitely worth it. We could make it prettier if autoload_relative ever lands into core. This should fix a bunch of problems with manually installed bundler versions coexisting with bundler versions as default gems 😃. As per vendored dependencies, I created CocoaPods/Molinillo#130 for I'd like others to chime in here 😃 |
Also, I think this might get green several specs that are currently skipped when running with the "ruby-core" flag. |
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 looks good to me. 👍🏻 Thanks!
Otherwise it could happen that support/hax defines Bundler::VERSION first, and then the real version file redefines it. Not at the moment due to the version overwriting hacks in our version.rb file, but those will be removed.
This way, we will never leak to a different bundler copy.
By doing this we avoid circular requires (`rubygems` requires `bundler` via `USE_GEMDEPS`, `bundler` requires `rubygems` when loading its own version), and also redefinition warnings when the `bundler.gemspec` is evaluated with `rubygems` already loaded (for example, when running `ruby setup.rb` from `rubygems` repo). But most importantly, we avoid leaking from a bundler installation to a different one, something quite common since bundler is a default gem. The previous hack meant acting like that didn't happen, but seems actually quite dangerous. We can do this due to the previous work of making all of bundler internal requires not rely on the LOAD_PATH, and thus making it impossible to leak to another copy of bundler.
7911450
to
c675b59
Compare
Thanks @indirect! @bundlerbot r+ |
7062: Remove version overwriting hacks r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that we are doing some hacks in our version file that are... dangerous. At very least they cause "circular problems" like #6927, but I also have bad feelings about it. ### What was your diagnosis of the problem? My diagnosis was we're overwriting the version of the currently loaded bundler 😬. This hack is _almost_ unnecessary (see the spec run here with the hack fully removed: https://travis-ci.org/bundler/bundler/builds/509497021. Only three jobs failed, and for old rubies/rubygems/bundler). ### What is your fix for the problem, implemented in this PR? My fix is to limit the hack to `bundler`'s spec run, since it's only needed there. ### Why did you choose this fix out of the possible options? I chose this fix because fully getting the build green with the hack fully removed is a bit of work. I dedicated a bit of time to investigate one of the failures and it all comes down to the priority of loading default gems vs `-I`. So I think something like rubygems/rubygems#1868 should fix it. But that's out of the scope of this PR. Closes #6927. Co-authored-by: David Rodríguez <[email protected]>
Build succeeded |
7193: More `require_relative` r=deivid-rodriguez a=deivid-rodriguez This is a follow up to #7062 and #7100, migrating the last missing internal requires to use `require_relative`. I thought I had migrated everything already, but I had not. As a note, I'm migrating thor's code here. I will open a PR to upstream's thor to incorporate this changes, but thor is still stuck with 1.8 support, so we have to wait a bit. Given the very low activity thor has, it's fine to make the changes here first, and wait until we can incorporate them upstream. ### What was the end-user problem that led to this PR? The problem was that sometimes we can end up requiring the default version of bundler, instead of the current copy that's being run, and that's dangerous and can cause version mismatch issues. ### What is your fix for the problem, implemented in this PR? My fix is to always use `require_relative` for internal requires. Co-authored-by: David Rodríguez <[email protected]>
What was the end-user problem that led to this PR?
The problem was that we are doing some hacks in our version file that are... dangerous. At very least they cause "circular problems" like #6927, but I also have bad feelings about it.
What was your diagnosis of the problem?
My diagnosis was we're overwriting the version of the currently loaded bundler 😬. This hack is almost unnecessary (see the spec run here with the hack fully removed: https://travis-ci.org/bundler/bundler/builds/509497021. Only three jobs failed, and for old rubies/rubygems/bundler).
What is your fix for the problem, implemented in this PR?
My fix is to limit the hack to
bundler
's spec run, since it's only needed there.Why did you choose this fix out of the possible options?
I chose this fix because fully getting the build green with the hack fully removed is a bit of work. I dedicated a bit of time to investigate one of the failures and it all comes down to the priority of loading default gems vs
-I
. So I think something like rubygems/rubygems#1868 should fix it. But that's out of the scope of this PR.Closes #6927.