Fix platform specific version for libv8-node and other allowlisted gems not being chosen in Truffleruby #6169
Conversation
deivid-rodriguez
force-pushed
the
truffle
branch
2 times, most recently
from
1b4b59a
to
f0e65dc
Compare
By using the `force_ruby_platform` dependency attribute and making it have the right value according to the dependency name on truffleruby. We actually had a spec for this case, but was not catching the problem because Truffleruby monkeypatches RubyGems with ``` Gem.platforms = [Gem::Platform::RUBY] ``` and our previous implementation of the `simulate_platform` helper was undoing that. By fixing the helper, our existing specs start covering the issue properly.
deivid-rodriguez
force-pushed
the
truffle
branch
from
f0e65dc
to
0b1edc0
Compare
deivid-rodriguez
changed the title
|
|
||
| if RUBY_ENGINE == "truffleruby" && !defined?(REUSE_AS_BINARY_ON_TRUFFLERUBY) | ||
| REUSE_AS_BINARY_ON_TRUFFLERUBY = %w[libv8 libv8-node sorbet-static].freeze | ||
| end |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
endin defaults/truffleruby then?
I can do that soon.
Or were you thinking of something else?
There was a problem hiding this comment.
Yup, that's what I was thinking.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Depending on #6238 (comment) we might need to remove the constant
There was a problem hiding this comment.
Yep, you shouldn't need remove_const because of that.
deivid-rodriguez
changed the title
|
Thank you for the fast fix! |
What was the end-user or developer problem that led to this PR?
This was supposed to have been fixed, but Bundler was still not able to choose the precompiled version of those gems that truffleruby has deemed as having precompiled versions that work fine with truffleruby.
This reintroduces the approach in 6085de0, which was reverted at #5711 because it caused some issues. Unfortunately the revert created the original problem again, due to a problem with the spec initially added that was not able to properly cover the fix.
What is your fix for the problem, implemented in this PR?
Reintroduce the same approach of setting a proper default value for the
:force_ruby_platformdependency attribute, and to locked specifications. This approach no longer seem to cause any issues.Also fix our specs to properly cover the issue.
Fixes #6165.
Make sure the following tasks are checked
WriteFix tests for features and bug fixes