Move external library

[200km]

Related issues

#423

Description

The src/library/external directory was moved to src/external_libraries.

Test results

See CI.

Impact

Users need to change the followings

• include directory path to the external libraries
• CMakeList

Supplementary information

NA

[sksat]

一応確認と整理: このディレクトリにあるライブラリに対して "external" みたいな括りをつけたいのって，元々 S2E 外で作られたもののうち，ソースファイルレベルである程度手を入れたいもの，という理解で合っていますかね？（一方で，ここと ExtLibraries が分離しているのは，向こうは手を入れずにインストールでき，かつ十分ダウンロードやビルドが面倒なのでキャッシュしたいから）

[200km]

external みたいな括りをつけたい　理由

ここには、

• S2E外で作られたものでダウンロードなどができない or やりづらいもの
• S2E外で作られたもの(ExtLibrariesにあるもの)をS2Eで使いやすくするためにラップする関数・クラス

が保管されています。
S2E外のもので命名規則をはじめとするコーディングルールに沿わないものを隔離して、管理しやすくする意図もあり他と分離して管理したいです。

ここと ExtLibraries が分離しているのは

ExtLibrariesは外部サイトなどからダウンロードできるもの、externalはダウンロードできないものという分け方になっています。

[sksat]

コーディングルールに沿うかどうかで分離したいのはたしかに．一方で，ダウンロードできるかどうか，という区別にはあまり意味が無いと感じています．例えば s2e-core や c2a-core は user から Git submodule で参照しているため，実態としてはダウンロードを伴うわけですけれど，ダウンロードできることを持ってしてなんらかの特別扱いはしていないですよね．

また，先ほど Issue 化したのですが，現行の ExtLibraries のかなり非自明な点として，ライブラリではないただのデータのダウンロードも扱ってしまっているという点があると思います: #611
そのため，ExtLibraries はそれはそれで整理したいと考えています．

[sksat]

あと，コーディングルールに沿わないものを隔離したい，というのも分かる一方で，外部ライブラリとそのラッパークラスの実装が離れた場所にあるのも結構非自明かつ扱いにくいと思うのですよね．なので，

• src
• thirdparty
  • libhoge（今 src/library/external にいるやつ）
  • libfuga（今 ExtLibraries にいるやつ．ビルド前にダウンロードが走る）
  • libfuga_wrapper（今 src/library/external にいるやつ）

みたいな整理をするのがいいんじゃないかと思っているんですが，どうでしょうか．ExtLibraries のライブラリ群のダウンロードやビルドが遅い上にキャッシュが効かない，みたいな問題はなんとかします（これをなんとかする面でもこのような構造になっていると扱いやすいです）．

[200km]

#612 なども関係すると思いますが、今の分類方法は大きな方針は次のような感じだと思います。

• src:ソースコード（コンパイル対象になるもの）
• ExtLibraries: ビルド（リンク）の対象にはなるが、毎度コンパイルはしないもの
• data: ビルド対象でないもの

これはこれで意味があり、分かりやすい分類な気がしています。

ご提案いただいている場合、thirdpartyに上記分類のsrcとExtLibrariesが混ざるのがイマイチな気がしています。
分類方法は東大の他メンバにもどちらが良いか意見をもらいたいと思います。

どちらかに決まったら、現状その分類に合っていないものを移動させるようにすれば #612　にもつながるのかなと思います。

[200km]

このPRで議論しているExtLibrariesとlibrary/externalの比較をするなら、dataは一旦置いておいて、重要なのは、コンパイル対象のコードとビルドでリンクされる対象のlibファイルなどの違いかなと思います。そこに注目すると、

• ExtLibrariesには、基本的にヘッダとlibファイルなどしかなくソースコード(cpp)はない。nrlmsiseにはcがあるが、これは先にコンパイルしており、s2e-coreコンパイル時には見ていない。中身も理解していない。
• library/externalにはcppがあり、コピーしているとはいえコメントを自分達でつけたり中身を理解している。

と明確に分かれているのが私のイメージです。これが伝わるような文章にはなっていませんが、CMkaeなどの構成を考えてもこうなっています。

誰が管理しているものか

こういう意味でも、library/externalの中のものはS2Eでメンテできるものと言えます。特にsgp4, igrfはc2a用には自分達で実装済みで、近いうちにそれに入れ替えたいとも思っています。nrlmsise_wrapperは明らかに自分達で作ったものなので、S2Eが今もメンテしているものです。
強いて言えば、inihはメンテはあまりしたくないので、今後ExtLibrariesに移動させ、事前にコンパイルしてリンクする形にしても良いと思っています。

（これとは別で、#612の議論としてExtLibrariesとdataのどちらにもコードでもないただのデータが入っているのは整理したいというのはあると思いますが、そちらは#612で議論した方が整理できると思います）

[200km]

ExtLibraries（の分類）が今提供している最大の価値は「毎度コンパイルはしない」つまり，ビルドするのが面倒かつ時間がかかるので，キャッシュできると便利，という部分ではないでしょうか？

これはそうですし、今もキャッシュしていると思っています。私のローカルでは毎回ExtLibrariesをコンパイルなどしていませんし、s2e-coreやs2e-aobcのCIの中でもキャッシュを持ってコンパイルすることなどはしていません。

