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

Remove version overwriting hacks#7062

Merged
6 commits merged into
masterfrom
remove_version_overwriting_hacks_from_regular_runtime
May 12, 2019
Merged

Remove version overwriting hacks#7062
6 commits merged into
masterfrom
remove_version_overwriting_hacks_from_regular_runtime

Conversation

@deivid-rodriguez

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

Copy link
Copy Markdown
Contributor

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 ruby/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
Copy Markdown
Contributor 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
Comment thread lib/bundler.rb
Comment thread .travis.yml Outdated
Comment thread spec/bundler/shared_helpers_spec.rb
Comment thread spec/runtime/load_spec.rb Outdated
@deivid-rodriguez

Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor Author

@segiddins This should be ready for a review now!

@deivid-rodriguez deivid-rodriguez requested a review from segiddins May 3, 2019 18:00
@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
Copy Markdown
Contributor 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

deivid-rodriguez commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

@indirect indirect left a comment

Copy link
Copy Markdown

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
Copy Markdown
Contributor 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 ruby/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

ghost commented May 12, 2019

Copy link
Copy Markdown

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.

3 participants