Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Remove version overwriting hacks #7062

Merged
6 commits merged into from May 12, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Mar 23, 2019

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.

@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 2 times, most recently from af4e349 to 0ce7b55 Compare April 3, 2019 15:50
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 2 times, most recently from 8f1b215 to f43d751 Compare April 5, 2019 16:40
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch from f43d751 to 9d5c8e6 Compare April 22, 2019 23:11
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 4 times, most recently from 788f7f4 to a98d8ba Compare April 23, 2019 16:45
@deivid-rodriguez
Copy link
Member Author

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 BundlerVersionFinder. So I added a check for the rubygems version, and removed the hack.

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 馃槵...

@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 2 times, most recently from 49c5beb to 76a6e63 Compare April 26, 2019 18:23
lib/bundler.rb Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
spec/bundler/shared_helpers_spec.rb Outdated Show resolved Hide resolved
spec/runtime/load_spec.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Member Author

Sorry @segiddins. This is not ready for review. A general idea of whether you think this is a good idea is appreciated though.

@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 2 times, most recently from 37a362c to 44c6655 Compare May 3, 2019 17:14
@deivid-rodriguez
Copy link
Member Author

@segiddins This should be ready for a review now!

@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch from 44c6655 to b134ca0 Compare May 8, 2019 10:05
@deivid-rodriguez deivid-rodriguez removed the request for review from segiddins May 8, 2019 10:07
@deivid-rodriguez
Copy link
Member Author

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 :)

@deivid-rodriguez deivid-rodriguez changed the title Remove version overwriting hacks from regular runtime Remove version overwriting hacks May 8, 2019
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch 5 times, most recently from 9802616 to 8e76547 Compare May 8, 2019 19:02
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch from 8e76547 to 7911450 Compare May 8, 2019 19:34
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented May 8, 2019

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 Molinillo, and fileutils has already been updated but changes have not yet been incorporated here (I created #7156 to do that separately). The more complicated one would be thor because it still supports 1.8.7. I requested that to be dropped in rails/thor#643 (comment). In any case, the activity in the thor repo is really low, so I think it's fine to incorporate these changes directly, and propose them upstream once they drop support.

I'd like others to chime in here 馃槂

@deivid-rodriguez
Copy link
Member Author

Also, I think this might get green several specs that are currently skipped when running with the "ruby-core" flag.

Copy link
Member

@indirect indirect left a 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.
@deivid-rodriguez deivid-rodriguez force-pushed the remove_version_overwriting_hacks_from_regular_runtime branch from 7911450 to c675b59 Compare May 12, 2019 13:26
@deivid-rodriguez
Copy link
Member Author

Thanks @indirect!

@bundlerbot r+

ghost pushed a commit that referenced this pull request May 12, 2019
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 <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented May 12, 2019

Build succeeded

@ghost ghost merged commit c675b59 into master May 12, 2019
@ghost ghost deleted the remove_version_overwriting_hacks_from_regular_runtime branch May 12, 2019 14:56
ghost pushed a commit that referenced this pull request Jun 10, 2019
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 <deivid.rodriguez@riseup.net>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants