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

Reactivate usused code for ruby version resolution #7559

Merged
1 commit merged into from Jan 17, 2020

Conversation

larskanis
Copy link
Contributor

The code was changed in commit 38b0e7e so that RubyVersion.system was no longer respected. This is reactivated now and specs are adjusted accordingly.

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

Not known.

What was your diagnosis of the problem?

I read the bundler source code and noticed this.

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

My fix re-adds the ruby version as it was before commit 38b0e7e , since it seems to be a mistake.

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

As a alternative I added #7558 that removes the code in question.

@deivid-rodriguez
Copy link
Member

Thanks @larskanis! Do you have any particular preference? Intuitively to me, less code is better than more code, so I would go with #7558. @kou any opinion?

@kou
Copy link
Contributor

kou commented Jan 14, 2020

I like #7558 too if we don't have regression by 38b0e7e. (I don't confirm it yet. Sorry.)

But I want to rename concat_ruby_version_requirements in #7558. Because it doesn't concat anything.

@larskanis
Copy link
Contributor Author

larskanis commented Jan 14, 2020

I honesty don't really understand the impact of this code and I didn't find a difference in resolution between #7558 and #7559. But there's at least a user visible difference. In case of ruby version conflicts with #7522 this PR prints:

$ ruby -Ilib exe/bundle install
Fetching gem metadata from https://rubygems.org/............
Resolving dependencies...
Bundler found conflicting requirements for the Ruby  version:
  In Gemfile:
    Ruby  (= 2.7.0.0) x64-mingw32

    ffi (~> 1.11) x64-mingw32 was resolved to 1.11.3, which depends on
      Ruby  (>= 2.0)

while #7558 doesn't print the current ruby version:

$ ruby -Ilib exe/bundle install
Fetching gem metadata from https://rubygems.org/..........
Resolving dependencies...
Bundler found conflicting requirements for the Ruby  version:
  In Gemfile:
    Ruby  x64-mingw32

    ffi (~> 1.11) x64-mingw32 was resolved to 1.11.3, which depends on
      Ruby  (>= 2.0)

This difference is why I had to adjust the specs in this PR.

@kou
Copy link
Contributor

kou commented Jan 14, 2020

OK. I'll check this with #7522.

@deivid-rodriguez
Copy link
Member

Mmm, I see. In that case, it sounds like this PR would be the way to go. At least I prefer the extra bit of clarification in the failure message.

@deivid-rodriguez
Copy link
Member

@larskanis Can you rebase this PR?

The code was changed in commit 38b0e7e so that RubyVersion.system was no longer respected.
This is reactivated now and specs are adjusted accordingly.
@larskanis
Copy link
Contributor Author

@deivid-rodriguez I rebased this PR and #7558 to master.

@deivid-rodriguez
Copy link
Member

I'm going to go with this PR out of the two, let me know if any of you have any concerns.

@bundlerbot merge

ghost pushed a commit that referenced this pull request Jan 17, 2020
7559: Reactivate usused code for ruby version resolution r=deivid-rodriguez a=larskanis

The code was changed in commit 38b0e7e so that RubyVersion.system was no longer respected. This is reactivated now and specs are adjusted accordingly.

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

Not known.

### What was your diagnosis of the problem?

I read the bundler source code and noticed this.

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

My fix re-adds the ruby version as it was before commit 38b0e7e , since it seems to be a mistake.

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

As a alternative I added #7558 that removes the code in question.


Co-authored-by: Lars Kanis <kanis@comcard.de>
@ghost
Copy link

ghost commented Jan 17, 2020

Build succeeded

@ghost ghost merged commit 394cd14 into rubygems:master Jan 17, 2020
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