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

Move manageiq and plugins to use zeitwerk autoloader instead of classic #22000

Closed
55 tasks done
jrafanie opened this issue Jul 15, 2022 · 2 comments
Closed
55 tasks done

Comments

@jrafanie
Copy link
Member

jrafanie commented Jul 15, 2022

https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#rails-autoloaders

Rails 7 requires zeitwerk for autoloading code and we have numerous code loading problems that may be resolved by moving our plugins and core code to using zeitwerk. We just have to decide how we can split it up and do it.

https://github.com/ManageIQ/manageiq/issues/zeitwerk

% bin/rails zeitwerk:check

https://github.com/rails/rails/blob/cbfe735c69a37446d0c5a9d448ad6d181abd1736/guides/source/classic_to_zeitwerk_howto.md

TODO list:

Phase 1: make our dependencies compatible with zeitwerk

Make core code zeitwerk compatible by enabling eager loading and fixing any problems or marking code for later fix or excluded from zeitwerk. We won't enable eager loading in the app but it helps us find problems. Note, rails engines shouldn't use zeitwerk by themselves as the rails app should handle autoload but they need to be zeitwerk friendly and pass zeitwerk:check.

High level changes:

Individual changes:

Phase 2: enable zeitwerk in application with eager load turned off

Make application code zeitwerk compatible by enabling eager loading and fixing any problems or marking code for later fix or excluded from zeitwerk. We won't enable eager loading in the app but it helps us find problems.

  • Verify include_concern not needed - may be verified in phase one for plugins that use it.
  • Verify require_nested not needed - may be verified in phase one for plugins that use it.
  • Confirm that DescendantLoader is still needed. This is for STI. We need to eager load the STI descendants, otherwise queries will not work correctly.
  • Verify autoload works correctly for main application and plugins
  • Test code reloading in dev mode
  • Verify test suite
@Fryguy
Copy link
Member

Fryguy commented Jul 19, 2022

Hoping this can kill DescendantLoader. 🙏

@jrafanie
Copy link
Member Author

I was considering if it would be easier to enable zeitwerk loader in the plugins first... since they might be easier to get in "compliance" and could guide the direction for these big repositories.

@Fryguy Fryguy added this to the Petrosian milestone Jan 4, 2023
jrafanie added a commit to jrafanie/manageiq that referenced this issue May 31, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this issue May 31, 2023
Hide the details about inspecting the autoloader by using the core method
which will use the interlock with classic and no interlock for zeitwerk.

Depends on: ManageIQ/manageiq#22539
Related: ManageIQ/manageiq#22000
jrafanie added a commit to jrafanie/manageiq-api that referenced this issue May 31, 2023
Hide the details about inspecting the autoloader by using the core method
which will use the interlock with classic and no interlock for zeitwerk.

Depends on: ManageIQ/manageiq#22539
Related: ManageIQ/manageiq#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 1, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 1, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 1, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 2, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 2, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 2, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
jrafanie added a commit to jrafanie/amazon_ssa_support that referenced this issue Jun 5, 2023
Both work but the namespaced name is more friendly to zeitwerk in the future.

Related: ManageIQ/manageiq#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 5, 2023
Both work but the namespaced name is more friendly to zeitwerk in the future.

Related: ManageIQ#22000
jrafanie added a commit to jrafanie/manageiq that referenced this issue Jun 5, 2023
Both work but the namespaced name is more friendly to zeitwerk in the future.

Related: ManageIQ#22000
GilbertCherrie pushed a commit to GilbertCherrie/manageiq that referenced this issue Jul 7, 2023
With zeitwerk, we actually get deadlocks with the use of the interlock.
This change allows us to use the interlock on master when using the classic autoloader and skip the interlock when using zeitwerk.

ManageIQ#22000
GilbertCherrie pushed a commit to GilbertCherrie/manageiq that referenced this issue Jul 7, 2023
Both work but the namespaced name is more friendly to zeitwerk in the future.

Related: ManageIQ#22000
@jrafanie jrafanie reopened this Aug 23, 2023
@Fryguy Fryguy modified the milestones: Petrosian, Quinteros Oct 31, 2023
@Fryguy Fryguy added this to Roadmap Jun 12, 2024
@Fryguy Fryguy moved this to Quinteros in Roadmap Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Quinteros
Development

No branches or pull requests

2 participants