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

CMakeLists.txtのリファクタリング #35

Merged
merged 18 commits into from
Dec 6, 2021
Merged

CMakeLists.txtのリファクタリング #35

merged 18 commits into from
Dec 6, 2021

Conversation

sksat
Copy link
Collaborator

@sksat sksat commented Nov 25, 2021

概要

CMakeLists.txtで現在では非推奨な書き方が使われていたり,そもそも非常に微妙なことになっていたりするので,どうにかする

Issue

詳細

  • coreにもCMakeLists.txtを生やして,userから参照するようにした
    • 今後はC2AそのものをCMakeでビルドすることもあるかもしれない
    • 注: もちろんcore単体ではビルドできない(これはこれでどうにかしたい)
  • 超巨大な変数にソースファイルを詰めていくやつをやめる
    • add_library(), target_sources()などを使うべき
    • 追記: target_sources()については.cなファイルをC++としてビルドすることができないため,一旦見送った
      • ただしCMakeLists.txtを分割したのでだいぶマシにはなった

検証結果

ビルドが通ればよし.手元では通るのでCIで通ればよし.

影響範囲

CMakeでビルドしたlibC2A.aを使うプロジェクトに影響がある(とはいっても,使っている側もCMakeの使い方が微妙なのでそれを直せばいい).
具体的にはS2E.

  • S2E側のCMakeLists.txtset(BUILD_C2A_AS_CXX ON)するようにしてもらう必要がある

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

おっと,ちょうど手元で試していないWin向けだけが落ちたな

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

まだ変更してない部分(コンパイルオプション指定)で落ちてて渋い!

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

userの方見て頭ひねってたけどcoreだった.そしてそれはそう

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

syntax errorは,おかしいだろ

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

理解した.

  • [前提] C2A(C89)をC++としてビルドしている
  • MSVCは拡張子が.cだと問答無用でCだと思う
  • なので/TPをコンパイルオプションに付けてビルドしている
  • が,別に真にC++なのはS2Eであって別にCとしてビルドは通るのではないか
  • ところがどっこい,src_user/IfWrapper/Sils/*.cに思いっきりC++がある

@meltingrabbit
Copy link
Collaborator

起承転結しっかりしてて,めっちゃ笑った.(笑えない)

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

src_user/IfWrapper/Sils/*.cにめちゃくちゃC++があるのは今後の課題として(たぶん今直すとS2Eが壊れる),なんでMSVCであることは認識できてんのに/TPが効いてないんだ?

@meltingrabbit
Copy link
Collaborator

src_user/IfWrapper/Sils/*.cにめちゃくちゃC++があるのは今後の課題として

これは,S2EがC++ビルドして使うことを想定しているのであれば,いい気もしているけれど,まずいっけ?

@meltingrabbit
Copy link
Collaborator

meltingrabbit commented Nov 25, 2021

issueきるか.

→ 切った

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

なんか見たくないタイトルの名前のIssue発見した!!!!!
https://gitlab.kitware.com/cmake/cmake/-/issues/19084

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

src_user/IfWrapper/Sils/*.cにめちゃくちゃC++があるのは今後の課題として

これは,S2EがC++ビルドして使うことを想定しているのであれば,いい気もしているけれど,まずいっけ?

何も問題は無いんですが,純粋にファイル名が変わるので,「変わるよ〜」って言って別タスクとしてやった方がいい

CMakeLists.txt Outdated Show resolved Hide resolved
@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

/TPが効かないやつ,色々調べてみたけどそもそもこれ動いてるやつコピーしてきたやつなのよな

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

たぶんtarget_sources()したやつにだけ効いてないんだろうけどそうだとしたら渋い

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

そもそもCMakeはCだと思ってるのにコンパイルオプションでas C++してるのも渋いしそっちをどうにかするか,と思ったけど個別にset_source_files_properties()とかしてもC++だとは思ってくれないな...

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

実は #36 でC++なやつの拡張子を.cppにするだけで解決したりしないだろうか.
というのは,まずそもそもC++でコンパイルする必要が本当にあるのはこれらだけ.
そして,C++なクラスが生えているとはいえ,結局のところC2A側とやりとりするためにCの関数でラップされている.
もし困ることがあるとすれば,

  • CとC++で解釈が違うものを踏んで挙動が変わる
  • (マングリングの有無で)シンボル名が変わってリンクできなくて困る

ぐらいだが,前者はSILSの場合むしろCの挙動に合わせるべきだし,後者は実は僕が雑にS2E with C2A on Linuxをビルドするやつに成功していて踏んでいないので問題無い(あったところでextern "C"するだけだろう).

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

とりあえずIfWrapper/Sils下のを全部.cppにするので通りはする.そしてこれは別PRを出すべきなので出します.

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

.c -> .cpp#37 でやったので/TP効かない問題については一旦そちらのマージを待ちます

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

coreはともかくuserはめちゃくちゃ各ディレクトリ肥大化しがちなんだからCMakeLists.txt分けるべきやろ,ということで分割

@sksat
Copy link
Collaborator Author

sksat commented Nov 25, 2021

さてコンパイルオプションをちゃんとするやつやるか(as C++のやつも普通に困るし)

@sksat sksat force-pushed the refactor-cmake branch 2 times, most recently from 447a350 to 9e96c27 Compare November 25, 2021 20:18
Copy link
Collaborator

@meltingrabbit meltingrabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

よさそう.

@sksat 追加したdocsをざっと見て問題なければマージしてしまってください.

Docs/Sils/S2eIntegration.md Outdated Show resolved Hide resolved
@sksat
Copy link
Collaborator Author

sksat commented Dec 6, 2021

もうちょい直しました.これで分かりやすくなったのでは.

@sksat
Copy link
Collaborator Author

sksat commented Dec 6, 2021

あとはs2e-integration.mdでいいんでは説もあるな

@meltingrabbit
Copy link
Collaborator

mdは,ファイル名直すなら,今度まとめてだな.

@meltingrabbit
Copy link
Collaborator

ありがとう.良さそうです.

@sksat
Copy link
Collaborator Author

sksat commented Dec 6, 2021

了解です.マージします.

@sksat sksat merged commit cac2106 into develop Dec 6, 2021
@sksat sksat deleted the refactor-cmake branch December 6, 2021 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority::medium priority medium S2E tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants