New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DepProxy == method #6669

Merged
merged 2 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@ChrisBr
Copy link
Contributor

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.

@bundlerbot

This comment has been minimized.

Copy link
Contributor

bundlerbot bot 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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

ChrisBr commented Aug 22, 2018

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

ChrisBr added some commits Aug 22, 2018

Fix DepProxy#== undefind method error
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
Fix DepProxy#hash calculation
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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

bundlerbot commented Aug 27, 2018

📌 Commit ce92868 has been approved by segiddins

@bundlerbot

This comment has been minimized.

Copy link
Contributor

bundlerbot commented Aug 27, 2018

⌛️ Testing commit ce92868 with merge 1856133...

bundlerbot added a commit that referenced this pull request Aug 27, 2018

Auto merge of #6669 - ChrisBr:fix_dep_proxy, r=segiddins
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

This comment has been minimized.

Copy link
Contributor

bundlerbot commented Aug 27, 2018

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

@bundlerbot bundlerbot merged commit ce92868 into bundler:master Aug 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

Auto merge of #6669 - ChrisBr:fix_dep_proxy, r=segiddins
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

merge v1.16.5
* 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 join this conversation on GitHub. Already have an account? Sign in to comment