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

platform can be of type string so it must be cast into a Gem::Platform #3565

Closed
wants to merge 2 commits into from

Conversation

jaym
Copy link
Contributor

@jaym jaym commented Apr 13, 2015

#3553 didn't actually fix anything and made things worse (universal gems no longer work on 32 bit ruby on windows)

It seems that the platform can be of type string, so wrapping in in Gem::Platform is necessary.

sorry for the regression

@@ -8,7 +8,7 @@ def match_platform(p)
Gem::Platform::RUBY == platform or
platform.nil? or p == platform or
generic(Gem::Platform.new(platform)) == p or
platform === p
Gem::Platform.new(platform) === p
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Gem::Platform.new(platform) is done twice, maybe it should be wrapped in a variable?

@segiddins
Copy link
Member

@jdmundrawala maybe it would just be better to do this in the initializer for LazySpecification?

@jaym
Copy link
Contributor Author

jaym commented Apr 13, 2015

@segiddins I'm a little worried about changing LazySpecification and EnpointSecification to do that.
A similar thing was done in rubygems, and produced a bug like: rubygems/rubygems#1184

@segiddins
Copy link
Member

Ah actually you’re right, I made that very mistake this weeked

On Apr 13, 2015, at 10:29 AM, Jay Mundrawala [email protected] wrote:

@segiddins https://github.com/segiddins I'm a little worried about changing LazySpecification and EnpointSecification to do that.
A similar thing was done in rubygems, and produced a bug like: rubygems/rubygems#1184 rubygems/rubygems#1184

Reply to this email directly or view it on GitHub #3565 (comment).

@jaym
Copy link
Contributor Author

jaym commented Apr 13, 2015

So reading this again, maybe the === comparison is all that is needed and the == can be dropped

@@ -5,10 +5,11 @@ module MatchPlatform
include GemHelpers

def match_platform(p)
generic_platform = generic(Gem::Platform.new(platform))
Gem::Platform::RUBY == platform or
platform.nil? or p == platform or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vast, vast majority of these comparisons are going to exit on the first three conditions, though, so I think it's a performance loss to create a Platform before we do the compare... can we switch to just the one === and wait to create the Platform until then?

@jaym
Copy link
Contributor Author

jaym commented Apr 13, 2015

Not really certain why the tests keep failing....

  it "installs the latest version of gxapi_rails", :ruby => "1.9"  do
    install_gemfile <<-G
    source "https://rubygems.org"

    gem "sass-rails"
    gem "rails", "~> 3"
    gem "gxapi_rails"
    G
    expect(out).to include("gxapi_rails 0.0.6")
  end

seems to never return. Tried running locally(on a mac) with rubygems 2.2.3 and ruby 1.9.3 with the following Gemfile

source "https://rubygems.org"

gem "sass-rails"
gem "rails", "~> 3"
gem "gxapi_rails"

never returns. Tried with bundler 1.9.2 and bundler 1.9.3

never in this case means

Fetching gem metadata from https://rubygems.org/...........
Fetching version metadata from https://rubygems.org/..
Resolving dependencies..................................................................................................................................................................................................................................................................................................................................................^C

@segiddins
Copy link
Member

@jdmundrawala that would imply that the resolver is either looping infinitely, or it's missing the match that it needs to terminate in a sane amount of time.

@jaym
Copy link
Contributor Author

jaym commented Apr 13, 2015

bundle install 171.70s user 1.46s system 97% cpu 2:58.17 total
It just took a while

@indirect
Copy link
Member

@segiddins that seems like a pretty pathological resolver case, any ideas?

@indirect
Copy link
Member

(For the record, 1-8-stable takes 19 seconds to finish resolving that Gemfile, while 1-9-stable takes... much longer.)

@indirect
Copy link
Member

@jdmundrawala Thanks for the patch!

@indirect
Copy link
Member

Cherry-picked to 1-9-stable as 99a7129 and de368e1.

@indirect indirect closed this Apr 14, 2015
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