Compare sources using source's root #5817

NickLaMuro · 2017-06-26T22:44:33Z

What was the end-user problem that led to this PR?

Given a setup where:

A project's Gemfile makes use of another project's Gemfile and the eval_gemfile method to share the dependencies. Something like:

# project_plugin's Gemfile

eval_gemfile(File.expand_path("../../main_project/Gemfile", __FILE__))

The main project includes a path gem in it that is nested in the main project:

# main_project's Gemfile
gem "foo_gem", :path => "vendor/foo_gem"

A bundle install is followed by a bundle install --deployment, the second of which triggers a comparison of the lockfile.sources and the Bundler.definition

A error will occur when comparing the specs, saying the the "foo_gem" source has been deleted:

$ bundle install
...
$ bundle install --deployment
You are trying to install in deployment mode after changing
your Gemfile. Run `bundle install` elsewhere and add the
updated Gemfile.lock to version control.

the gemspecs for path gems changed


You have deleted from the Gemfile:
* source: source at `../main_project/vendor/foo_gem`

What was your diagnosis of the problem?

(extracted from the commit message)

When doing the following:

expand(other.original_path)

inside a Bundler::Source::Path instance, the @root_path from the instance that is having eq? called on it, the the other instance's root_path. This, in obscure cases, can cause a bug when you are doing an nested eval_gemfile or other calls when comparing the lockfile's locked path sources to the Bundler.definition's path sources.

What is your fix for the problem, implemented in this PR?

Use a new public method, Bundler::Source::Path#expanded_original_path, in the eq? method instead of using's the instance's #expand method.

Why did you choose this fix out of the possible options?

(extracted from the commit message)

Creating the expanded_original_path method allows a public interface to be made available (and doesn't mess with any exiting functionality) that allows comparing the source path of one Source::Path's expand_path to another, where each uses their own root_path to resolve their full path, instead of sharing the base one and causing edge case bugs

ghost · 2017-06-26T22:44:42Z

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

NickLaMuro · 2017-06-26T22:45:48Z

@segiddins pinging you only because you have touched this file recently... sorry if this is a bother

Wanted to add a test for this change, but wasn't sure the best spot to add it... have any suggestions on where to put it?

segiddins · 2017-06-27T14:05:46Z

spec/install/gemfile/eval_gemfile_spec.rb ?

segiddins · 2017-06-27T15:15:44Z

lib/bundler/source/path.rb

@@ -65,7 +65,7 @@ def hash

      def eql?(other)
        return unless other.class == self.class
-        expand(@original_path) == expand(other.original_path) &&
+        expand(@original_path) == other.expanded_original_path &&


expanded_original_path == other.expanded_original_path ?

You know... that change would have made sense, huh... oops

I will update when I add a test for this as well. Thanks.

NickLaMuro · 2017-06-27T22:04:27Z

spec/install/gemfile/eval_gemfile_spec.rb

@@ -41,6 +41,13 @@
      bundle! :install
      expect(the_bundle).to include_gem("a 1.0")
    end
+
+    # Make sure that we are properly comparing path based gems between the
+    # parsed lockfile and the evaluated gemfile.


For this one, it will fail in a blaze of glory (when using the old code) with something along the lines of the following outputted (trimmed down a bit for readability):

$ ruby -Ilib:spec -rsupport/hax -rsupport/artifice/fail exe/bundle install --no-color Resolving dependencies... Using a 1.0 from source at `../gems/a` Using bundler 1.15.1 Bundle complete! 1 Gemfile dependency, 2 gems now installed. Use `bundle info [gemname]` to see where a bundled gem is installed. # $? => 0 $ ruby -Ilib:spec -rsupport/hax -rsupport/artifice/fail exe/bundle install --deployment --no-color You are trying to install in deployment mode after changing your Gemfile. Run `bundle install` elsewhere and add the updated Gemfile.lock to version control. The list of sources changed The gemspecs for path gems changed You have added to the Gemfile: * source: source at `../gems/a` # $? => 16

Not sure if it was worth adding all this as a comment to the spec, but leaving this hear for some context

NickLaMuro · 2017-06-27T22:09:33Z

@segiddins Added a unit test in there as well that contains a few light tests for the eql? method specifically. Let me know if you would prefer that one omitted and I can remove it. Thanks.

When doing the following: expand(other.original_path) inside a Bundler::Source::Path instance, the `@root_path` from the instance that is having eql? called on it, the other instance's root_path. This, in obscure cases, can cause a bug when you are doing an nested eval_gemfile or other calls when comparing the lockfile's locked path sources to the Bundler.definition's path sources. Creating the expanded_original_path method allows a public interface to be made available (and doesn't mess with any exiting functionality) that allows comparing the source path of one Source::Path's expand_path to another, where each uses their own root_path to resolve their full path, instead of sharing the base one and causing edge case bugs

