Fix force_ruby_platform: when the lockfile only locks the ruby platform

[segiddins]

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

I was unable to force ruby platform for google-protobuf in the rubygems.org bundle

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

Respect force_ruby_platform: when materializing by transferring it over to the lazy spec from the gemfile dep

Make sure the following tasks are checked

• Describe the problem / feature
• Write tests for features and bug fixes
• Write code to solve the problem
• Make sure you follow the current code style and write meaningful commit messages without tags

[segiddins]

(I don't love this fix but I got really fed up after a few hours and figured it was worth getting something up that (a) had a failing spec and (b) fixed that one spec)

[martinemde]

The fix itself makes sense to me, though I don't fully understand the implications from the change.

[martinemde]

Hmm, maybe a little refactor could make this solution feel more elegant to you?

matching _specs =
  if dep.force_ruby_platform
    GemHelpers.force_ruby_platform(specs_for_name)
  else
    GemHelpers.select_best_platform_match(specs_for_name, platform || Bundler.local_platform)
  end

Then you don't need to iterate through matching_specs twice since it can be handled in GemHelpers more cleanly.

[segiddins]

honestly, feels the same. matching specs should always be a very small array, so the double iteration doesn't feel that bad

[deivid-rodriguez]

Looks great, nice catch!

I only added a comment to simplify the spec to better illustrate the bug.

[deivid-rodriguez]

This bug seems to happen when a platform specific variant is pre-installed, the Gemfile is using :force_ruby_platform => true, and the lockfile includes "RUBY" platform.

In this case, the locally installed platform specific variant will be selected, and unless we propagate the force_ruby_platform flag to it from the Gemfile, it will be happily picked as already installed, and used for materialization.

So, I believe transitive dependencies are unrelated to the fix, and I think the whole spec can be simplified to better illustrate this case like this:

it "reinstalls the ruby variant when a platform specific variant is already installed, the lockile has only RUBY platform, and :force_ruby_platform is used in the Gemfile" do
  lockfile <<-L
    GEM
      remote: #{file_uri_for(gem_repo4)}
      specs:
        platform_specific (1.0)

    PLATFORMS
      ruby

    DEPENDENCIES
      platform_specific

    BUNDLED WITH
       #{Bundler::VERSION}
  L

  system_gems "platform_specific-1.0-#{Gem::Platform.local}", :path => default_bundle_path

  install_gemfile <<-G, :env => { "BUNDLER_SPEC_GEM_REPO" => gem_repo4.to_s }, :artifice => "compact_index", :verbose => true
    source "#{file_uri_for(gem_repo4)}"

    gem "platform_specific", :force_ruby_platform => true
  G

  expect(the_bundle).to include_gems "platform_specific 1.0.0 RUBY"
end

[segiddins]

maybe just add that as a second spec, since we also want to test the transitive case, and the behavior could very well end up different with a different implementation

[deivid-rodriguez]

Sure, I just don't see it as related to this PR.

I'm also not sure which role dep_level_2 intends to play since it's in the lockfile but not in the gemfile. I guess it's testing replacing one dependency in the Gemfile with a subdependency of it? If we want to keep it, I'd at least clarify the wording.

[deivid-rodriguez]

Approving this to clarify that my comment on the spec is definitely not blocking. This is a great fix!