テストのビルドにパッケージを使ってビルド時間を短縮する(MinGW版) #894
Conversation
|
AppVeyor のログによると、ダウンロードとインストールで +2 秒、コンパイル省略で -20 秒というところでした。効果はあります。 msys2 に本格的に依存するということですが、以前にも他所で触れた通り、QITAB 関連の定義が足りないために AppVeyor で |
tests/CMakeLists.txt
Outdated
| set_target_properties(gmock_main PROPERTIES FOLDER GoogleTest) | ||
| set_target_properties(gtest PROPERTIES FOLDER GoogleTest) | ||
| set_target_properties(gtest_main PROPERTIES FOLDER GoogleTest) | ||
| if(MSVC) |
There was a problem hiding this comment.
この判定、vc かどうかの判定ではなく、google test を自前でビルドするかどうかのフラグを設けて、そのフラグを vcで有効にするようにするのがいいと思います。
なぜならこの部分は別に vc に依存する処理ではなく、単に現在の構成でvcのみで実行する処理というだけで、将来変更することが十分予想されるからです
There was a problem hiding this comment.
次が控えているのでサクサクと進めていきたいです。
https://github.com/berryzplus/sakura/commits/pendings/use_gtest_package_for_MSVC
他環境でも使うことは織り込み済みです。
| No | コンパイラ | ジェネレータ | GTest参照先 |
|---|---|---|---|
| 1 | MSVC | Visual Studio | NuGet(保留中、上記リンクの変更) |
| 2 | MSVC | Ninja | 現状通りソースからビルド(NuGet利用に変更したいが方式検討中) |
| 3 | MinGW | any | pacman |
| 4 | any other | any | 現状通りソースからビルド |
このPRで対応するのは 3 だけです。
なぜならこの部分は別に vc に依存する処理ではなく、単に現在の構成でvcのみで実行する処理というだけで、将来変更することが十分予想されるからです
そうですね。今回対応するのは「 MinGW だけ」なので、「MinGW以外」を表す条件に変更します。
There was a problem hiding this comment.
そうではないです。
パッケージ版を使うか、ソースからコンパイルするかです。
それをコンパイラで切り替える、です
There was a problem hiding this comment.
パッケージ版を使うか、ソースからコンパイルするかです。
ソースからコンパイルすることにメリットはありますか？
There was a problem hiding this comment.
そういう意味ではないです。
今やろうとしていることは、パッケージを使うかソースから
ビルドするかをコンパイラの種類によって切り替えようとしていますが
この処理に関しては別にコンパイラによって処理を
わける必要のあるものではなく、パッケージ版か
ソースコンパイルするかを切り替えようとしているか
で切り替えるべきものです。
その上でその変数をコンパイラによって定義すればいいことです。
だからここでもしコンパイラで切り替えるようにしてしまうと
汎用性が低く、分かりづらいとものになります。
MinGW でパッケージ版を使うのは単にプロジェクトの現在の
運用なだけで、別にコンパイラの違いによるものではないからです。
There was a problem hiding this comment.
最後に、ぼくはテストバッチ変更には今後関わるつもりがないことを宣言していますが、オプションを付ける対応ではバッチへの変更が必須です。
別にバッチファイルを変更する必要はないです。
イメージ的には #182 のようなものです。
x64 版でバージョン情報対応がありますが、
@berryzplus さんの対応方針でいくと、_WIN64 かどうかでアルファ版の表示を
行うようなコードを書くことになりますが、
本当に x64 版に依存する部分と、現状の運用で使う必要な構成でやっていることが
混ざってしまいます。アルファ版の表示をやめるときとかのどの部分を削除する
のかの判断をするときに苦労することになります。
何度も言いますが、
googletest のパッケージ版を使うか、ソースからビルドするかを切り替えるのは
別にコンパイラには依存しません。
本当にコンパイラに依存する部分と依存しない部分とを切り替えることにより
仕様変更する場合に対応しやすくするのと、コードをわかりやすくすることが
できます。
C/CPP言語でマクロを使って、特定のマクロを定義したり、しなかったり
して機能を有効にしたり、無効にしたりするようにしたりできます。
特定のヘッダでその定義を行うことによって集中的に管理できます。
そうすることで、将来的にその部分を config というような形で
ユーザーがカスタマイズすることができるようにすることも
考えられますが、そこまでする必要はないです。
There was a problem hiding this comment.
条件の切り方が、メンテしにくく、わかりづらいように
書くことにメリットがあるようには思えません。分かりづらいと感じたのはサクラエディタをビルドできる環境にどんなものがあるか、
どんな環境でビルドできるように動いているかについての認識が甘いからだと思います。あとでやっぱり、MinGW で ソースからビルドする構成も
やろうとした場合にどの部分を切り替えたらいいのか
逐一確認しないと判断できません。変更するときは、逐一確認して判断すべきです。（バッサリ
どんなに（君が）分かりやすいと思う書き方をしていようとも、確認は発生します。
レビュー観点としてはよろしくないと思います。
bfef92f で 対応したみたいですが、
- 元のコードのほうがいいと思いますか?
- 対応してみて、新しいコードのほうがわかりやすいと思いませんか? メンテしやすいと思いませんか?
- 大きな対応工数がかかりましたか?
There was a problem hiding this comment.
おかえりなさい。お待ちしてました。
最後に、ぼくはテストバッチ変更には今後関わるつもりがないことを宣言していますが、オプションを付ける対応ではバッチへの変更が必須です。
別にバッチファイルを変更する必要はないです。
イメージ的には #182 のようなものです。
提示された #182 を見てみました。
これをもって「俺のやり方のが分かりやすいんだ！」と言いたいんだと思うんですが、レビューした@kobakeさん、めっちゃ困惑してますね。分かりにくいから困惑したんじゃないのかな。客観的にはx64ブランチのマージを優先したいから分かんないけど通しちゃいます！に見えました。
x64 版でバージョン情報対応がありますが、@berryzplus さんの対応方針でいくと、_WIN64 かどうかでアルファ版の表示を行うようなコードを書くことになりますが、本当に x64 版に依存する部分と、現状の運用で使う必要な構成でやっていることが混ざってしまいます。
何を言ってるか分からんかったので #182 を読み返してみました。
このPR、x64対応と言いつつ、Alphaバージョン表示に関する仕様変更なんですね。
@kobakeさんが何に困惑したのか分かった気がします。
「何をいってるか分からん」の詳細としてまず、「運用で使う必要な構成でやっていることが混ざる」が理解不能でした。運用によって「x64ビルドでもAlpha表示をしないようにしたい」ないし「x64ビルド以外でもAlpha表示したい」のニーズが発生することを危惧した発言と受け取れたからです。
Alpha版表示は、動作検証が不十分なx64版をそうとは知らずに使うことを抑止したいで付けることになったと思っています。要件としてx64版であるかどうかを確実に判定できる条件を選択する必要があると思います。つまり、 処理内容がx64版に依存するかどうかとは関係なく「x64ビルドだったら付ける」 が仕様です。しかし、#182 はこの仕様を x64ビルドであるかどうかに関係なく「付けたいときに付ける」 に引っくり返すものでした。
プログラムの実装は、目的を満たすのに何が必要か（≒仕様）を決め、具体的にどう実現するか（≒設計）を考えた上で行うものです。（少なくともウォーターフォールな現場では。）
要件定義⇒基本設計⇒詳細設計・製造
古典的な開発現場ではこういう流れで話が進んでいきます。
通常、各工程でレビューを挟み、手戻りが少なくなるように話を進めていくものです。開発規模によりますが、各工程に数日かけるのが通例です。#182 でやってることは製造レビューの会議の場で「あ、基本設計変えといたんで読み替えてくださいね」といきなり仕様変更を告げるようなものです。
自分がレビューする立場で考えてみてください。
自分が予算調整して、プロジェクトの進捗を管理する立場にいると思って考えてみてください。
レビュー依頼で「x64版でAlpha表示を行います」って書いてるのに、会議の場でいきなり「運用のことを考えて、x64かどうかと関係なくAlpha表示したくなるかも知れないんでできるように設計し直してみました、てへっ。」とか言いだす開発者を見てどう思いますか？
言ってることが正しいかどうかに関係なく「帰れ！下級海賊。」とか言いたくなると思います。ここはOSSなのでそんなこと「ネタ」でしか言いませんけど、やられたら普通に困惑する内容だと思います。事前に合意した内容と違うことをするなら「事前の話とちょっと違うよ」と話を通して欲しいです。普通に例示として挙げたってことは、やらかしに自分では気付いてなかったんだと思います。「そういう見方をする人もいる」程度の認識で構わないので、気に留めといてください。
結構ボロカスにこき下ろしていますが、この行動は効果を期待して行っているパフォーマンスです。こういうコメントを書けば恨まれるかも知れないし、帰ってこなくなっちゃうかも知れない。考えてみるとほぼデメリットしかないんですけど、それでもプロジェクトのためには必要だと考えてやっています。
@m_tmatma さんの発想が全部ダメだなんてまったく思ってません。
むしろ、ぼくにとっては得るもののほうが多いと思います。コメントは考えて読んでいるつもりですが、真意が不明な書き込みが結構多いと感じています。これからも分からんときはちゃんと突っ込んで、真意を把握していきたいと思っています。
お互いのいいところを活かしてやっていくためにはコミュニケーションが大事です。質問にはできるだけ、真面目に答えてください。
何度も言いますが、googletest のパッケージ版を使うか、ソースからビルドするかを切り替えるのは別にコンパイラには依存しません。
このPRはGoogleTestのビルド時間が長いことへの対策PRです。
目的はビルド時間を短くすること。
手段としてパッケージ版を使える場合にはgoogleTestをビルドしないことを選択しました。主目的がビルド時間を短くすることなので、パッケージ版を利用できるかを判断基準としました。
「速度重視」の観点から「自前でGoogleTestをビルドしたい」の要望が出る可能性は切り捨てました。
目的は「ビルド時間を短くすること」なので、これはこれで正しいと思います。
gccのライブラリはMSVCでは扱えません。「使えるならライブラリを使う」だけを考えた対応で、判定条件がコンパイラになるのは自然です。ライブラリを使えるかどうかはコンパイラに依存するからです。
@m_tmatmaさんの指摘が「速度重視とはいえ、需要切り捨ての可能性がある変更は止めるべきだ」みたいな仕様寄りのコメントだったなら、ぼくも反発しなかったと思います。
C/CPP言語でマクロを使って、特定のマクロを定義したり、しなかったりして機能を有効にしたり、無効にしたりするようにしたりできます。
特定のヘッダでその定義を行うことによって集中的に管理できます。そうすることで、将来的にその部分を config というような形でユーザーがカスタマイズすることができるようにすることも考えられますが、そこまでする必要はないです。
理解できたから出てきたんじゃないんかいっ！ｗ
指摘後の修正で BUILD_GTEST オプションを追加しました。
既に「そこまでしてる」って読み取れてますよね？
bfef92f で 対応したみたいですが、
•元のコードのほうがいいと思いますか?
•対応してみて、新しいコードのほうがわかりやすいと思いませんか? メンテしやすいと思いませんか?
•大きな対応工数がかかりましたか?
- 元のコードでも目的(速度改善)は達成できるので問題なかったと思います。後追いでビルド切替を入れるにしても、目的の異なる別件対応が入るだけの話で、PR自体に問題があるわけじゃないと思います。
- 「仕様が変わってる」ので単純比較に意味はありません。が、切替オプションを提供することで、より良い仕様にできたと思います。この点は感謝しています。
- 「実績で2日もかかったじゃん。」というのが、その質問への答えです。2日全部かけたとは思ってませんけどね。
There was a problem hiding this comment.
おかえりなさい。お待ちしてました。
静岡から戻ってきました。静岡めっちゃ広い。
目的は「ビルド時間を短くすること」なので、これはこれで正しいと思います。
「ビルド時間を短くすること」は達成できていますし、意義のあることです。
そのうえで、こうすればもっとよくなるという提案をしています。
C/CPP言語でマクロを使って、特定のマクロを定義したり、しなかったりして機能を有効にしたり、無効にしたりするようにしたりできます。
特定のヘッダでその定義を行うことによって集中的に管理できます。
そうすることで、将来的にその部分を config というような形でユーザーがカスタマイズすることができるようにすることも考えられますが、そこまでする必要はないです。理解できたから出てきたんじゃないんかいっ！ｗ
指摘後の修正で
BUILD_GTESTオプションを追加しました。
既に「そこまでしてる」って読み取れてますよね？
ユーザーがカスタマイズする必要がないというのはコマンドラインで切り替えるという意味です。
BUILD_GTEST オプションを設けたうえで、プラットフォームによって、CMakeLists.txt の中で
切り替えたらいいということです。
この PR で対応していただいたたように、パッケージ版を使うかソース版を使うかの切り替えの
箇所を局所化するということです。
以下で書いているような、コマンドラインで切り替える必要はなく、ライブラリの切り替えに必要な処理に対して
条件コンパイルとして切り替えられていればよいという意味です。
最後に、ぼくはテストバッチ変更には今後関わるつもりがないことを宣言していますが、
オプションを付ける対応ではバッチへの変更が必須です。そして、バッチを変更したらドキュメントを
更新していくのが暗黙のルールとなっております。ドキュメントを書くのはめんどくさいので嫌です。
したがって、対応は行いますがドキュメント更新は行わない旨を宣言しました。必要だと思う人が作成すればよいと思います。
* 「実績で2日もかかったじゃん。」というのが、その質問への答えです。2日全部かけたとは思ってませんけどね。
やり方をすぐに調べて、すぐに動けば2, 3 時間もかからないと思います。違いませんか?
「テストバッチ変更には今後関わるつもりがない」ということにこだわらなければ
時間はかからないんではないですか?
There was a problem hiding this comment.
Alpha版表示は、動作検証が不十分なx64版をそうとは知らずに使うことを抑止したいで付けることになったと思っています。要件としてx64版であるかどうかを確実に判定できる条件を選択する必要があると思います。つまり、 処理内容がx64版に依存するかどうかとは関係なく「x64ビルドだったら付ける」 が仕様です。しかし、#182 はこの仕様を x64ビルドであるかどうかに関係なく「付けたいときに付ける」 に引っくり返すものでした。
#162 の最後に書いていますが、
アルファ版の表示がいらなくなったら、簡単に削除できるようにする、と書いています。
仕様としては、「x64ビルドだったら付ける」です。
でも同時に、不要になったら簡単に削除できるようにしています。
Alpha 表示をつけたいわけではなく、簡単に削除できるようにするためのものです。
|
ドキュメントは必要なら自分でPRしてくださいね。 |
berryzplus
changed the title
berryzplus
force-pushed
the
feature/use_gtest_package_for_MinGW
branch
from
8216ce9 to
bfef92f
Compare
berryzplus
changed the title
|
Google Test のビルド時間が気になる場合は Catch2 というヘッダファイルオンリーのテストフレームワークに切り替えると良いと個人的に思います。 Google Test が prebuilt な場合のビルド時間とは比較してませんが、ヘッダオンリーなので準備の手間は結構省けます。 |
|
以前にも Catch2 を推していましたね。乗り換えるために越えるべきハードルが色々考えられ、ビルド時間は大きな推進力にならないと思います。
他人が最初から Catch2 に興味を持っていると期待するのは高望みです。こういう疑問に予め答えを用意しておくことが採用への一歩かなと思います。もちろん、たった一人のシンパが現れるだけでとんとん拍子に話が進むということもあると思います。 |
それは前に聞いた（というかコメントを見た）っすｗ テスト用のcliを生成する機構を持ち、CMakeでのビルドに対応していることは確認しました。 「乗換」だと色々きっついので、「試験導入」から始めるのがお勧めです。 一点だけ、ライブラリの取り込み先についてはモメるかも知れません。 |
よく覚えてますね！
これについては下記のようなマクロをかます事でテストの記述を Google Test 互換に出来ます。 #define STR(x) #x
#define CONCAT(x,y) STR(x ## _ ## y)
#define TEST(x,y) TEST_CASE(CONCAT(x,y))
#define ASSERT_TRUE(x) REQUIRE(x)
#define ASSERT_EQ(x,y) REQUIRE( (x) == (y) )
#define ASSERT_NEAR(x,y,tolerance) REQUIRE( (x) == Approx(y).margin(tolerance) )
#define ASSERT_DOUBLE_EQ(x,y) REQUIRE( (x) == Approx(y) )
#define ASSERT_FLOAT_EQ(x,y) REQUIRE( (x) == Approx(y) )
#define EXPECT_EQ(x,y) CHECK( (x) == (y) )
#define EXPECT_STREQ(x,y) CHECK( strcmp((x),(y)) == 0 )
#define EXPECT_NE(x,y) CHECK( (x) != (y) )
#define EXPECT_DOUBLE_EQ(x,y) CHECK( (x) == Approx(y) )
#define EXPECT_FLOAT_EQ(x,y) CHECK( (x) == Approx(y) )
#define EXPECT_TRUE(x) CHECK(x)
#define EXPECT_FALSE(x) CHECK(!(x))
#define EXPECT_NEAR(x,y,tolerance) CHECK( (x) == Approx(y).margin(tolerance) )
#define EXPECT_LT(x,y) CHECK((x) < (y))
#define EXPECT_LE(x,y) CHECK((x) <= (y))
#define EXPECT_GE(x,y) CHECK((x) >= (y))
#define EXPECT_THROW(stmt, exc_type) CHECK_THROWS_AS( stmt, exc_type )
その３つはどれも十分満たしていると思います。gtest と比べてどうか？は主観が入りそうですが。。
長文の記事を書いて Catch2 ﾏｼﾞｵﾇﾇﾒ! と喧伝する evangelist になる気概は無いです…。 |
よく覚えてますね！
PR(#433) は昔作ったんですが食いつきが悪かったです。。
amalgamate されたヘッダファイルが用意されてるのでそれをどっかに置くのが良いと思います。せっかくシングルヘッダーファイルなんだし submodule でやらないでよいと自分は思いますが…。 |
あれは「切替」の提案だったのでｗ あと、ぼくはcatch2を導入するにしてもgoogletestは残したいんです。
たぶん、通常ルートはそれですね。(どっか＝externals/catch/include) |
確かに Google Test の方が一般的ですね。 並存させる必要があるのかというと無い気がします。切替の仕組みを入れる余計な手間を掛けるのは本末転倒な気がします。
リリース物にシングルヘッダーファイルが用意されているので externals 直下にフォルダを作らずに catch.hpp を単純に追加するだけで良いと思います。最新版のリリースに即時追従する必要って有るんでしょうか？最新版だとなんとなく気分的に気持ち良いとかは有るかもしれませんね。でも少し古いバージョンを使ってると呼吸困難で死んでしまう、という人はいないと思います…。 |
|
ちょっと脱線気味ですがもう少し。 サクラエディタのテストといえば、JScript マクロを用いたテストも含まれると思うんです。SourceForge.net に投稿してコミットされたパッチには、そういうマクロを添付して挙動を確かめながら修正を重ねていったものがあります。そのテストマクロの置き場所と自動実行フレームワークが今は存在しません。 以下の２件で合計８つくらいのテストマクロがあります。
ですから、テスト = googletest or Catch2 のような択一的な考え方はしなくてもいいかな、と。「自分は Catch2 でテストが書きたい。その方が簡単だし、それ以外の流儀で書くくらいならテストは付けない」という考えのもと行動するのもありではないかな、と。CI で見られるテスト結果をどう統合できるかはちょっとわかりませんが。 |
|
catch2を一番簡単に導入する方法は、 みんな、レビューしようぜ！ｗ |
|
レスだけ付ける！
並存させる必要はないと思います。
即時追従の必要性はないと思います。気付いたたときに上げればいいと思います。ローカルで通ったテストが取らなかった原因を調べてみたら、既に修正されたテストフレームワークのバグだった、みたいなケースに遭遇したら「すぐ変えなきゃ！」になると思います。そういう時はしょうがない...。ただ、そんなの確率的には「大当たり」だと思います。発生確率の低い事象で思い悩むのは、宝くじだけにしたいです。（7億当たったらどうしよう・・・:sob: |
|
いま帰宅！仕事で残業よりも身内に拉致られるほうが返り時間は読みづらいもので、もう今日は帰れないかと思いました。（注：現在時刻01:16 レスは気が向いたら付けます。（ﾏﾃ |
* 一部の環境でしかテストプログラムを作成できなくなっていた。
* パッケージ版で gmock が利用できなかった。
=> 必要になるまで gmock 非対応とし、ソースビルド版から
gmock を削除することで足並みを揃えた。
* 一部の環境でしかテストプログラムを作成できなくなっていた。
* パッケージ版で gmock が利用できなかった。
=> 必要になるまで gmock 非対応とし、ソースビルド版から
gmock を削除することで足並みを揃えた。
* AppVeyor で、パッケージを使わないときにもインストールしていた。
* 一部の環境でしかテストプログラムを作成できなくなっていた。
* パッケージ版で gmock が利用できなかった。
=> 必要になるまで gmock 非対応とし、ソースビルド版から
gmock を削除することで足並みを揃えた。
* AppVeyor で、パッケージを使わないときにもインストールしていた。
…t_package_for_MinGW テストのビルドにパッケージを使ってビルド時間を短縮する(MinGW版)
目的
MinGW向けテストのビルド時間を短縮します。
変更内容
subdirectory(googletest)を MinGW ビルドでは実行されないようにします。メリット
MinGW版でGooleTestをビルドする必要がなくなるため、ビルドが少し速くなります。
他の有用なライブラリを取り込む際に参考にできる実績を作ることができます。
デメリット
ビルド時にmsys2のパッケージをインストールする必要があります。
MinGW版ビルド環境が本格的にmsys2に依存するようになります。
備考
MSVCでは引き続きGoogleTestのビルドが必要です。
MSVC向けの変更は別件とします。
このPRがマージされ次第WIPを外す予定です。