segiddins · 2017-07-05T13:38:07Z

Thanks!
@bundlerbot r+

bundlerbot · 2017-07-05T13:38:08Z

📌 Commit 3db89ca has been approved by segiddins

bundlerbot · 2017-07-05T14:17:43Z

⌛ Testing commit 3db89ca with merge fb33846...

…, r=segiddins Compare sources using source's root ### What was the end-user problem that led to this PR? Given a setup where: 1. A project's Gemfile makes use of another project's Gemfile and the `eval_gemfile` method to share the dependencies. Something like: ```ruby # project_plugin's Gemfile eval_gemfile(File.expand_path("../../main_project/Gemfile", __FILE__)) ``` 2. The main project includes a path gem in it that is nested in the main project: ```ruby # main_project's Gemfile gem "foo_gem", :path => "vendor/foo_gem" ``` 3. A `bundle install` is followed by a `bundle install --deployment`, the second of which triggers a comparison of the `lockfile.sources` and the `Bundler.definition` A error will occur when comparing the specs, saying the the "foo_gem" source has been deleted: ```console $ bundle install ... $ bundle install --deployment You are trying to install in deployment mode after changing your Gemfile. Run `bundle install` elsewhere and add the updated Gemfile.lock to version control. the gemspecs for path gems changed You have deleted from the Gemfile: * source: source at `../main_project/vendor/foo_gem` ``` ### What was your diagnosis of the problem? (extracted from the commit message) When doing the following: expand(other.original_path) inside a `Bundler::Source::Path` instance, the `@root_path` from the instance that is having `eq?` called on it, the the `other` instance's `root_path`. This, in obscure cases, can cause a bug when you are doing an nested eval_gemfile or other calls when comparing the lockfile's locked path sources to the `Bundler.definition`'s path sources. ### What is your fix for the problem, implemented in this PR? Use a new public method, `Bundler::Source::Path#expanded_original_path`, in the `eq?` method instead of using's the instance's `#expand` method. ### Why did you choose this fix out of the possible options? (extracted from the commit message) Creating the `expanded_original_path` method allows a public interface to be made available (and doesn't mess with any exiting functionality) that allows comparing the source path of one `Source::Path`'s `expand_path` to another, where each uses their own `root_path` to resolve their full path, instead of sharing the base one and causing edge case bugs

bundlerbot · 2017-07-05T18:24:56Z

☀️ Test successful - status-travis
Approved by: segiddins
Pushing fb33846 to master...

