-
Notifications
You must be signed in to change notification settings - Fork 168
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
テストをコンパイル済みのエディタコードとリンクします。 #579
テストをコンパイル済みのエディタコードとリンクします。 #579
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMです。
一件指摘入れましたがスタブを入れるときに使えますし、削らなくても何の影響もないはずです。
目的を達成していることを重視してok出しときます。
オブジェクトファイルをグロブしてリンクするのではなく、エディタのコンパイル時についでに sakura.lib のようなスタティックライブラリを作成しておき、それひとつをリンクするのが通常かと思います。プロジェクトファイルの編集が困難なためそれは TODO ということでお願いします。 |
googletest の導入を主導したのは m-tmatma さんなので、@m-tmatma さんの LGTM も欲しいです。引っかかる点があるのであれば解消しておきたいです。 |
tests/create-project.bat
Outdated
@@ -14,6 +14,16 @@ if exist "%BUILDDIR%" ( | |||
rmdir /s /q "%BUILDDIR%" | |||
) | |||
mkdir %BUILDDIR% | |||
|
|||
set SAKURA_OBJ_OUT=%platform%/%configuration% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMakeLists.txt ではなくなぜバッチファイルでコンパイルオプションを指定しているのですか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この部分ですね。
sakura/tests/create-project.bat
Lines 18 to 25 in 70656fb
set SAKURA_OBJ_OUT=%platform%/%configuration% | |
if "%configuration%" == "Release" ( | |
set CXXFLAGS=-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GL /GF /FD /EHsc /MT /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis | |
set LDFLAGS=-LTCG comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib | |
) else ( | |
set CXXFLAGS=-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GF /FD /EHsc /MTd /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis | |
set LDFLAGS=comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib | |
) |
tests/unittests/CMakeLists.txt は tests*.exe をコンパイルするための設定だと思います。エディタをコンパイルした設定は知りません。なので具体的な部分をできるだけ外部(tests/create-project.bat)から注入する形をとり、CMakeLists.txt には外形的な内容だけを書こうとしています。いずれにしろコンパイルオプションを一元的に管理する体制になるまでの一時的な手段です。
tests/unittests/CMakeLists.txt
Outdated
|
||
# add definitions | ||
target_compile_definitions(${project_name} PUBLIC _CONSOLE UNICODE _UNICODE) | ||
|
||
# add include directories for sakura_core | ||
target_include_directories(${project_name} PRIVATE ${CMAKE_SOURCE_DIR}/../sakura_core) | ||
|
||
# set output directories | ||
set_target_properties(${project_name} PROPERTIES | ||
RUNTIME_OUTPUT_DIRECTORY "$ENV{SAKURA_OBJ_OUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
バッチファイルで定義した環境変数を使わずに CMakeLists.txt
内で定義した定数を使うべきだと思います。
そうでないと、 create-project.bat を使わないとプロジェクトを生成できなくなります。
(直接 cmake をたたいてプロジェクトを作成できなくなる)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この指摘は同時にコンパイルオプションに対する指摘としても適用されますが、それでよろしいでしょうか。
つまり RUNTIME_OUTPUT_DIRECTORY "$ENV{SAKURA_OBJ_OUT}"
というのは、リンク時コード生成のためにリンカが呼び出したコンパイラの生成物が、エディタのコンパイル時設定に基づくパスに配置されるために、そのための場所を予め用意しておくために設定しています。
コンパイルオプションの一部だということで、#579 (comment) で書いた通りに具体的部分を「できるだけ」外部に押し出した結果です。
この指摘を受け止めた結果がどうなるかといえば……
現在 CMakeLists.txt には「エディタのオブジェクトコードをリンクする」という外形的な、間違いではないルールだけを書いていますが、そこに特定のコンパイルオプションという、将来的には別の場所に追い出されるジャンクを埋め込むということです。
現状ではエディタとテストに共通するコンパイルオプションを埋め込む適切な場所がないために、create-project.bat なり unittests/CMakeLists.txt なりのいずれかにジャンクを押しつけるしかなく、それがどちらであるかにはこだわっていません。
create-project.bat を迂回して直接 cmake を叩くことに価値があるのなら対応しない理由はありません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
現状ではエディタとテストに共通するコンパイルオプションを埋め込む適切な場所がないために、create-project.bat なり unittests/CMakeLists.txt なりのいずれかにジャンクを押しつけるしかなく、それがどちらであるかにはこだわっていません。
以下のような内容でファイルを作って、CMakeLists.txt からインクルードしては
いかがでしょうか?
set(CMAKE_CXX_FLAGS "")
set(CMAKE_CXX_FLAGS_DEBUG "-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GF /FD /EHsc /MTd /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GL /GF /FD /EHsc /MT /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis")
set(CMAKE_CXX_FLAGS_RELEASE "-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GL /GF /FD /EHsc /MT /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis")
set(CMAKE_CXX_FLAGS_MINSIZEREL "-DWIN32 -D_WIN32_WINNT=_WIN32_WINNT_WIN7 /GL /GF /FD /EHsc /MT /Zi /TP /source-charset:utf-8 /execution-charset:shift_jis")
set(CMAKE_EXE_LINKER_FLAGS "")
set(CMAKE_EXE_LINKER_FLAGS_DEBUG " comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib")
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "-LTCG comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib")
set(CMAKE_EXE_LINKER_FLAGS_RELEASE "-LTCG comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib")
set(CMAKE_EXE_LINKER_FLAGS_MINSIZEREL "-LTCG comctl32.lib Imm32.lib mpr.lib imagehlp.lib Shlwapi.lib")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
つまり
RUNTIME_OUTPUT_DIRECTORY "$ENV{SAKURA_OBJ_OUT}"
というのは、リンク時コード生成のためにリンカが呼び出したコンパイラの生成物が、エディタのコンパイル時設定に基づくパスに配置されるために、そのための場所を予め用意しておくために設定しています。
通常 cmake のプロジェクトは Debug, Release に関係なくプロジェクトを作成できますが、
オブジェクトファイルを再利用する考え方では Debug 専用 とか Release 専用になってしまいますね。
コマンドラインで与えない方法を考えて、CMAKE_GENERATOR_PLATFORM
や CMAKE_BUILD_TYPE
を使えないか検討していたのですが、
プロジェクト生成段階では CMAKE_BUILD_TYPE
は確定しないので使えないですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
通常 cmake のプロジェクトは Debug, Release に関係なくプロジェクトを作成できますが、
オブジェクトファイルを再利用する考え方では Debug 専用 とか Release 専用になってしまいますね。
プロジェクトという単位の扱い方次第です。そのプロジェクトの中に生成ターゲットとしてテストとエディタが両方入っているのなら、オブジェクトファイルはいつでも共用できます。プロジェクトの中であえて個別のコンパイルオプションを定義して与えたりしない限りは。
独立させるのなら、Cランタイムライブラリを LIBCMT.lib と LIBCMTD.lib とで切り替えるように sakura.lib と sakuraD.lib を使い分けることになるでしょうし、TCHAR の定義が分かれることに備えて sakuraA.lib, sakuraW.lib, sakuraAD.lib, sakuraWD.lib を使い分けることも考えなければいけないでしょう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
つまり、ソースファイルを共有するエディタとテストは独立していないし、独立させずにソースコード間に依存関係を設定するなら、中間生成物であるオブジェクトファイル、スタティックライブラリがリリースビルドかデバッグビルドかという区別は消えるということです。
この PR がオブジェクトファイルを再利用しているのは、プロジェクトファイルの中にエディタを構成するソースファイルのリストやコンパイルオプションが隠されているがための苦肉の策に過ぎません。しかし当面の役には立ちます。
.cpp ファイルを GREP して再コンパイルする例も示しましたし、そちらはリリースもデバッグも気にせずにテストをビルドできますが、ビルド時間が膨大になりました。ソースコード間に適切な依存関係を設定できなくて、エディタとテストとで大部分が共有されているソースコードを2回繰り返してコンパイルしたためです。
tests/create-project.bat
Outdated
@@ -14,6 +14,16 @@ if exist "%BUILDDIR%" ( | |||
rmdir /s /q "%BUILDDIR%" | |||
) | |||
mkdir %BUILDDIR% | |||
|
|||
set SAKURA_OBJ_OUT=%platform%/%configuration% | |||
if "%configuration%" == "Release" ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cmake.org/cmake/help/v3.0/variable/CMAKE_BUILD_TYPE.html
cmake にはビルド構成として
DEBUG
RELEASE
RELWITHDEBINFO
MINSIZEREL
という構成があります。
RELEASE
ではないイコール DEBUG
ではないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
初めて知りましたが build-and-test.bat は AppVeyor から呼ばれるのですね。
appveyor.yml には
configuration:
- Release
- Debug
call tests\build-and-test.bat %PLATFORM% %CONFIGURATION%
とあり、create-project.bat は build-and-test.bat から呼ばれます。
%configuration% は Release か Debug です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%configuration% は Release か Debug です
AppVeyor から呼ぶ場合は現在はそうです。
しかし別に このバッチファイル自体は appveyor からだけ呼ばれるわけではなく
ローカルでも呼ぶことができます。そのとき引数に制限はないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
その場合はどういう対応をするのが正解なのでしょうか。対応できるのでしょうか。分岐に漏れを作らないために、else を使います。Release と Debug とそれ以外を見分けても、それ以外の場合にできることがないからです。
楽観的に突き進むのではなく早期にエラーを返すべきだということでしょうか。それは、なんというか、いい考えだと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
以下の構成があって
DEBUG
RELEASE
RELWITHDEBINFO
MINSIZEREL
RELEASE, RELWITHDEBINFO, MINSIZEREL がお友達と考えると
とりあえずでやるとしたらDEBUG か、それ以外かという対応かと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「DEBUG か、それ以外か」とするのはよいのですが、Release, Debug 以外の値をサクラエディタのプロジェクトでサポートする予定があるのですか?
それらは CMake が種々のコンパイラ向けに用意したプリセットの名前だと思います。リリース構成を Release, RelWithDebInfo, MinSizeRel のどれかをベースとしてカスタマイズするのがリーズナブルな選択だとしても、それだけのものではありませんか?
AppVeyor のビルド時間がプラットフォーム×コンフィグの掛け算で増えることを考えても、コンフィグの数は絞るべきだと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「DEBUG か、それ以外か」とするのはよいのですが、Release, Debug 以外の値をサクラエディタのプロジェクトでサポートする予定があるのですか?
今現在、対応の予定があるかではなく、将来対応したいと思ったときに問題のある
可能性のあるコードを作らないという話です。
一般論として最初に対応するときに考慮すれば簡単に対応できますが、
後で対応するとなると多くの時間がかかります。
64bit 対応やエンディアンに依存しない コードとかの場合、実装当初は対応予定がなくても
最初の対応段階から計画しておいて、必要になったら対応できるようにしておけば、
いざ対応することになったときに必要な処理を追加すればすぐに対応できます。
今必要なくても後で必要になるのはよくある話です。
AppVeyor のビルド時間がプラットフォーム×コンフィグの掛け算で増えることを考えても、コンフィグの数は絞るべきだと思います。
ここのバッチファイルで他の構成で問題ないようにする話と、appveyor で実行する
という話は全く別の話です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI という考え方もありますが黙っておきます。
この PR の寿命は長くても #378 が取り込まれるまでです。そのときにはこれより適切な方法がとれるはずなので。
↑ 同じコメントが何回も登録されていて紛らわしいので、削除しておいてほしいです。 |
リロードしてください。すでに削除しています。 今朝からの障害で最新版から過去のいずれかの版まで、あらゆるバージョンのページが表示されていますが、書き込みは基本的に一度で通っているようです。それに気がつくまでに投稿を繰り返してしまいました。 |
https://status.github.com/messages を見ると以下とある。
|
#378 の master へのマージはまだまだ先に見えます。 自分は #553 (review) に対する答えとしてテストを書こうとして、結果としてあれ(#560) やこれの横道に入り込んでいます。これなしではテストが書けないからです。 #378 はプロジェクトファイルの CMake 化が目的であって、エディタとテストのビルド統合を目的にはしていなかったと思います。ひとまずプロジェクト設定の CMakeLists.txt への反映を #378 で済ませてから、エディタとテストの CMakeLists.txt が設定を共有する方法を考えたいです。この PR がトリッキーなのはひとえに、設定がプロジェクトファイルと CMakeLists.txt とに分かれていることに理由があるからです。 それまではこの PR を master に取り込む一方、#378 ではそのコミットを裏返したコミットを挟んで従来の作業を続けてもらうのがいいと思うのです。 (追記) トリッキーになるのはこの PR に対する #378 の対応だったのでしょうか。それはすぐ上で書いたように、ひとまず変更をキャンセルするのがいいと思います。 |
@berryzplus さんのこの重複コメントはまだ削除されていない気がします…… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve取消の意味でrequest changeに変えておきます。判断は他の方に任せます。
include_external_msproject という設定もあるようです。 これなら重複してビルドするのを避けられるかもしれません。(まだ全く試してません) |
70656fb
to
da8540d
Compare
コミット da8540d の変更まとめです。
変更に含まれない点について。
|
対応ありがとうございます。いい感じです。 一点お願いがあります。 現在では、build-sln.bat はリビルドしないので二回呼んでも問題ないと思います。 appveyor で二回呼ばれるのが気になるのなら、tests\create-project.bat の中で tests\create-project.bat を呼ぶだけで、プロジェクトの作成をできるようにしたいというのが目的です。 |
193ce33
to
e826cc2
Compare
こんな感じ ポイントは
|
1 についてtests/create-project.bat が pushd する前だと tests/create-project.bat を呼ぶ人間が、一番ありうる tests ディレクトリとは違う作業ディレクトリに予め移動していなければいけません。 tests\build-and-test.bat を build-sln.bat とは独立して呼び出すのが目的の変更なのにそれでは使いにくいです。 (追記) 「sakura.sln が見つからなくなる」という現象について確認していません。そういう対処すべき現象があって pushd に不都合があるのなら、 2 について異論はありません。コミット後に気がついて他所から引っ張ってきたコードの核心は 3 について冪等性もしくは RESTful という言葉が好きです。省けると聞いたので省きたいです。また、AppVeyor 環境では呼ばなくてもいい、という前提を不要にできるならそれに越したことはありません。前提条件を保守・保証しないで済むので。 4 について
|
ディレクトリに関しては、最初に トップディレクトリに対して pushd してあとで popd でもとに戻せばいいです。 ビルドをするのは、ヘッダーの生成に加えて、オブジェクトファイルを生成するためのはずです。 3 に関しては対応してもしなくてもどちらでもいいです。 |
e826cc2
to
1fbaba8
Compare
コミットを更新しました> 1fbaba8 #579 (comment) でいただいた指摘に対して
4 について。ヘッダファイルの欠落と同じようにオブジェクトファイルの欠落を CMake がプロジェクト作成過程でエラーにするという事情があるのなら、コメントを修正するのにやぶさかではありません。 英語の文言については遠慮なく指摘が欲しい立場なのですが、「build sakura.sln in advance to generate obj file」だと、build-sln のあとで create-project が obj ファイルを生成するように読めます。 |
ビルドエラーの原因はコミットではありません。
Artifact というものへの考え方が AppVeyor との間で違うのでしょうか。新しいビルドを優先して勝手に古いものから削除してくれればいいのに、と考えるのですが。 |
70GBを超過ですか。1回のビルドで100MB以上のArtifacts作ってるので数百回で超過と。 |
失礼しました。 以下で アスタリスクの展開が cmake でのプロジェクト生成段階で行われると思い込んでいましたが、 sakura/tests/unittests/CMakeLists.txt Line 39 in 1fbaba8
生成されたプロジェクトを見ると、以下のようにアスタリスクが生成プロジェクトの中に埋め込まれていました。
なので object ファイルがビルド時に必要になるのは正しいです。 だめですね~~~ |
これは忘れてください。 |
またサイズ拡張をお願いしました。 |
…ルドするとコンパイルエラーになる」
1fbaba8
to
532e01c
Compare
コミットに付いていたコメントを --amend で漂流させてしまいました(Show outdated もできない)。コピペします。 1fbaba8#diff-f1be1d71083981c2380d13a27ef247e0R5 m-tmatma 14 minutes ago Member
ds14050 5 minutes ago Member
こうしてみると日時の相対表示はわかりやすいけども引用には向きませんね。 |
サイズ増やしてもらったので、リビルドかけました |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対応ありがとうございます。
appveyor も通りました。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/19800961 |
…torObjects * テストをコンパイル済みのエディタコードとリンクします。 * Fix sakura-editor#561 「通常のソリューションのコンパイル前に tests\build-and-test.bat でビルドするとコンパイルエラーになる」
意義は #561 (comment) を参照してください。そこで「テストを書くのが簡単になります」とは書きましたが、これがスタート地点だと思います。
コンパイラオプションの定義がエディタのプロジェクトファイルとテストの CMakeLists.txt とで重複していますが、いずれかの形式に統合されるまでは目をつぶってほしいと思います、テストを書くために。