Support zeitwerk & clasissic both on Rails 6 #64

eudoxa · 2022-11-19T07:49:26Z

現状

Rails6以上のzeitwerkを利用した環境ではproductionのeager_loadが正しく機能せず、Unitの読み込みでNameErrorが発生する。

再現

Rails 6.1.7
Chanko 2.2.1

Sampleユニットを定義後、RAILS_ENV=productionで起動時に下記のエラーが発生する

D, [2022-11-21T13:41:52.850574 #92788] DEBUG -- :   [Chanko] NameError - uninitialized constant Sample
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/activesupport-6.1.7/lib/active_support/inflector/methods.rb:274:in `const_get'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/activesupport-6.1.7/lib/active_support/inflector/methods.rb:274:in `constantize'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/activesupport-6.1.7/lib/active_support/core_ext/string/inflections.rb:74:in `constantize'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:66:in `constantize'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:43:in `load_from_file'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:29:in `load'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:7:in `load'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:16:in `block in eager_load_units!'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:15:in `each'
  [Chanko]   /Users/morita/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/chanko-2.2.1/lib/chanko/loader.rb:15:in `eager_load_units!'

調査結果

現在、zeitwerkの環境下のinitializer("chanko.eager_load_units")では、autoload_pathsを追加せずにconstantizeを実行します。この変更は下記のzeitwerk対応PRで適用されました。
#57

しかしconstantize前に実行が必要なzeitwerkのinitializer "eager_load!"はFinisherで定義されており、通常のinitializerよりもあとに実行されます。

https://github.com/rails/rails/blob/main/railties/lib/rails/application.rb#L429-L433
https://github.com/rails/rails/blob/main/railties/lib/rails/application/finisher.rb#L71-L83

つまり、このタイミングはeager_load!の実行前のため、その後のconstntaizeに失敗します。

対応

zeitwerk(≒Rails6)で動かすため、下記の対応を行います。

Railsの各バージョンに対応したSpecの実行

Zeitwerk対応に先立ち、Rails5及び6でテストが正しく動かない状態にあったため、Gemfileを切り分け、テストを実行可能にします。

CIについては下記のアドバイスを参考に、github workflowsを利用します。
#65 (comment)

Zeitwerkでは自前のEager loadを行わない

このPRを作るきっかけとなった問題への対応としては、自前のEager loadを行わないようにします。これは以下の二つを理由に可能です。

zeitwerkではcollapseを指定することでNamespaceのコントロールが可能であり、先のPRですでに設定されています。
https://github.com/fxn/zeitwerk/blob/main/lib/zeitwerk/loader/eager_load.rb#L164-L179
現在の自前Eagar loadは、Unitの_spec.rbを自動読み込みすることを防ぐ目的で導入されました。Zeitwerkではignoreを使うことで問題を回避できます。
May need handmade eager-loading logic in production #38
https://github.com/fxn/zeitwerk#ignoring-parts-of-the-project

その他の修正

initializerに順番を指定
spec内のActionViewとControllerの生成方法変更
条件分岐が増えてきたのでLoaderの実態をClassicLoaderとZeitwerkLoaderに分離
SimpleCov & Coverallsのgithub actions対応
- 現在出ているset-outputのwarningはこちらの対応を追いかけます
  The set-output command is deprecated and will be disabled soon. coverallsapp/github-action#135
環境変数EAGER_LOADとAUTOLOADERによるテスト環境の切り替え
Chanko::Config.eager_loadを廃止しRails.configuration.eager_loadを参照
Rails.root外のunits_directory_pathに対応

修正概要

コードの修正

Chankoのzeitwerkへの対応: 8eb5202
Specの修正: d02f72c

CIの対応等

各Railsに対応するGemfileの用意: 9f1a9c9
github workflowsのファイルを追加: 57ff356
開発支援ファイル追加: 9e18e76
不要になったtravis.ymlの削除: af38ba2

動作確認

Rails: 6.1.7 Ruby: 2.7.6 & 3.1.2
Rails: 5.2.8.1 Ruby: 2.7.6

お願い

github workflowsの設定については自分のPRで確認をしました。
eudoxa#1
https://github.com/eudoxa/chanko/actions/runs/3540408482/jobs/5943423013

cookpad/chankoでは実行されないようなので、Settingsを見られる方に有効かどうか確認をしていただきたいです。
Coverallsの認証周りがすんなり動くかも分からないので、結果をみつつ対応をします。

TODO

Rails 7についてはまだ未確認

やること

各Rails & Rubyバージョンごとにテストを実行できるようにする
travisからgithub workflowsへの以降
zeitwerkモードへの対応
Autoloader zeitwerk/classic のテスト対応
EagerLoad on/off のテスト対応
Coverallsが動かないので修正 + github workflowsへの対応

r7kamura · 2022-11-24T21:48:32Z

Rails6以上のzeitwerkを利用した環境ではproductionのeager_loadが正しく機能せず、Unitの読み込みでNameErrorが発生する。

という問題への対策を入れようとしているので、eager_loadが有効化された状態でUnitが読み込めることが保証されるテストになっていると良いと思いました。

テスト時もeager_loadを有効化しておくのはどうでしょうか？最近のRailsの雛形だとtest環境で config.eager_load = ENV['CI'].present? となっているので、テスト時にeager_loadを有効化しておくことは不自然ではないと思います。

実際にeager_loadを有効化してみると、Unitが正常に読み込まれずRSpecが失敗するように見える (Unitの定数を探索しようとして NameError が起きる) ので、何かまだ実装に問題があるか、あるいはテストでの設定に不足があるかもしれません。

eudoxa · 2022-11-25T00:03:34Z

ありがとうございます。おっしゃる通りですね。eager load = trueのみのテストにすると、今度は開発モードのバグが残ってしまうので、両方をテストできる方法を考えますね。

実際にeager_loadを有効化してみると、Unitが正常に読み込まれずRSpecが失敗するように見える (Unitの定数を探索しようとして NameError が起きる) ので、何かまだ実装に問題があるか、あるいはテストでの設定に不足があるかもしれません。

調べたところ、テストではunitsディレクトリがapp/外に設定されている事が原因でした。
InitializerのどこかでChanko::Config.units_directory_pathを事前に設定する必要がありそうです。

また、zeitwerk環境ではeager_loadの挙動をRails側と同期させずに挙動を担保することが難しいため、Rails.config.egar_loadを参照するように変更しようと思います。

eudoxa · 2022-11-25T01:46:58Z

一旦Draftに戻します。

eudoxa · 2022-11-25T06:05:30Z

環境変数 EAGER_LOAD=true/falseとAUTOLOADER=zeitwerk/classicでテスト環境を切り替えられるようにしました。
テストの修正も終えたためReady for reviewに戻します。

関連コミット: d02f72c
自分のリポジトリでのテストの実行結果: https://github.com/eudoxa/chanko/actions/runs/3545729845/jobs/5954106391

eudoxa · 2022-11-25T08:21:24Z

spec/chanko/invoker_spec.rb

-        include Chanko::Helper
-        include Chanko::UnitProxyProvider
-      end.new
+      controller.helpers


この変更は、LookupContextのCacheの誤動作(compiled_method_containerが違うviewとcontrollerで共有されてしまう)への対処です。

他の手法として、Cacheを無効化してもうまくいくかもしれません。別のテストファイルへの影響が把握しきれていないこともあり追加で調べる予定ですが、少し先になりそうです。

r7kamura · 2022-11-26T00:46:51Z

ReadonlyなCollaboratorの観点でレビューすると、コード的には全体的にかなり良さそうでした。

普通のOSSのライブラリとして見ると下記の点が気になりましたが、この辺りは運用する側のポリシーや労力の掛け方に大きく依存する話だと思うので、Memberの意見次第だと思っています。

互換性をどの程度気にするか
- 例えば eager_load の設定項目は外部から利用されている可能性が高いが、ここで廃止することを良しとするのか
- テスト対象のRubyとRailsのバージョンの選定基準はこれで良いのか
- 次回のリリースはバージョンをどれくらい変更すべきか
Pull Requestの粒度はこれで良いか
- 上の互換性の話を細かくやりたいなら、変更を分けて個々のPull Requestで議論できるようにした方が良いかもしれない
- 分けるのは正直かなり面倒そう

個人的には、cookpadの内部で上手く動くなら現状このPull Requestの対応で十分だろうと思っています。

eudoxa · 2022-11-28T01:51:52Z

ありがとうございます。記録として論点についての考えを残しておきます

例えば eager_load の設定項目は外部から利用されている可能性が高いが、ここで廃止することを良しとするのか

下記の3点で妥当だと思います。

Chankoのeager_loadのみを有効にしたいケースが思いつかない
zeitwerk時に自前のeager_load実装は避けたい
自前実装の理由は、非zeitwerk環境下でSpecファイルの自動読み込みを除外するためである

テスト対象のRubyとRailsのバージョンの選定基準はこれで良いのか

こちらについては社内で意見を聞いているのですが、今の所Rails4は切り捨てる予定です。理由としては

自分達が利用していない
過去のバージョンを利用すれば動く
古い

次回のリリースはバージョンをどれくらい変更すべきか

まだ考えていません

上の互換性の話を細かくやりたいなら、変更を分けて個々のPull Requestで議論できるようにした方が良いかもしれない

仕様変更とバグ対応が密接に絡んでおり複雑なので諦めました

uasi

Workflow を動かすために仮で approve します。

後述

lib/chanko/config.rb

chanko.gemspec

lib/chanko/loader.rb

spec/dummy/config/application.rb

lib/chanko/railtie.rb

lib/chanko/loader.rb

uasi · 2022-11-28T18:34:35Z

cookpad/chankoでは実行されないようなので、Settingsを見られる方に有効かどうか確認をしていただきたいです。

設定では有効になっています。 master に .github/workflows ない場合 fork から送った PR では workflow が動かない気がします。

eudoxa · 2022-11-29T12:23:22Z

@uasi
指摘いただいたegar_load=nilの問題について、各バージョンの挙動を確認し、Initializerの実行タイミングを分けました。
ff4b332

分かりづらい箇所は、Rails6.0(classic)と6.1(classic)のeager_loadの挙動の違いです。これは下記の変更に起因しています。
rails/rails#37331

また、問題を検知出来るように、処理時にconfig.eager_loadがnilの場合は例外を出すようにしました。

	Rails5	Rails6.0(classic)	Rails6.0(zeitwerk)	Rails6.1(classic)	Rails6.1(classic)
Freeze	false	false	true	false	true
eager_load	nil	nil	nil	Boolean	Booelan

(eager_load == set_autoload_paths時点のconfig.eagerload)

uasi · 2022-11-29T16:45:56Z

spec/chanko/loader_spec.rb

-          described_class.load(:example_unit)
-          described_class.load(:example_unit)
+        it "load unit from cache", classic: true do
+          expect_any_instance_of(Chanko::Loader::ClassicLoader).to receive(:load_from_file).and_call_original


[nit] cache からロードされることをテストするのであれば expect_any_instance_of(Chanko::Loader::ClassicLoader).to receive(:load_from_cache).and_call_original も追加したほうがいいと想います。

ありがとうございます。PRが肥大化する中なので、既存の改善についてはマージ後に別PRでだしますね。expect_any_instance_of自体も現在は非推奨なので、使わない形に書き換えられたらと思っています。

lib/chanko/loader.rb

eudoxa · 2022-11-30T06:28:33Z

今後の作業

. merge後のリビジョンでcookpad/chankoにrails6ブランチとして一旦登録
"when loader has ever loaded specified unit"の修正
並行してREADMEの編集を含めたbunmupのPRを作成
bumpup依頼(社内対応)

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch 2 times, most recently from dd2dd6e to df336d7 Compare November 22, 2022 02:03

eudoxa changed the title ~~fix eager load on zeitwerk~~ Support zeitwerk on Rails 6 Nov 24, 2022

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch 4 times, most recently from 7c5783f to d5fc057 Compare November 24, 2022 12:34

eudoxa marked this pull request as ready for review November 24, 2022 12:40

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch 2 times, most recently from fcd61a3 to 3bf7bb6 Compare November 24, 2022 13:06

eudoxa marked this pull request as draft November 25, 2022 01:47

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch 2 times, most recently from 27a3f83 to af38ba2 Compare November 25, 2022 05:59

eudoxa marked this pull request as ready for review November 25, 2022 06:07

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch 2 times, most recently from f751ed7 to 7e6d39c Compare November 25, 2022 06:13

support eager load on zeitwerk

9495328

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch from 7e6d39c to f09d0d1 Compare November 25, 2022 06:16

eudoxa added 5 commits November 25, 2022 15:17

Support and fix to run specs on each Rails version

4a64017

parepare Gemfile for each rails version

9f1a9c9

add ci on github workflows

48445e9

add setup up and test bin files

5391cde

remove unused .travis.yml

68b7864

eudoxa force-pushed the fix_eager_load_on_zeitwerk branch from f09d0d1 to 68b7864 Compare November 25, 2022 06:17

eudoxa commented Nov 25, 2022

View reviewed changes

eudoxa requested a review from uasi November 25, 2022 10:33

eudoxa requested a review from kentarohorie November 25, 2022 10:33

uasi previously approved these changes Nov 28, 2022

View reviewed changes

uasi marked this pull request as draft November 28, 2022 09:14

uasi marked this pull request as ready for review November 28, 2022 09:14

uasi self-requested a review November 28, 2022 09:15

uasi requested changes Nov 28, 2022

View reviewed changes

eudoxa added 8 commits November 29, 2022 13:41

initializer distinguishes between String and Symbol

405e72a

fix wrong text

abfdbf5

sort gem files

dd95d95

return abolute path if rails root join given absolute path

10f35bf

return symbol for compatibility

6921fee

pathname dont have to_sym

1d7d68b

fix autoload initializer on classic mode

ff4b332

merge zeitwerk initializer because of similar processes

005e906

uasi reviewed Nov 29, 2022

View reviewed changes

eudoxa changed the title ~~Support zeitwerk on Rails 6~~ Support zeitwerk & clasissic both on Rails 6 Nov 30, 2022

uasi approved these changes Nov 30, 2022

View reviewed changes

lib/chanko/loader.rb Outdated Show resolved Hide resolved

fix typo

3c7e603

eudoxa merged commit fe6e6bb into cookpad:master Nov 30, 2022

This was referenced Nov 30, 2022

When using zeitwerk, eager_load fails in production mode #61

Closed

refactor loader and related spec #67

Merged

eudoxa mentioned this pull request Apr 25, 2023

Rails6(開発環境)において、Viewファイル保存後のリロードで、Unit側のViewファイルの呼び出しが失敗する #69

Closed

eagletmt mentioned this pull request Oct 4, 2023

Do not include setup/test command as executable #72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support zeitwerk & clasissic both on Rails 6 #64

Support zeitwerk & clasissic both on Rails 6 #64

eudoxa commented Nov 19, 2022 •

edited

Loading

r7kamura commented Nov 24, 2022

eudoxa commented Nov 25, 2022 •

edited

Loading

eudoxa commented Nov 25, 2022

eudoxa commented Nov 25, 2022 •

edited

Loading

eudoxa Nov 25, 2022 •

edited

Loading

r7kamura commented Nov 26, 2022

eudoxa commented Nov 28, 2022 •

edited

Loading

uasi left a comment

uasi commented Nov 28, 2022

eudoxa commented Nov 29, 2022 •

edited

Loading

uasi Nov 29, 2022

eudoxa Nov 30, 2022 •

edited

Loading

eudoxa commented Nov 30, 2022 •

edited

Loading

Support zeitwerk & clasissic both on Rails 6 #64

Support zeitwerk & clasissic both on Rails 6 #64

Conversation

eudoxa commented Nov 19, 2022 • edited Loading

現状

再現

調査結果

対応

Railsの各バージョンに対応したSpecの実行

Zeitwerkでは自前のEager loadを行わない

その他の修正

修正概要

コードの修正

CIの対応等

動作確認

お願い

TODO

やること

r7kamura commented Nov 24, 2022

eudoxa commented Nov 25, 2022 • edited Loading

eudoxa commented Nov 25, 2022

eudoxa commented Nov 25, 2022 • edited Loading

eudoxa Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

r7kamura commented Nov 26, 2022

eudoxa commented Nov 28, 2022 • edited Loading

uasi left a comment

Choose a reason for hiding this comment

uasi commented Nov 28, 2022

eudoxa commented Nov 29, 2022 • edited Loading

uasi Nov 29, 2022

Choose a reason for hiding this comment

eudoxa Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

eudoxa commented Nov 30, 2022 • edited Loading

eudoxa commented Nov 19, 2022 •

edited

Loading

eudoxa commented Nov 25, 2022 •

edited

Loading

eudoxa commented Nov 25, 2022 •

edited

Loading

eudoxa Nov 25, 2022 •

edited

Loading

eudoxa commented Nov 28, 2022 •

edited

Loading

eudoxa commented Nov 29, 2022 •

edited

Loading

eudoxa Nov 30, 2022 •

edited

Loading

eudoxa commented Nov 30, 2022 •

edited

Loading