…, r=segiddins Compare sources using source's root ### What was the end-user problem that led to this PR? Given a setup where: 1. A project's Gemfile makes use of another project's Gemfile and the `eval_gemfile` method to share the dependencies. Something like: ```ruby # project_plugin's Gemfile eval_gemfile(File.expand_path("../../main_project/Gemfile", __FILE__)) ``` 2. The main project includes a path gem in it that is nested in the main project: ```ruby # main_project's Gemfile gem "foo_gem", :path => "vendor/foo_gem" ``` 3. A `bundle install` is followed by a `bundle install --deployment`, the second of which triggers a comparison of the `lockfile.sources` and the `Bundler.definition` A error will occur when comparing the specs, saying the the "foo_gem" source has been deleted: ```console $ bundle install ... $ bundle install --deployment You are trying to install in deployment mode after changing your Gemfile. Run `bundle install` elsewhere and add the updated Gemfile.lock to version control. the gemspecs for path gems changed You have deleted from the Gemfile: * source: source at `../main_project/vendor/foo_gem` ``` ### What was your diagnosis of the problem? (extracted from the commit message) When doing the following: expand(other.original_path) inside a `Bundler::Source::Path` instance, the `@root_path` from the instance that is having `eq?` called on it, the the `other` instance's `root_path`. This, in obscure cases, can cause a bug when you are doing an nested eval_gemfile or other calls when comparing the lockfile's locked path sources to the `Bundler.definition`'s path sources. ### What is your fix for the problem, implemented in this PR? Use a new public method, `Bundler::Source::Path#expanded_original_path`, in the `eq?` method instead of using's the instance's `#expand` method. ### Why did you choose this fix out of the possible options? (extracted from the commit message) Creating the `expanded_original_path` method allows a public interface to be made available (and doesn't mess with any exiting functionality) that allows comparing the source path of one `Source::Path`'s `expand_path` to another, where each uses their own `root_path` to resolve their full path, instead of sharing the base one and causing edge case bugs (cherry picked from commit fb33846)

NickLaMuro mentioned this pull request Jun 26, 2017

travis - run bundler with --no-deployment ManageIQ/manageiq-ui-classic#1606

Merged

segiddins reviewed Jun 27, 2017

View reviewed changes

NickLaMuro force-pushed the bug_with_path_gem_source_equivalency branch from 491a0f8 to b4e94d6 Compare June 27, 2017 22:00

NickLaMuro commented Jun 27, 2017

View reviewed changes

himdel mentioned this pull request Jun 29, 2017

Stub mini-mime in for mime-types ManageIQ/manageiq#14525

Merged

NickLaMuro force-pushed the bug_with_path_gem_source_equivalency branch from b4e94d6 to 3db89ca Compare June 29, 2017 16:32

NickLaMuro mentioned this pull request Jul 3, 2017

Use full path in the Gemfile ManageIQ/manageiq#15503

Merged

segiddins added this to the 1.15.2 milestone Jul 5, 2017

bundlerbot merged commit 3db89ca into rubygems:master Jul 5, 2017

NickLaMuro mentioned this pull request Sep 7, 2017

Shim of the mime-types gem discourse/mini_mime#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare sources using source's root #5817

Compare sources using source's root #5817

NickLaMuro commented Jun 26, 2017

ghost commented Jun 26, 2017

NickLaMuro commented Jun 26, 2017

segiddins commented Jun 27, 2017

segiddins Jun 27, 2017

NickLaMuro Jun 27, 2017

segiddins Jun 27, 2017

NickLaMuro Jun 27, 2017

NickLaMuro commented Jun 27, 2017

segiddins commented Jul 5, 2017

bundlerbot commented Jul 5, 2017

bundlerbot commented Jul 5, 2017

bundlerbot commented Jul 5, 2017

Compare sources using source's root #5817

Compare sources using source's root #5817

Conversation

NickLaMuro commented Jun 26, 2017

What was the end-user problem that led to this PR?

What was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

Why did you choose this fix out of the possible options?

ghost commented Jun 26, 2017

NickLaMuro commented Jun 26, 2017

segiddins commented Jun 27, 2017

segiddins Jun 27, 2017

Choose a reason for hiding this comment

NickLaMuro Jun 27, 2017

Choose a reason for hiding this comment

segiddins Jun 27, 2017

Choose a reason for hiding this comment

NickLaMuro Jun 27, 2017

Choose a reason for hiding this comment

NickLaMuro commented Jun 27, 2017

segiddins commented Jul 5, 2017

bundlerbot commented Jul 5, 2017

bundlerbot commented Jul 5, 2017

bundlerbot commented Jul 5, 2017