例えば，シミュレーションのケースを少し弄っただけなのに S2E 全体のビルドが走る，というのはあんまり健全な状況ではないはずです

そのためにdataがあり、シミュレーションの細かな条件はリビルドなどせずに変更できるようにしています。
シミュレーションのケースを少し弄るがどこまでが少しなのかは人によって違うかもですが、基本的には、コードが確定して大量の解析を回したいフェーズではdataディレクトリ内の初期設定ファイルを修正するだけで良いような構成になっていると思います。

[sksat]

そうですね．最終的な整理はこの PR というよりは #611, #612 で議論すべきだと思います．なのでこの PR では ExtLibraries を含めた整理は out-of-scope だ，ということについては同意するのですが，この PR の意図を（#611, #612 のためにも）念のため確認しておきたい，という気持ちでのスレッドでした．（その上でも，単に移動させるだけでなく名前も変えている意図があまり明らかでないことと，ExtLibraries との被りを疑問視しているため，この PR に関する質問は #605 (comment) の方で分けてしていました）

[sksat]

キャッシュについては単に伝わっていないと思うのでもう少しシンプルに言うと，ExtLibraries とかに関わらず，src 内の一つの .cpp ファイルを編集したら，その一つの .cpp ファイルのコンパイルとリンクだけがされるようにできる，みたいな話です．

[200km]

お互いの意図はわかってきたと思うので、とりあえずこのPRとしては、別スレッドの命名についてを議論してクローズさせたいと思います。

キャッシュについては今よりももっと細かくという意図だと理解しました。ただ、s2eの基本思想であるiniファイルの活用をちゃんとやれていれば、実際の解析時にはソースコードを直接変更するようなことはないと思っています。
もし、ここが問題だと感じるようなら、私が想定しているような使い方でない可能性が高く、うまく活用できていないということになるのだと思います。

現状、私の使い方で困るとするとC2Aと組み合わせた場合に、C2Aの内部のゲインを修正したときに全てビルドする必要があるというのがあります。ただ、これはs2e内部の話にとどまらず、c2a側の問題も含まれていると思います。

[sksat]

このディレクトリを src/external_libraries と命名したいのはなぜでしょうか？単に src/external でも困らないように感じています．

[sksat]

他の案としては，一般的なソフトウェアで https://github.com/ut-issl/s2e-core/pull/605/files#r1490153761 の用途であれば thirdparty みたいな命名をしがちですね．

[200km]

externalだけだと一般的な命名すぎてよく分からないかなと思って、librariesとつけています。また、ExtLibrariesと「外部のもの」という意味では同じ属性なので、名前も合わせました。

[200km]

thirdpartyも良いと思いますが、やはり一般的な名前なのでthirdparty_librariesでしょうか。それとも、libraryという名前がつくのが気になっているのでしょうか。

[sksat]

ExtLibraries と「外部のもの」という意味では同じ属性，というのはそれこそ一般的すぎる命名をしてしまった結果異なる扱いをしたいはずなのに同じ名前になるという事故が発生してしまっていると思います．そのため最低限 ExtLibraries とは名前を分けたいのと，追加で library という名前が付くのが単に不要に長くなっているようで気になっています．

[200km]

また事前の議論について、issueでの議論を見てもらえれば分かる通り、この件は6月の私のコメントから半年も何も進みませんでした。何度かslack等でもリマインドさせていただいたにも関わらず１２月まで全く進まないということもありました。

議論をしたいのに議論が進まないというのも、改善に前向きでコードを修正したい私にとってはとても困ることですので、ご配慮いただけると嬉しいです。

[sksat]

externalディレクトリの存在意義みたいなこと

これについては，別にこちらも（この PR 上で）議論することになるとは全く思っていませんでした．
この PR に固有の議論としては，あくまで（最初にこのスレッドに書いた通り）src/external_libraries と ExtLibraries が同時に存在するのはおかしいのでは，という一点だけです．ここに納得できない具体例として src/library/external と ExtLibraries の関係というこの PR の後に議論したいと思っていたことを直近で困る例として出そうとしたところ，お互いあまり前提を共有しないまま感想を出し合うだけのことが多く建設的な議論になりにくかった，という理解をしています．

また，合意できたのは
#605 (comment)
#605 (comment)
あたりだと思っているのですが，PR の diff はまだ src/external_libraries になっていると思うので，approve できていないです．

[sksat]

各返答が遅くなってしまっていたのは申し訳ないです．一方で，S2E に関するこちらのエフォートの割き方が非常に難しいとも感じていて，そこのやり方含めて今後調整させて頂けるとありがたいです（どんなスケジュール感，目的で開発をしているのかわからないまま「PR 溜まっています」と言われても正直困ってしまう，など）．あとたぶん CODEOWNERS の指定が大きすぎて ISSL AOCS 関連の通知が溢れているというのがあると思うのでそれも調整すべきですね．

[200km]

external関連は最終的に全て消すので、これは命名に拘らずマージして別PRで作業したいということをお伝えしたと思うので、approveお願いします。

[sksat]

了解しました．その結論であれば OK です．