Skip to content
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.

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

Fix TruffleRuby no longer installing lockfiles using "ruby" platform correctly #5694

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Jul 6, 2022

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

Until 2.3.17, Bundler would always use the RUBY platform for Truffleruby, so the RUBY platform would always be used in lockfiles generated on Truffleruby.

From 2.3.18, Bundler will consider Gem::Platform.local as Truffleruby's platform, just like CRuby, and use that inside lockfiles.

But we need to properly deal with old lockfiles that used the RUBY platform, because currently Bundler tries to install the most specific variant available and that's usually not expected on Truffleruby.

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

My fix is to use TruffleRuby platform selection logic (according to the gem name) not only for resolution but also for materialization, to make sure we never materialize a lockfile into an incorrect platform specific gem.

Closes #5691.

Make sure the following tasks are checked

Until 2.3.17, Bundler would always use the RUBY platform for Truffleruby, so
the RUBY platform would always be used in lockfiles generated on Truffleruby.

From 2.3.18, Bundler will consider `Gem::Platform.local` as
Truffleruby's platform, just like CRuby, and use that inside lockfiles.

But we need to properly deal with old lockfiles that used the RUBY
platform.

This commit implements that.
@deivid-rodriguez deivid-rodriguez merged commit 759c47f into master Jul 9, 2022
@deivid-rodriguez deivid-rodriguez deleted the truffleruby-issues branch July 9, 2022 09:41
@eregon
Copy link
Contributor

eregon commented Jul 9, 2022

Thank you!
Could you also test that an existing lockfile generated by CRuby and so containing a local platform and even the nokogiri platform-specific gem name installs correctly on truffleruby? (unless that's already tested?)

For instance we initially noticed the issue of #5691 by bundle install in stimulus_reflex which has a checked-in Gemfile.lock:
https://github.com/stimulusreflex/stimulus_reflex/blob/master/Gemfile.lock
with things like:

...
    nokogiri (1.13.6-x86_64-darwin)
      racc (~> 1.4)
    nokogiri (1.13.6-x86_64-linux)
      racc (~> 1.4)
...
PLATFORMS
  x86_64-darwin-19
  x86_64-linux
...

@deivid-rodriguez
Copy link
Member Author

Nice catch. If I had to guess I'd say it doesn't currently work as expected. I will find some time to verify that and try to fix it if necessary.

@deivid-rodriguez
Copy link
Member Author

I had a look and this is indeed still broken. I'm not sure how to best fix it so I created a PR to revert the whole new approach to supporting TruffleRuby using :force_ruby_platform, so that we can get things back to working. I might retry the idea in the future.

@deivid-rodriguez deivid-rodriguez changed the title Fix TruffleRuby no longer installing some lockfiles correctly Fix TruffleRuby no longer installing lockfiles using "ruby" platform correctly Jul 13, 2022
@eregon
Copy link
Contributor

eregon commented Jul 13, 2022

Maybe Bundler could detect the platforms changed, and in that case keep gem versions the same but re-resolve which variant of each gem to use?
Also I would have thought the :force_ruby_platform should force a ruby platform even if the lockfile says otherwise (a bit similar to someone adding force_ruby_platform: true in their Gemfile).

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Jul 13, 2022

Maybe Bundler could detect the platforms changed, and in that case keep gem versions the same but re-resolve which variant of each gem to use?

How can we detect that platforms changed? Unless TruffleRuby reports its own platform (and we record it in the lockfile), I'm not sure how to do it.

Also I would have thought the :force_ruby_platform should force a ruby platform even if the lockfile says otherwise (a bit similar to someone adding force_ruby_platform: true in their Gemfile).

Yes, I expect this too, but the logic is complicated and since I want to release today I had no time to fix it. I can retry "the new way" in the future, but for now I just want things to work again.

@eregon
Copy link
Contributor

eregon commented Jul 13, 2022

How can we detect that platforms changed?

I'm not sure either, but for instance we could detect "only 'ruby' platform" vs "other platforms than 'ruby'".

Unless TruffleRuby reports its own platform (and we record it in the lockfile)

I'm not sure if this would help, but I can consider it if it would help.

Yes, I expect this too, but the logic is complicated and since I want to release today I had no time to fix it. I can retry "the new way" in the future, but for now I just want things to work again.

OK.

@deivid-rodriguez
Copy link
Member Author

I'm not sure if this would help, but I can consider it if it would help.

In the case you reported, the current platform is x86_64-linux and that's in the lockfile, so Bundler thinks there's no need to re-resolve due to platform changes. In the previous implementation, TruffleRuby reports itself as "ruby", so Bundler thinks it needs to re-resolve due to platform changes and that's why things work. But with the "force_ruby_platform" implementation, TruffleRuby also reports the "real platform", x86_64-linux, so things no longer work as expected.

I could've tried to keep the "force_ruby_platform" changes but still let TruffleRuby be reported as "ruby", but again, I just wanted to fix things for now so I reverted the two commits involved.

I will make a proposal if I feel we need TruffleRuby to report something specific as its platform when I retry this in the future 👍.

deivid-rodriguez added a commit that referenced this pull request Jul 13, 2022
Fix TruffleRuby no longer installing some lockfiles correctly

(cherry picked from commit 759c47f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundler 2.3.17 installs precompiled CRuby gems on TruffleRuby
2 participants