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

Enable Zeitwerk by default on TruffleRuby #40141

Merged
merged 1 commit into from
Aug 30, 2020

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Aug 30, 2020

Summary

Since the Zeitwerk test suite now passes on TruffleRuby, I think it is time to enable it by default when running Rails on TruffleRuby.

I generated a new empty Rails app with this change and it worked fine.
Screenshot from 2020-08-30 13-54-51

In general, I'd like to have as few differences as possible between running Rails on TruffleRuby and running Rails on CRuby.
So that way, it would be possible to switch between both at any time without changing anything (notably, no need to change the Gemfile, configuration, etc), and have minimal behavior changes too.

cc @fxn

@rails-bot rails-bot bot added the railties label Aug 30, 2020
@fxn
Copy link
Member

fxn commented Aug 30, 2020

Fantastic! 🎉

@fxn fxn merged commit 9d6339f into rails:master Aug 30, 2020
@fxn
Copy link
Member

fxn commented Aug 30, 2020

Would that need a release I guess?

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

@fxn A release of TruffleRuby or a release of Rails?

Re TruffleRuby the next release is planned on Nov 17. I don't think we need to worry if people use the 19.2 release instead, the basics of Zeitwerk should still work fine, and for Rails we'd still recommend to use truffleruby-head for now as there are lots of performance changes in master recently that matter for Rails.

Re Rails releases I think we can just wait for the next release :)

@fxn
Copy link
Member

fxn commented Aug 30, 2020

Question basically is: This patch enables the mode for any release of TruffleRuby. So, people upgrading to 6.1 applications running on a stable release of TruffleRuby today will get the mode silently switched. People creating new 6.1 apps with a previous version of TruffleRuby will have the mode enabled in a version that does not pass the suite.

However, I guess TruffleRuby users are vigilant enough that I was thinking about adding a CHANGELOG entry like:

zeitwerk mode is enabled by default with TruffleRuby. Please make sure you are running at least TruffleRuby such and such.

On the assumption that in practice nobody would be running on a previous version. But do not know what to say instead of "such and such".

Alternatively, we could revert this patch and document that you can enable zeitwerk mode yourself if running truffleruby-head.

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

I think the CHANGELOG entry makes sense, I'll make a PR.

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

#40144

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

We could also check the TruffleRuby version if you want, like here.
But it makes it harder for the user to know which autoloader is used.

IMHO almost every behavior difference between running on TruffleRuby and CRuby is essentially a bug. So here we are fixing that behavior difference, and previous Rails versions simply had the unexpected difference.

@fxn
Copy link
Member

fxn commented Aug 30, 2020

We could also check the TruffleRuby version if you want, like here.
But it makes it harder for the user to know which autoloader is used.

Yeah.

On a second thought, perhaps it is too early to enable this. Maybe a better first step would be to document that you can enable zeitwerk mode if running truffleruby-head, and wait just a bit until everything is more mature.

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

On a second thought, perhaps it is too early to enable this. Maybe a better first step would be to document that you can enable zeitwerk mode if running truffleruby-head, and wait just a bit until everything is more mature.

I don't like that, because it makes a significant difference between running on CRuby and on TruffleRuby, and Rails 6 users typically can assume Zeitwerk is used and don't manually set the autoloader. Also very few people would actually find that in the docs.

I can make a PR with the version check if you want to make sure nothing changes when running on older TruffleRuby.

@fxn
Copy link
Member

fxn commented Aug 30, 2020

I don't like that, because it makes a significant difference between running on CRuby and on TruffleRuby, and Rails 6 users typically can assume Zeitwerk is used and don't manually set the autoloader. Also very few people would actually find that in the docs.

Well, the fact that Zeitwerk works only in CRuby is well documented in the upgrade notes and the autoloading guide. It does not need a version check because Zeitwerk supports the Ruby version required by Rails 6.

Therefore, if you run Rails 6 on JRuby and think you're running in zeitwerk mode, that is not something we can do much about. And what we cannot certainly do is to let you run on zeitwerk mode by default on an interpreter that does not support it. On the other hand, you have the means to override the default.

I merged assuming a release was about to be out and we could document or something, but if this is only to be valid on TruffleRuby's main branch for a while, I believe I need to ponder this again. Let me think.

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

Let's check the TruffleRuby version and be on the safe side then: #40145

@fxn
Copy link
Member

fxn commented Aug 31, 2020

Re TruffleRuby the next release is planned on Nov 17. I don't think we need to worry if people use the 19.2 release instead, the basics of Zeitwerk should still work fine, and for Rails we'd still recommend to use truffleruby-head for now as there are lots of performance changes in master recently that matter for Rails.

Based on this comment, and after thinking about this for a while, I believe I prefer to leave the check as is. In normal circumstances, one should play by the book and control things precisely. But, due to the nature of the project and usage patterns, I think we can be more optimistic with TruffleRuby and keep it simple.

The only tweak I've done is moving the check to the 6.1 defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants