[Hotfix] Remove invalid comment in ini file for celestial rotation

suzuki-toshihir0 commented 9 months ago

Related issues

N/A

Description

Details on Bug

Rotation mode for the Earth and Moon was not set correctly at s2e-core v7.2.2 at VS2022 on Windows.
As you see in the following figure, earth_rotation->rotation_mode_ is kIdle (0), though we intend to set kFull.
This problem occurs only at VS2022 on Windows.

image (1)

Cause of Bug

IniAccess::ReadVectorString (in the same way as IniAccess::ReadString) cannot identify // style comment.
We cannot use // style comment at EOL when IniAccess::ReadString and IniAccess::ReadVectorString are used as parser.

Treatment in this PR

I removed // style comment at EOL for the setting of celestial rotation.

Test results

earth_rotation->rotationmodeis set askFull (2)` correctly.

image (2)

Impact

Large (Windows user only): We cannot conduct verification related to ECI <--> ECEF transformation.

Supplementary information

N/A

suzuki-toshihir0 commented 9 months ago

以下のissueとも関連するので、一応参照しておきます。

254
549

200km commented 9 months ago

私の手元で最新develope16b5d2をコード、iniファイルの修正なしで動かした場合、問題ないように見えています。バグ再現のための詳細情報をもらえると嬉しいです。

suzuki-toshihir0 commented 9 months ago

@200km Descriptionに書いているとおり、S2E coreのバージョンはv7.2.2です。念のためdevelopでも試しましたが再現しました。環境はVS2022 on Windowsです。

suzuki-toshihir0 commented 9 months ago

WIN32環境では GetPrivateProfileStringA を使ってiniファイルからの文字列読み出しをしていますが、そうでない環境では INIReader::GetString を使って文字列読み出しをしていることが根本的な挙動の違いであると考えています。

200km commented 9 months ago

Descriptionに書いているとおり、S2E coreのバージョンはv7.2.2です。

この情報を元にコードとしては同じ最新developを使いましたが、再現しなかったので、それ以外のより細かい情報をくださいという意図です。
iniファイルの修正などが行われているわけではないのでしょうか？

200km commented 9 months ago

Windowsで動かしているかどうかが問題だと考えられているということでしょうか？

suzuki-toshihir0 commented 9 months ago

他の修正はいっさい行っていません。

suzuki-toshihir0 commented 9 months ago

Windowsで動かしているかどうかが問題だと考えられているということでしょうか？

そのとおりです。

200km commented 9 months ago

わかりました。descriptionにwindows限定の問題だと明記してもらえると助かります。
また、根本原因解決のためにはiniファイルの修正ではなく、windows側の修正をした方が良いのではないでしょうか？

suzuki-toshihir0 commented 9 months ago

WIN32以外の環境では library/external のINIReaderを使っていますが、ここで以下のマクロを定義しており、明示的に // スタイルのコメントを許容するように設定しており、これが根本的な挙動の違いだと思っています。

https://github.com/ut-issl/s2e-core/blob/e16b5d241b0166a5d996176402225f9782e43ebb/src/library/external/inih/ini.h#L94

suzuki-toshihir0 commented 9 months ago

descriptionにwindows限定の問題だと明記して

わかりました。

windows側の修正

これはどういうものを想定されていますか？

200km commented 9 months ago

これはどういうものを想定されていますか？

Windows環境でも他と同じように//　をコメントとして認識するように修正できると良いと思います。document的には//　をコメントとして定義していますので、それができていないwindows側のコードが問題な気がしています。

200km commented 9 months ago

関連質問ですが、windowsの人は下記部分などでもコメントアウトできずにおかしくなるのでしょうか？それとも、数字だから大丈夫で、文字列を読もうとすると問題になるのでしょうか？

https://github.com/ut-issl/s2e-core/blob/e16b5d241b0166a5d996176402225f9782e43ebb/data/sample/initialize_files/sample_simulation_base.ini#L37

suzuki-toshihir0 commented 9 months ago

本来iniファイルのコメントとして許容されているスタイルは ; だけであり、これまで文法的に誤っている // でも問題があまり表面化してこなかったのは

Windowsで使われる GetPrivateProfileStringA などはコメントも含めて文字列を拾ってしまうが、string --> doubleの暗黙の型変換でたまたま救われているだけである

https://github.com/ut-issl/s2e-core/blob/e16b5d241b0166a5d996176402225f9782e43ebb/src/library/initialize/initialize_file_access.cpp#L57

（これは以下の実行例を見るとわかりやすいです）

WIN32以外の環境では上で書いたとおり、// を何故か認めるような設定がされている

ということだと理解しています。標準のiniファイルの仕様をあえて逸脱するような修正は適切ではないと考えています。

suzuki-toshihir0 commented 9 months ago

今回のPRでは手軽な修正を提案していますが、根本的な修正をかける場合、iniファイルの標準の仕様に合致させる以下の改修をやるべきではないでしょうか？

https://github.com/ut-issl/s2e-core/issues/549

200km commented 9 months ago

私からみた経緯としては、下記のような感じです。

s2eはdocumentに書いている通り、昔からiniファイル内の//はコメントアウトとして使うと定義していた
これが標準のiniファイルではないとしても、S2Eのiniファイルの定義としては、長く定着してきた（実際、現状のiniファイルにはこのスタイルが適用されて運用されている）
この状況でほとんど問題は出ないが、Windowsユーザーがstringを読み込もうとした時限定で問題が出ることがわかった

この問題を解決するための方法は次のいずれかがあるように思います。

A. iniファイルで、Windowsユーザー向けにstring + //という表記を禁止する。（このPR）
B. Windowsユーザーもstring + //という表記ができるようにコードを修正する。（私はこれが良いと思っている）
C. そもそもS2Eのiniファイルの定義というのを止めて、標準のiniファイルに準拠するため、iniファイル内のすべての//を禁止する (#549)

鈴木くんの提案としては、A or Cということのようですが、次のような問題があると思います。

Aはstringか数字かでiniの表記ルールを変えるという人間の作業が入りミスが起きやすそう。特にwindowsでないユーザーはミスに気づけないので、これを放置するのは良くなさそう。（実際、rotationのPR時には、windows以外の環境では正しく動作していたので、レビューをすり抜けている）
Cは最終的にはこれを目指した方が良いかもしれないが、S2E全体の方針の大変更になるので、やるのは時間がかかり、また今やりたいかと言われるとそうでもない。

Bは今までのS2Eの仕様を維持しつつ、クイックに修正でき、今後も問題が発生しづらくなるという意味で現時点では適切な修正なのではないかと思います。

200km commented 9 months ago

ReadString関数内で、すでに特別文字のハンドリングをしています。これと似たようなものをWindows限定で、//に対してやれば修正できたりしませんかね。

https://github.com/ut-issl/s2e-core/blob/e16b5d241b0166a5d996176402225f9782e43ebb/src/library/initialize/initialize_file_access.cpp#L163

suzuki-toshihir0 commented 9 months ago

このPRで本当にやりたいことは iniファイルで、Windowsユーザー向けにstring + //という表記を禁止する ではありません。あくまでBやCのような（人間の作業が入り込む余地のない）根本的な変更をやりたいものの、直近でECI <--> ECEF変換に失敗しているという実用上の問題が出ている以上、手早く当てられるパッチとしてひとまずこのPRを出しています。
Cの変更に対する「S2E全体の方針の大変更になるので、やるのは時間がかかり」は、本当でしょうか？実際の作業は find data/sample/initialize_files/ -type f -exec sed -i 's|//|;|g' {} \; をするだけでよく、migration上の困難はほぼないと思っています。
- S2Eの方針の変更、という意味でも、コメントスタイルに明確な根拠（iniファイルの仕様）を持たせるという変更であり、妥当性の観点からすんなりと受け入れられないでしょうか？
- 今やりたいか、という話についても、現に問題が見つかっているいまこそやるべきであるとも捉えられると思っています。

200km commented 9 months ago

実用上の問題について

手早く当てられるパッチが必要なのは理解します。その手段として、Aのiniファイルを修正するのか、Bのコードを修正するのかのどちらかがあると思います。

Aのiniファイルを修正する方は、非明示的でありますが、documentに書いてあるコメントとして//を使うというのが実はwindowsユーザーのstringには当てはまらないから気をつけてねというように対処しているように見えます。なので、このPRを受け入れてBをしないということになると、S2Eのiniファイルの仕様を変えたように見えると私は思っています。
Bのコード修正は、documentに書いてあるコメントとして//を使うという仕様を違反するバグを修正するということなので、仕様は変わらずパッチを適切に当てていると言えると思います。

なので、Bをやった方が良いのでは？と提案しています。

また、ここで修正提案されているのはあくまでsample_simulation_base.iniです。実利用しているユーザーは、ユーザー独自のレポジトリで各種iniファイルを管理する方針です。ここでsampleのiniファイルが修正されても、s2e-coreのバージョンアップデートではユーザーは問題を解決できません。ユーザーがそれを取り込むためには、自身のiniファイルを別途修正しなければなりません。

また、そもそもユーザーの独自iniファイルでは、コメントをつける必要すらないかもしれません。このコメントはあくまで、sampleとして使い方を表示するためのもので、ユーザーがコメントなしでrotation_mode(0) = FULLと書いても何も問題ないです。

iniファイルの仕様を今まで通りに維持するのか
ユーザーへの波及効果

などを考えて、Bが適切だと思っています。Bの作業に時間がかかるなら、AのPRを通すのではなく、issueを立ててユーザーに「windowsでこういうバグがあります。回避するためにはユーザーiniファイルをこう修正してください」と伝えるのが良い気がします。

200km commented 9 months ago

「S2E全体の方針の大変更になるので、やるのは時間がかかり」は、本当でしょうか？

こちら、時間がかからないという提案ありがとうございます。やり方がわかっているなら、ぜひPRを出してもらえると嬉しいです。

ただ次のような懸念があり、コーディング作業以外の検証などで時間がかかる可能性があることもご考慮ください。

iniファイルの修正が入るので、Major updateになり、リリースタイミングは難しくなること
iniファイルほぼ全てに修正が入るので、検証項目が膨大になり得ること

また前述の通り、s2e-coreのsampleのiniファイルが書き換えられたとしても、全ユーザーは自分自身でiniファイルを修正する必要があります（Major updateなので当然）。

suzuki-toshihir0 commented 9 months ago

Aの提案をやるのであればS2Eの仕様を守るために必ずBとセットになるので、その場合はBを実施すべきなのでは？という話は理解しました。
とはいえ、Bに比べてCのコストが膨大であるとはやはり考えにくいです。
- さきほどの私のコメントでshellのコマンドを貼りましたが、それを ユーザー含めて 対応させるとしても、コマンド一発で済む以上、大きな労力にはならないと思います。
- 私から見えている範囲で、かつs2e-coreのバージョンアップデートに対応させる必要があるリポジトリは https://github.com/ut-issl/s2e-aobc , https://github.com/ut-issl/s2e-ff , org内のprivate repo 1個であると思っていて、それに対しては（C案を取るなら）私の方で責任を持ってアップデート対応します。
- インターフェースの変更がある以上、major updateになるためリリースタイミングが遅れることも理解します。
- 検証項目ですが、結局は IniAccess classに定義されている関数のテストを書くということになるかなと思っています。

200km commented 9 months ago

Aの提案をやるのであればS2Eの仕様を守るために必ずBとセットになるので、その場合はBを実施すべきなのでは？

私の言いたいことと違いますね。
Bをやるのであれば、Aはやる必要はないです。なぜなら、Aをやってもやらなくても現状のS2Eの仕様に合っているからです。仕様を逸脱しているのは、コードの方でそこにパッチを当てるべきということです。

Aだけやる場合は、「とりあえずs2e-coreを直接動かす初期チュートリアルユーザーは救われる」とは思いますが、独自ユーザーレポジトリを使っている多くのユーザーは別に救われず、自分でiniファイルを修正する必要があります。この「自分でiniファイルを修正する」というのは、AのPRが通っていても通らなくてもユーザーはできることで、多くのユーザーにとってAのPRは特に意味がないと思っています。
また、文字列のパラメータは今回のrotation以外でも多く使われています。s2e-coreのsampleでは、「文字列 + //コメント」になっていないけれど、ユーザーレポジトリのiniの中で別の場所でユーザーが「文字列 + //コメント」を使ってしまう可能性もあります。これはAのPRでは防げず、issueなどを立ててユーザーに注意喚起する必要があると提案しています。

Bだけやる場合は、ユーザーがs2e-coreのバージョンアップデートを行えばよく、今までの仕様のまま特に何も気にせずに進めることができるはずです。これがBの一番のメリットです。Aもやって、さらにBもやるというのは特に意味はなく、Bだけやれば良いはずです。

200km commented 9 months ago

Bに比べてCのコストが膨大である

これは仕様を今までから変えるか、変えないかという話です。Bは仕様の変更はなく、Cは仕様の変更が入ります。仕様の変更が入る場合、次のようなことをしっかり議論したいです。（どれも一理ある気がしていて、私はすぐには決めれず対面で話し合いたいです）.

別にS2E独自の仕様として、iniファイル内で//をコメントとして扱うことを認める仕様でも良いのでは？（;を使うよりわかりやすくないか？）
iniファイル使っているなら、その標準仕様に絶対に合わせるべきだ
そもそもiniファイルやめて他のファイルにしよう

ユーザー含めて対応させるとしても、コマンド一発で済む

どんなユーザーを想定するかというのが難しいので、必ずしも一発で済むわけではない気がします。例えば、iniファイルを自動生成するコードを使ってs2eを動かしているユーザーさんもすでに存在しています。そのプロジェクトにとっては、コメントの仕様が変わるのは、大きなことでコマンド一発で済むわけではないです。
Major updateでそういうことが起こるのは仕方ないので、必要があればそういう変更はするべきだと思いますが、今回この件のためにやるのかというのはちょっと違う気がしています。（追記）より正確に書くと、それくらいMajor updateは重要なので、上に書いたようなことをしっかり話し合いたいということです。

200km commented 9 months ago

とりあえず、issueは作りました。

https://github.com/ut-issl/s2e-core/issues/576

suzuki-toshihir0 commented 9 months ago

AはあくまでサンプルのPRで、後追いでその他のユーザーサイドリポジトリに撒く予定であったことはお伝えします。
- いっぽう、Bの改修であればそのような改修はもとから不要であるということも理解しました。ありがとうございます。
iniファイル内で // をコメントとして扱うことを認める仕様でも良いのでは？ という件については、いかなる処理系であっても漏れなく // をコメントとして扱うと保証できるのでしょうか？実際本来いれるべきだった特殊な処理を入れて忘れていたことで今回のような問題が起きているとも思っています。処理系によっては https://github.com/ut-issl/s2e-core/pull/575#issuecomment-1899820508 のような暗黙の型変換すらして正しくしてくれないかもしれません。標準のコメントスタイルにあえて従わないことによる不都合が今後起きないのかどうかを懸念しています。
- // が ; よりわかりやすいとは思っていません。むしろ標準のコメントスタイルに慣れたユーザーからだと混乱の原因にもなりうると思っています。とはいえ、だからだめだ、というわけではなく、仕様であるといえばそれで済む話であるとは考えます。
ini の標準仕様に絶対に合わせるべきだ、とまでは考えませんが、上記のような懸念を事前に払拭しておくためには、なるべく早いタイミングで標準仕様に合わせておいたほうが、あとあと問題が大きくなった時にパッチを何度も当てずに済むのではないか、とは思っています。
- 実際、iniファイルを自動生成するユーザーも出てきているように、今後S2Eがより広いユーザーに使われるようになったらこのような仕様変更はどんどんやりにくくなっていくのではないかな、と想像しています。

200km commented 9 months ago

その他のユーザーサイドリポジトリに撒く予定

この作業は我々が把握しているユーザーにしかできず、すでに我々の認知できないユーザーがいることを考慮すべきかと思います。私が認知していないが鈴木くんが知っているユーザーレポジトリがあるでしょうし、その逆も確実に存在します。なので、見えないユーザーのことを考えると、AではなくBが良いと思っているということです。また、繰り返しですがユーザーサイドリポジトリに撒くのはAのPRがなくてもできることです。

いかなる処理系であっても漏れなく // をコメントとして扱うと保証できるのでしょうか？

この辺りの議論は、「今すぐ当てるパッチ」と「今後の仕様を考えた長い目線での修正」のうち、後者に当たることだと思っています。 いかなる処理系でもというのを考え始めると、ここ以外でも気になるバグ、挙動はあり得ますしすでに存在しています。その中で、せめてCIで扱っている、Windows Visual Studio 2022, gcc 11, clangでは正しく動くようにしようというのが良い妥協点だと思っています。
今回私の方では、gcc 11とclangで動作確認をして問題ないことを確認しています。残ったWindows Visual Studio 2022でのバグをとるためにパッチを当てるというのは、「今すぐ当てるパッチ」という意味では適切な妥協点だと思います。

今後S2Eがより広いユーザーに使われるようになったらこのような仕様変更はどんどんやりにくくなっていく

それはその通りだと思うので、だから仕様変更をしっかり議論したいということです。例えば、ini標準に合わせで//を禁止しますとやって、そのすぐ後にやっぱりiniやめますとかになる方が混乱する気がします。

いずれにせよ、今回のこのPR件は「今すぐ当てるパッチ」という話だと思っています。そのために適切なのは、AでもCでもなくBだと思います。AじゃないならCになるというのは飛びすぎだと思います。

「今後の仕様を考えた長い目線での修正」という話のためにBが適切でなく、Cの方が良いのはわかりますが、それは下記issueで丁寧に話し合うべきだと思います。この話し合いがなかなか進まないので優先度をあげてほしいということなら、そう提案してもらえると嬉しいです。

https://github.com/ut-issl/s2e-core/issues/549 https://github.com/ut-issl/s2e-core/issues/254

suzuki-toshihir0 commented 9 months ago

わかりました。影響範囲・改修の規模の観点でBが妥当であるということは納得したので、このPRを閉じ、別途Bに対応するPRを発行しますね。そのうえで、

ini標準に合わせで//を禁止しますとやって、そのすぐ後にやっぱりiniやめますとかになる方が混乱する

はその通りだと思うので、真にやるべき対応が何であるかは丁寧に議論すべきであると私も考えます。それは #254 などで引き続き議論させてください。

200km commented 9 months ago

ありがとうございます。よろしくお願いします。

ut-issl / s2e-core