Skip to content

Fix platform specific version for libv8-node and other allowlisted gems not being chosen in Truffleruby #6169

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.

Sign up for GitHub

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

Merged
deivid-rodriguez merged 2 commits into from
Dec 21, 2022
Merged

Fix platform specific version for libv8-node and other allowlisted gems not being chosen in Truffleruby #6169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2ebafc
Better platform mismatch spec failures
deivid-rodriguez Dec 21, 2022
0b1edc0
Fix platform selection on truffleruby
deivid-rodriguez Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Manifest.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ bundler/lib/bundler/fetcher/compact_index.rb
bundler/lib/bundler/fetcher/dependency.rb
bundler/lib/bundler/fetcher/downloader.rb
bundler/lib/bundler/fetcher/index.rb
bundler/lib/bundler/force_platform.rb
bundler/lib/bundler/friendly_errors.rb
bundler/lib/bundler/gem_helper.rb
bundler/lib/bundler/gem_helpers.rb
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ def unbundled_exec(*args)
end

def local_platform
return Gem::Platform::RUBY if settings[:force_ruby_platform] || Gem.platforms == [Gem::Platform::RUBY]
return Gem::Platform::RUBY if settings[:force_ruby_platform]
Gem::Platform.local
end

Expand Down
6 changes: 3 additions & 3 deletions bundler/lib/bundler/dependency.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
module Bundler
class Dependency < Gem::Dependency
attr_reader :autorequire
attr_reader :groups, :platforms, :gemfile, :path, :git, :github, :branch, :ref, :force_ruby_platform
attr_reader :groups, :platforms, :gemfile, :path, :git, :github, :branch, :ref

