Skip to content
This repository has been archived by the owner. It is now read-only.

Fix DepProxy == method #6669

Merged
merged 2 commits into from Aug 27, 2018
Merged

Fix DepProxy == method #6669

merged 2 commits into from Aug 27, 2018

Conversation

@ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Aug 22, 2018

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

After implementing a new hash table strategy in JRuby, bundle is broken for JRuby. The problem is caused that the == method in bundler does not check the class of the other object. This causes problems now when calling == on object other than DepProxy or nil.

jruby/jruby#5280 travis-ci/travis-ci#9994

What was your diagnosis of the problem?

The code crashes for anything other than DepProxy class or nil.

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

Checking now also that other class is the same as self.

@ghost
Copy link

@ghost ghost commented Aug 22, 2018

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

@colby-swandale
Copy link
Member

@colby-swandale colby-swandale commented Aug 22, 2018

Forgive the ignorance but i don't understand how this is a Bundler issue. If JRuby is incorrectly returning objects it shouldn't, patching the application code to go around that bad behavior seems backwards to me. Shouldn't the change in JRuby that is causing this be reverted or patched instead?

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 22, 2018

Forgive the ignorance but i don't understand how this is a Bundler issue. If JRuby is incorrectly returning objects it shouldn't, patching the application code to go around that bad behavior seems backwards to me. Shouldn't the change in JRuby that is causing this be reverted or patched instead?

Hey @colby-swandale, thanks for your response. JRuby is not incorrectly returning objects, it was just hidden before because of checking hash AND object equality, so it never checked object equality when hash is different. We will change JRuby to the old behavior but I think the code in bundler is just wrong. E.g. this will crash and not return false as it is expected:

proxy = Bundler::DepProy.new('foobar', 'java')
proxy == "string"
@colby-swandale
Copy link
Member

@colby-swandale colby-swandale commented Aug 22, 2018

Thanks for clearing that up, i don't know where i got the idea of an incorrect object being returned from. 😞

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 22, 2018

Thanks for clearing that up, i don't know where i got the idea of an incorrect object being returned from.

No worries! Should I add tests for this? Altough I didn't find specs for dep_proxy so far ...

@segiddins
Copy link
Member

@segiddins segiddins commented Aug 22, 2018

Yes, please do add a test for the bug this is trying to fix

@ChrisBr ChrisBr force-pushed the ChrisBr:fix_dep_proxy branch from fdd15fa to 9e0578e Aug 22, 2018
@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 22, 2018

@segiddins @colby-swandale added tests, please have a look again!

ChrisBr added 2 commits Aug 22, 2018
DepProxy#== crashed with an undefind method error
for anything other than a DepProxy class or nil
as parameter. This was caused that it was assumed
only DepProxy instances or nil can get passed as
parameter. This commit implements checking the class
as well and returns false if the classes are not
the same.

Fixes jruby/jruby#5280 travis-ci/travis-ci#9994
According to the official Ruby documentation, "the eql? method returns true if obj and other
refer to the same hash key." This was not the case as the hash key of a DepProxy instance was
only calculated based on the dep object but in the eql? method dep and platform attributes were
computed. This caused that equal objects returned different hash keys.

https://ruby-doc.org/core-2.5.1/Object.html#method-i-eql-3F
@ChrisBr ChrisBr force-pushed the ChrisBr:fix_dep_proxy branch from 9e0578e to ce92868 Aug 23, 2018
@segiddins
Copy link
Member

@segiddins segiddins commented Aug 27, 2018

Thanks for this!

Just a note that the prior definition of hash was technically acceptable. eql? implies hash equality, but its OK (though undesirable) for !eql? objects to have the same hash value.

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

@bundlerbot bundlerbot commented Aug 27, 2018

📌 Commit ce92868 has been approved by segiddins

@bundlerbot
Copy link
Collaborator

@bundlerbot bundlerbot commented Aug 27, 2018

Testing commit ce92868 with merge 1856133...

bundlerbot added a commit that referenced this pull request Aug 27, 2018
Fix DepProxy == method

### What was the end-user problem that led to this PR?
After implementing a new hash table strategy in JRuby, bundle is broken for JRuby. The problem is caused that the ``==`` method in bundler does not check the class of the ``other`` object. This causes problems now when calling ``==`` on object other than DepProxy or nil.

 jruby/jruby#5280 travis-ci/travis-ci#9994

### What was your diagnosis of the problem?
The code crashes for anything other than DepProxy class or nil.

### What is your fix for the problem, implemented in this PR?
Checking now also that other class is the same as self.
@bundlerbot
Copy link
Collaborator

@bundlerbot bundlerbot commented Aug 27, 2018

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

@bundlerbot bundlerbot merged commit ce92868 into rubygems:master Aug 27, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bundlerbot
homu Test successful
Details
@colby-swandale colby-swandale added this to the 1.16.5 milestone Sep 10, 2018
colby-swandale added a commit that referenced this pull request Sep 14, 2018
Fix DepProxy == method

### What was the end-user problem that led to this PR?
After implementing a new hash table strategy in JRuby, bundle is broken for JRuby. The problem is caused that the ``==`` method in bundler does not check the class of the ``other`` object. This causes problems now when calling ``==`` on object other than DepProxy or nil.

 jruby/jruby#5280 travis-ci/travis-ci#9994

### What was your diagnosis of the problem?
The code crashes for anything other than DepProxy class or nil.

### What is your fix for the problem, implemented in this PR?
Checking now also that other class is the same as self.

(cherry picked from commit 1856133)
colby-swandale added a commit that referenced this pull request Sep 18, 2018
* 1-16-stable:
  Version 1.16.5 with changelog
  scope TruffleRuby platform specs to be RubyGems >= 2.1.0
  Auto merge of #6689 - bundler:colby/fix-bundler-load-error, r=colby-swandale
  Auto merge of #6695 - bundler:segiddins/6684-gvp-prefer-non-pres, r=colby-swandale
  Auto merge of #6693 - eregon:truffleruby, r=colby-swandale
  Auto merge of #6692 - eregon:simplify-autoload-require-deprecate, r=segiddins
  Auto merge of #6688 - voxik:check-search, r=colby-swandale
  Auto merge of #6682 - bundler:bundle_env_formatting, r=colby-swandale
  Auto merge of #6675 - MaxLap:master, r=greysteil
  Auto merge of #6669 - ChrisBr:fix_dep_proxy, r=segiddins
  Auto merge of #6664 - greysteil:avoid-printing-git-error, r=colby-swandale
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants