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

ビルド時間とアーティファクトサイズの軽減が狙いですが、主としてバッチのリファクタリングです。 #615

Closed
wants to merge 4 commits into from

Conversation

ds14050
Copy link
Contributor

@ds14050 ds14050 commented Nov 16, 2018

関連 #427 「appveyor のビルドで削れるものがないか検討する」

対象領域が被っているためにひとつの PR にまとめていますが、3つのコミット毎に主眼が異なります。順番に適用する毎にビルドが通ることを確認しています。

コミットを紹介します(※ rebase の度に更新します)。

  1. 7cba65b 「他のバッチを束ねるビルド系バッチのリファクタリング&計時機能の追加」
  2. 53c41eb 「テストバッチのリファクタリング&ビルドターゲット指定。」
  3. be13e50 「テスト関連のビルド産物置き場を指定する。」

ご意見ください。


残件。

エディタをビルドするときとテストをビルドするときの2度 build-gnu.bat が呼ばれるが、2回とも sakura.exe を作成している。2回目はリンクだけだがそれでも10秒20秒はかかる。

appveyor.yml に clone_depth を指定することでビルド最初期の git clone におけるログの取得数を定数にできる。コミットが殺到してキューが伸びたときにビルド対象のコミットが取得できなくなることがあるらしいので、1 など極端な数字は NG。似たオプションに shallow_clone があるがこれは .git フォルダが作られないために必要な情報が得られなくなる。

分かれているために過半が重複しているテスト用のバッチ: create-project.bat と build-project.bat の統合。

エディタをビルドするオーガナイザ(build-all.bat)とテスト用の同等品(tests/build-and-test.bat)の統合。これも過半が重複している。

* コードの繰り返しを配列データに変換し、ビルド別に実行内容を取捨選択しやすく。
* バッチの実行環境(echo, PATH, カレントディレクトリなど)をバッチ毎に切り離す。
* 各バッチの実行時間をログで確認できるように。
* tests1.exe をターゲットにすることで gmock, gmock_main のビルドを省略する。
* pushd, popd の使用を減らし、バッチ中断時に変なディレクトリに残されないように。
* CMake のために sh.exe を PATH から外す対策をより汎用的に。
  ※遅延環境変数が必要だが PATH 操作を外部に作用させるために
    サブルーチン単位で SETLOCAL はできない。
* 継承・共有される環境変数 ERROR_RESULT の使用をやめ、SETLOCAL する。
* create-project.bat と build-project.bat のサブルーチンは完全に一致する。
  いずれ両バッチを統合したい。
* run-tests.bat が対応できておらず、pushd が常に失敗していた。
  pushd  C:\projects\sakura-clone\tests\build\%platform%\bin\%configuration%
  actual C:\projects\sakura-clone\tests\build\Win32\unittests\Win32\Release\tests1.exe
  actual C:\projects\sakura-clone\tests\build\MinGW\bin\tests1.exe
* MinGW ビルドではディレクトリにコンフィグ(Debug/Release)の区別がなかった。
* --gtest_list_tests オプションを内包するだけの run-tests.bat を削除する。
  代わって appveyor.yml にテストプログラムのパスを記述することで無駄な探索をやめさせる。
@ds14050 ds14050 added refactoring リファクタリング 【ChangeLog除外】 CI appveyor など CI 関連 【ChangeLog除外】 labels Nov 16, 2018
@m-tmatma
Copy link
Member

#617 とぶつかっちゃったー

@berryzplus
Copy link
Contributor

#617 とぶつかっちゃったー

ブーブーww

👎 :-1: のエモ字はきっとこういうときに使うものに違いない。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 17, 2018

マージができないだけでレビューはできますよ? 遠慮なくどうぞ。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 17, 2018

ブラウザでコンフリクト解消できちゃいました。

でもこのあと git pull するのを忘れていつものように追加修正を git push --force するとマージコミットが消えちゃうんだろうなあ。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 19, 2018

この PR に魅力がない可能性にはあえて目をつぶって書きますが、みなさん @m-tmatma さんの出方を窺っているか、@m-tmatma さん案件だとして沈黙を守っているんですよ。観念して態度だけでも表明してください。

@berryzplus
Copy link
Contributor

ぶっちゃけると「分からん」のです。
こう言うと何が分からん?と言われそうなんですが、
何がしたいのか分からん部分がある、といったらよいでしょうか。

ぼくが見るときの基準は、まず目的が妥当であるか否か。
何度か書いていますけど、ほぼそれが全てです。
このPRに反応してないのは目的が一部よく分らなかったからです。

誰かがコメント付けたら詳細説明が出るんじゃないかと思って静観していました。