ALL_RUBY_VERSIONS = ((18..27).to_a + (30..31).to_a).freeze
PLATFORM_MAP = {
Expand Down Expand Up @@ -42,15 +42,15 @@ def initialize(name, version, options = {}, &blk)
@env = options["env"]
@should_include = options.fetch("should_include", true)
@gemfile = options["gemfile"]
@force_ruby_platform = options["force_ruby_platform"]
@force_ruby_platform = options["force_ruby_platform"] if options.key?("force_ruby_platform")

@autorequire = Array(options["require"] || []) if options.key?("require")
end

# Returns the platforms this dependency is valid for, in the same order as
# passed in the `valid_platforms` parameter
def gem_platforms(valid_platforms)
return [Gem::Platform::RUBY] if @force_ruby_platform
return [Gem::Platform::RUBY] if force_ruby_platform
return valid_platforms if @platforms.empty?

valid_platforms.select {|p| expanded_platforms.include?(GemHelpers.generic(p)) }
Expand Down
18 changes: 18 additions & 0 deletions bundler/lib/bundler/force_platform.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

module Bundler
module ForcePlatform
private

# The `:force_ruby_platform` value used by dependencies for resolution, and
# by locked specifications for materialization is `false` by default, except
# for TruffleRuby. TruffleRuby generally needs to force the RUBY platform
# variant unless the name is explicitly allowlisted.

def default_force_ruby_platform
return false unless RUBY_ENGINE == "truffleruby"

!Gem::Platform::REUSE_AS_BINARY_ON_TRUFFLERUBY.include?(name)
end
end
end
4 changes: 4 additions & 0 deletions bundler/lib/bundler/lazy_specification.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
# frozen_string_literal: true

require_relative "force_platform"

module Bundler
class LazySpecification
include MatchPlatform
include ForcePlatform

attr_reader :name, :version, :dependencies, :platform
attr_accessor :source, :remote, :force_ruby_platform
Expand All @@ -14,6 +17,7 @@ def initialize(name, version, platform, source = nil)
@platform = platform || Gem::Platform::RUBY
@source = source
@specification = nil
@force_ruby_platform = default_force_ruby_platform
end

def full_name
Expand Down
11 changes: 10 additions & 1 deletion bundler/lib/bundler/rubygems_ext.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
require "rubygems/source"

require_relative "match_metadata"
require_relative "force_platform"
require_relative "match_platform"

# Cherry-pick fixes to `Gem.ruby_version` to be useful for modern Bundler
Expand Down Expand Up @@ -153,12 +154,16 @@ def dependencies_to_gemfile(dependencies, group = nil)
end

class Dependency
include ::Bundler::ForcePlatform

attr_accessor :source, :groups

alias_method :eql?, :==

def force_ruby_platform
false
return @force_ruby_platform if defined?(@force_ruby_platform) && !@force_ruby_platform.nil?

@force_ruby_platform = default_force_ruby_platform
end

def encode_with(coder)
Expand Down Expand Up @@ -277,6 +282,10 @@ def normalized_linux_version_ext
without_gnu_nor_abi_modifiers
end
end

if RUBY_ENGINE == "truffleruby" && !defined?(REUSE_AS_BINARY_ON_TRUFFLERUBY)
REUSE_AS_BINARY_ON_TRUFFLERUBY = %w[libv8 libv8-node sorbet-static].freeze
end

@deivid-rodriguez deivid-rodriguez Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Later I will propose that truffleruby moves this constant to the defaults/truffleruby file, so that eventually we no longer need to keep it ourselves. We currently need to keep it ourselves for our own tests, and to work properly with upgraded copies of RubyGems that don't include it like the "blessed copy" of RubyGems included with truffleruby does.

@eregon eregon Jan 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should I do a PR to truffleruby to have

class Gem::Platform
  remove_const :REUSE_AS_BINARY_ON_TRUFFLERUBY if const_defined?(:REUSE_AS_BINARY_ON_TRUFFLERUBY)
  REUSE_AS_BINARY_ON_TRUFFLERUBY = %w[libv8 libv8-node sorbet-static]
end

in defaults/truffleruby then?
I can do that soon.

Or were you thinking of something else?

@deivid-rodriguez deivid-rodriguez Jan 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's what I was thinking.

@eregon eregon Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See truffleruby/truffleruby#2819

I think we don't need the remove_const, because defaults/truffleruby should be loaded before Bundler defines it and RubyGems itself doesn't define it, right? (if it actually happens we'll see a warning)

Also this means Gem::Platform.match_gem? etc cannot be called between starting to load RubyGems and until defaults/truffleruby is loaded. That seems fine.

@eregon eregon Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Depending on #6238 (comment) we might need to remove the constant

@deivid-rodriguez deivid-rodriguez Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, you shouldn't need remove_const because of that.

end

Platform.singleton_class.module_eval do
Expand Down
10 changes: 4 additions & 6 deletions bundler/lib/bundler/spec_set.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,10 @@ def tsort_each_node

def specs_for_dependency(dep, platform)
specs_for_name = lookup[dep.name]
if platform.nil?
matching_specs = specs_for_name.map {|s| s.materialize_for_installation if Gem::Platform.match_spec?(s) }.compact
GemHelpers.sort_best_platform_match(matching_specs, Bundler.local_platform)
else
GemHelpers.select_best_platform_match(specs_for_name, dep.force_ruby_platform ? Gem::Platform::RUBY : platform)
end
target_platform = dep.force_ruby_platform ? Gem::Platform::RUBY : (platform || Bundler.local_platform)
matching_specs = GemHelpers.select_best_platform_match(specs_for_name, target_platform)
matching_specs.map!(&:materialize_for_installation).compact! if platform.nil?
matching_specs
end

def tsort_each_child(s)
Expand Down
5 changes: 4 additions & 1 deletion bundler/spec/support/hax.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@ def self.ruby=(ruby)
end

if ENV["BUNDLER_SPEC_PLATFORM"]
previous_platforms = @platforms
previous_local = Platform.local

class Platform
@local = new(ENV["BUNDLER_SPEC_PLATFORM"])
end
@platforms = [Gem::Platform::RUBY, Gem::Platform.local]
@platforms = previous_platforms.map {|platform| platform == previous_local ? Platform.local : platform }
end

if ENV["BUNDLER_SPEC_GEM_SOURCES"]
Expand Down
2 changes: 1 addition & 1 deletion bundler/spec/support/matchers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def indent(string, padding = 4, indent_character = " ")
end
if exitstatus == 65
actual_platform = out.split("\n").last
next "#{name} was expected to be of platform #{platform} but was #{actual_platform}"
next "#{name} was expected to be of platform #{platform || "ruby"} but was #{actual_platform || "ruby"}"
end
if exitstatus == 66
actual_source = out.split("\n").last
Expand Down