分かっている部分についての判断と分からない部分はこんな感じ。

  • バッチに計時機能を追加したい(賛成
  • 複雑化して分かりづらくなってきたのでリファクタリングしたい(賛成
  • テストバッチにビルドターゲット指定を付けたい(なかったんですね。付けたくないけど許容です
  • ビルド産物置き場を指定したい(この変更については意図すら理解できてないです

@m-tmatma
Copy link
Member

ぱっと見、よくわからなかったし、残件が書いてあったので放置してます。

対象領域が被っているためにひとつの PR にまとめていますが、3つのコミット毎に主眼が異なります。

やることが違うなら、別 PR とした方がいいと思います。

変更箇所単位ではなく、ユーザー視点、あるいは開発者視点で影響のある単位で
細かく分けた方が、レビューする側も楽ですし、途中で方針変更してやり直す場合の
手戻りも少ないです。

一連の処理でひとまとまりになるなら、チケットを作った上でやりたいことを
列挙してしてから、その単位ごとに PR を投げた方がいいです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

テストバッチにビルドターゲット指定を付けたい(なかったんですね。付けたくないけど許容です

付けたくない理由というのを教えてほしいです。

現在使用していない gmock, gmock_main のビルドを外すために指定しましたが、いずれ使用するので目的として近視眼的であるとか、tests1 という特定のひとつのターゲットを埋め込むことがやはり将来のテスト構成にそぐわないとか、そういうことでしょうか。

モックを使ったテストや複数のテストバイナリが存在する構想が持てないでいるので、リファクタリングの結果が現在の構成に近づきすぎているならそれも教えてほしいです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

ビルド産物置き場を指定したい(この変更については意図すら理解できてないです

コミットコメントを引用します。ひとつの修正目標で3つの効果を狙っています。

  • run-tests.bat が対応できておらず、pushd が常に失敗していた。
    pushd C:\projects\sakura-clone\tests\build%platform%\bin%configuration%
    actual C:\projects\sakura-clone\tests\build\Win32\unittests\Win32\Release\tests1.exe
    actual C:\projects\sakura-clone\tests\build\MinGW\bin\tests1.exe
  • MinGW ビルドではディレクトリにコンフィグ(Debug/Release)の区別がなかった。
  • --gtest_list_tests オプションを内包するだけの run-tests.bat を削除する。
    代わって appveyor.yml にテストプログラムのパスを記述することで無駄な探索をやめさせる。

Win32/x64 版テストプログラムが pushd するディレクトリとは違う場所にできていて、テストプログラムの探索が偶然により成功していた理由は、確かめていませんが自分の変更のあとからです。今回の PR に含まれている tests/unittests/CMakeLists.txt への変更でそういう副作用のない方法に修正しました。

MinGW 版のテストプログラム置き場をコンフィグ別に分けない理由がないと思ったので分けました。そうすることで他のビルドと処理の共通化が図りやすくなります。具体的にはテストプログラムの探索と appveyor.yml に記述するテストプログラムのパスです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

ぱっと見、よくわからなかったし、残件が書いてあったので放置してます。

残件とは開発中に目に付いた TODO ということで、自分のやり残し(WIP)という意味ではありませんでした。

やることが違うなら、別 PR とした方がいいと思います。

目的は同じですが芋づる式に修正が連鎖していきました。

一連の処理でひとまとまりになるなら、チケットを作った上でやりたいことを
列挙してしてから、その単位ごとに PR を投げた方がいいです。

それぞれが密に依存しているために独立した PR を作成するためには3つのコミットを作り直すのと同じ手間がかかりますし、どれかひとつをマージしたあとで他の2つをマージ可能な状態にするのにはそれとは逆方向の同じ手間がかかります。

いったんこのままにして、マージすべきでないと判断されるコミットがあればそのときにこの PR から外させてほしいです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

リファクタリング部分を無視すればそれぞれのコミットで見るべき点はシンプルです。

  1. Platform-Config 別に取捨選択されたビルド内容(※以前の通りにもできます)と計時機能。
  2. ビルドターゲット指定の追加。
  3. AppVeyor にテストプログラムの探索「Discovering tests...OK」をやめさせる。

@berryzplus
Copy link
Contributor

最後の趣旨了解しました。

目的には大筋で合意です。
実現方法がたぶん反対です。
中身見られてないので違うかもしれません。

とりあえずレビューできる状態になりました。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

とりあえずレビューできる状態になりました。

コミットの構成を間違えていたと考え直しました。レビューはお待ちください 😔

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

コミットを再構成してレビューしやすくなった #626 を作成しました。こちらは閉じますのでお手数をおかけしますが移動をお願いします。

@ds14050 ds14050 closed this Nov 20, 2018
@berryzplus
Copy link
Contributor

とりあえずレビューできる状態になりました。

コミットの構成を間違えていたと考え直しました。レビューはお待ちください 😔

タイトルは引き継ぎなんですね。
狙いが達成できたのかどうか地味に気になっています。主観で構わないので、根拠と判断が欲しいです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

時間とサイズを軽減するには #427 の議論に結論を出す必要があります。この PR はその下地作りの意味が大きいので、実際の効果を測定はしていません。

たとえばここを引き継いだ #626 と master のビルド時間の比較が以下の通りですが、やっていたことをやっていないのだからある意味当たり前の結果です。

Job Duration Duration(master) 要因
Win32/Release 3:52 5:32 減少 ⇐ Cppcheck を実行していないから
x64/Release 4:53 6:38 減少 ⇐ Cppcheck を実行していないから
MinGW/Release 3:16 3:15 同じ ⇐ 元々 Cppcheck を実行していなかったから
Win32/Debug 5:05 5:10 同じ ⇐ chm とインストーラを作成しなかったが、それらに時間はかかっていなかった。アーティファクトのサイズは減っている。
x64/Debug 6:26 6:46 同じ ⇐ chm とインストーラを作成しなかったが、それらに時間はかかっていなかった。アーティファクトのサイズは減っている。
MinGW/Debug 4:16 3:54 増加 ⇐ なんで?

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

この PR の最大の効果は #626 の2番目以降のコミットの差分の小ささに見られます。変更をもたらさない1番目のコミットによるリファクタリングが下地を作っているからです。

2番目の効果はコマンドインタープリタの実行環境をよりよく制御することにあります。

ささやかな違いですが zipArtifacts.bat を呼び出した後でバッチ行のエコーバックが ON に変更されていたものが影響されなくなります。

スタティックリンクしていなかったテストの実行にはビルド時と同じ bin ディレクトリへパスが通っている必要がありましたが、これまではビルド時に設定したパスが暗黙に影響していたために不都合がありませんでした。run-tests.bat だけではテストを実行できなかったのです。呼び出す側と呼び出された側、親子でぐずぐずに影響を与えあう環境は不健全で制御されているとはいえません。ファイルを分ける意味がない。

@berryzplus
Copy link
Contributor

今日中に結論までいけそうにないので濁していた部分だけ追記します。

自分が書いたコメント

目的には大筋で合意です。
実現方法がたぶん反対です。
中身見られてないので違うかもしれません。

これは「3.AppVeyor にテストプログラムの探索「Discovering tests...OK」をやめさせる。」が「言葉通りなら反対」という意味でした。

・テストプログラムの探索はさせたいです(複数exeになる可能性を考慮しています
・探索に時間を食うなら、効率良く探せるように手を打ちたいです(環境変数?設定系で手が打てる気がします

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

zipArtifacts.bat を呼び出した後でバッチ行のエコーバックが ON に変更されていたものが影響されなくなります。

lastexitcode を GREP してもサクラのソースには存在しない。このメッセージはどこから出ていたんだろう。

---- end   zipArtifacts.bat ----
C:\projects\sakura>exit /b 0 
C:\projects\sakura>set lastexitcode=0 
C:\projects\sakura>set  1>C:\Users\appveyor\AppData\Local\Temp\1\tmpD6BA.tmp 
C:\projects\sakura>echo C:\projects\sakura  1>C:\Users\appveyor\AppData\Local\Temp\1\tmpD6BB.tmp 
C:\projects\sakura>exit /b 0 

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

探索に時間を食うなら、効率良く探せるように手を打ちたいです(環境変数?設定系で手が打てる気がします

現在は tests1.exe だけなので、この PR では tests1.exe のパスを AppVeyor に教えています。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

これまでのようにビルドの一環としてテストを実行してしまうとテストに失敗しただけで sakura.exe が手に入らなくなってしまうので、AppVeyor のテストフェイズを利用しようという狙いがあります。あくまでも狙いであって、テスト失敗を AppVeyor に通知してしまうとアーティファクトの収集フェイズが始まりません。

テストプログラムの探索「Discovering tests...OK」

これの意味は「OK、見つからなかったよ」です。

@k-takata
Copy link
Member

lastexitcode を GREP してもサクラのソースには存在しない。

AppVeyorが用意しているバッチファイルかもしれません。
echo onのままAppVeyorに制御を返すとそんな感じの表示が出た気がします。

@berryzplus
Copy link
Contributor

やっぱ勘違いしてそうですわ。
週末に時間をとって内容見てからまた書きます。

やっぱりテストフェーズの拡張ポイントってあるんですよね?
いままではビルドフェーズのバッチをテストに流用してたけどそっちを使おう、と。
当座でパスを直書きする方法が使えそうだからそうしよう、と。

ちゃんと見てから書きまっす。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

ところが zipArtifacts.bat で全ての echo を眺めてみましたが echo on とはしていないのです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 20, 2018

tools/zip.bat の最終行が echo on でした。

#626 の1番目のコミットによるリファクタリングの第2の目的が、こういう影響を排除することです。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI appveyor など CI 関連 【ChangeLog除外】 refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants