[WIP] AppVeyor のビルドキャッシュを利用してビルド時間を削減(※およそ５分)できるようなオプションを用意します。 #650
Conversation
ds14050
changed the title
There was a problem hiding this comment.
LGTMです。
ここのリポジトリの設定を変えるのではなく、変えられる枠組みを作るのが目的のPRと理解しています。sakura-editor/sakura の master の env:cache を Enabled にする機会はこれからもないと思うんですが、「ローカルリポジトリでの開発時に不便」という問題の解決にはなると思います。
キモチ的に approve なんですが、approveにするとマージできちゃうので、
少なくともあと1人の賛同を得てここで賛同表明してもらってください。
責任かぶるのは構わないけれど、独断はマズい気がするので。
|
全くドキュメントがないので、ドキュメントを追加する必要があると思います。 powershell のスクリプトの処理内容や、キャッシュの仕組とか このままでは誰も変更できないコードになってしまいます。 |
|
ドキュメントが足りないのはわかりますし付けることも吝かではありませんが、付けるならコードを書いた人間にしか書けないこと、つまりは「意図」が主になります。「事実」はコードがそれですのでコメントで繰り返すことはしません。AppVeyor のことは AppVeyor が提供する一次ソースに当たってもらうのが前提です。 とりあえず何がわからないのか手当たり次第に挙げてもらえると大変助かります。何が不明なのか書いた本人にはよくわからないものです。 |
|
あ、ドキュメントとコメントを混同してるや。 |
それでいいと思いますよ。 バッチの説明なんで、何をしているか、どうやって起動するか、結果としてどうなるか、を網羅できているのが理想だと思っています。たたきで軽くテキスト化してもらってもいいですし、コミット積んでもらってもいいです。コミットな場合、バッチ関連は「バッチ名.md」を作るのがなんとなくの法則になってる認識です。 |
|
appveyor.yml って日本語が書けるのでしょうか。あまり長々とコメントを書いても邪魔になりますし、一案ですが、この PR へのリンクで済ませることはできないでしょうか。 |
バッチではなく PowerShell スクリプトなんですが、「RestoreCommitterDateRoughly.ps1」という名前がすべてです。……ということを言ってしまえるのは書いた本人だからなのかもしれませんが、これ以上言葉を尽くすことが無駄に思えてしまいます。 |
ds14050
force-pushed
the
RestoreCommitterDate
branch
from
c7c72da
to
bfa438c
Compare
コメントではなく markdown を想定しています。 |
このpowershellスクリプトが何をするもので、どういう呼出し方をされると想定していて、処理結果として何が起こるか、を書いてもらえればいいっす。バッチじゃなくてpowershellっす、という返しは華麗にスルーしたいところですが、ちょっとだけ食いついてみました。
スクリプトの目的はこのコメントだと思います。 → 何をするものか、に書けばいいはず。 どうやって起動するか、については、想定する呼出し方法（おそらく実際の呼出しのコピペ）を書いてもらえればよいはず。 処理結果がどうなるか、については「名前がすべてを語る」ということなので、 |
ds14050
force-pushed
the
RestoreCommitterDate
branch
2 times, most recently
from
e838f38
to
8e1ee11
Compare
|
appveyor.md に追記しました。 |
|
最近 PowerShell のヘルプを読んで勉強しています。統合されたヘルプシステムに則って RestoreCommitterDateRoughly.ps1 にコメントを付けてみました。 なんていうんでしょうね、あの青い画面を。 |
ds14050
force-pushed
the
RestoreCommitterDate
branch
from
8e1ee11
to
5c1f569
Compare
ds14050
force-pushed
the
RestoreCommitterDate
branch
from
5c1f569
to
fa5b5af
Compare
せっかくの approve とコメントを頂きましたがさっそくコメントを修正してしまいました。 キャッシュを有効にするのに手順１と手順２の両方が必要だと誤解されるおそれがあったため、「手順１: Web ページでの設定」を「Web ページで設定する場合」という具合に表現を改めました。 |
ds14050
force-pushed
the
RestoreCommitterDate
branch
2 times, most recently
from
26f5e4e
to
fe22bb8
Compare
| @( | ||
| "?$($Oldest.ToString("yyyy-MM-dd 00:00:00"))" | ||
| , (git ls-files --full-name "$TopDir") | ||
| , (git log --format=format:?%ci --name-only --since=$($Oldest.ToString("yyyy-MM-dd")) --reverse) |
There was a problem hiding this comment.
--since=$($Oldest.ToString("yyyy-MM-dd")) は単に --since="1 month ago" とすればいいと思います。
参考: https://stackoverflow.com/questions/14618022/how-does-git-log-since-count
There was a problem hiding this comment.
そう書けるということは知りませんでした。それによって $Oldest が省けるならそうしようと思いましたがどうもそうではありませんでした。その場合 --since="1 month ago" の使用は「そうすべきである」でも「そうすることもできる」でもなく「そうすべきではない」ものになると思います。「１か月」というキーワードがスクリプト中の２か所に現れることになるからです。
There was a problem hiding this comment.
論点を勘違いしていました。時分秒を付けます。
| } elseif ($_[0] -eq "?") { | ||
| $t = $_.Substring(1) -as [DateTime] | ||
| } else { try { | ||
| ([System.IO.FileInfo]"$TopDir\$_").LastWriteTime = $t |
There was a problem hiding this comment.
インデントが読みにくいので以下のような感じにしたほうがいいと思います。
} else {
try {
([System.IO.FileInfo]"$TopDir\$_").LastWriteTime = $t
} catch {
Write-Debug($_)
}
}
There was a problem hiding this comment.
try も catch も無視したい物で、それによってインデントが狂うのが嫌なんですよね。Ruby なら迷わずこう書くところです。
([System.IO.FileInfo]"$TopDir\$_").LastWriteTime = $t rescue Write-Debug($_)
There was a problem hiding this comment.
下記のような記述はどうでしょうか？
} else {
try { ([System.IO.FileInfo]"$TopDir\$_").LastWriteTime = $t }
catch { Write-Debug($_) }
}
There was a problem hiding this comment.
異議２人目。加えて周到な例示。コンパクトな方を採用しました。
ds14050
force-pushed
the
RestoreCommitterDate
branch
2 times, most recently
from
102e11f
to
0e583da
Compare
-m オプションに対する考察先に紹介したサイトの変更履歴に、git log に -m オプションを付加するものがあります>「Latest revision as of 17:16, 5 October 2012 (view source) Nihen (Talk | contribs)」 -m オプションの説明はこうです。
理解できなかったために敢えてオプションを付けることはしませんでしたが、ようやくおぼろげにわかってきたことがあります。 -m オプションなしでは中身のないマージコミットがファイルへの修正を行ったとは見なされない。「中身のないマージコミット」とは２つのコミットオブジェクトを結び付けるだけの、典型的には「Merge pull request #XXX from berryzplus/feature/adjust_searchbox」のようなメッセージで表されるコミットのことです。 これが適するのは対等な２つのブランチが合流して１つのブランチを作る場合です。ファイルに対して実際の変更が行われたと見なされるのは(更新日時が取得されるのは)、それぞれのブランチで実際に作業をしてコミットした日時になります。 -m オプションありではマージコミット時点で「も」同じ修正を行ったとみなされる。これが適するのはマージの 一方 がいわゆる「master」ブランチの場合です。master ブランチの歴史を遡る際に重要なのはブランチでの作業日時ではなく、ブランチでの一群の修正が master に統合された日時です。-m オプションを付加することでマージコミットの日時が「最終」更新日時として取得されるようになります。 マージの「一方」のブランチを特別扱いする --first-parent オプション-m の説明にも登場していましたが、独立した説明はこうです。
これが意味するところは実際の例を見てもらうとわかりやすいと思います。オプションなし、-m オプションのみ、-m --first-parent オプション付きの３通りの結果です。
-m オプションのみの場合に、双方のブランチから見た修正内容が２つのコミットとして出力されています。一方が長大なリストを出力しているのはブランチの側で master を追いかけて pull/merge/rebase した結果だと見られます。明らかに無視すべきものです。 まとめ-m --first-parent オプションを付加するのが使用状況により適していると考えられますので、これから追加コミットをする予定です。 余談以下も含めてすべて単一の git log のヘルプページに書いてあるんですよね……。気がつかないし、一読しても熟読してもそうとわかるまではわからないのだけども。
|
|
これ以上の修正はない予定です。 追記 この修正自体予定外のものですが、現在特に気になっていることはありません。それを言うと -m オプションのことも(リンクを張って紹介するまでは)忘れていたぐらいで気にしていなかったのですが……。 |
|
googletest で cmake 自体使用しないようにする試み |
この PR には関係しません。むしろ build-and-test.bat, build-gnu.bat が切られ Visual Studio しかサポートされなくなれば、自分の環境に関してですが、ローカルビルドが不可能になり AppVeyor が唯一可能なビルド環境になります。この PR への依存が増します。 |
|
ビルド時間を削減することが目的だと思うので、ビルド時間全体が大幅に短縮されると |
|
自分は「いつでも」クリーンビルドをする人間ではありませんし、他の人もそうはしないと思っています。 |
|
#650 (comment) に関する雑談です。 承知していない事実を指摘され思わず迎合してしまいましたが、じっくり考えた結果、$Oldest 変数の文字列化は早すぎる最適化だったのではないかとの思いを強くしています。 PowerShell ではパイプを通してコマンド間でオブジェクトをやりとりできます。日時の文字列表現でなく、UNIXエポックでなく、型付きの日時をやりとりできます。コマンドの最初から最後まで型を維持するのが自然なやり方ではないのか。 また、コメントで「$Oldest を利用する２か所(それで全部)」「$Oldest の利用場面がその２つに限られる現状」と限定条件を繰り返したように、'YYYY-MM-DD HH:MM:SS' フォーマットが通用しない第３の利用場面が出現すれば、文字列表現を再解釈したり置換したりするのではなく $Oldest 変数を DateTime 型に戻すのが当然の手段です。結果的に無駄になる早すぎる最適化だったのではないか。 @berryzplus さんが書きました。「リテラル文字列 "yyyy-MM-dd HH:mm:ss" の登場回数を減らすことができ、配列に詰める値が「同じ」であることをより明確に表現できると思います。」 配列に詰める値は論理的に「同じ」ものではありません。同じであることが要求されているのは「配列に詰めた $Oldest の文字列表現(YYYY-MM-DD HH:MM:SS)」と「git log が パラメータ --format=format:?%ci に従って出力したコミッタ日時の文字列表現」であって、「git log に与える --since パラメータの値」は git が解釈できるどんなフォーマットであっても構いません。%ci のフォーマット(とそれに合わせて配列に詰めた $Oldest の文字列表現)と --since パラメータに与える $Oldest の文字列表現が共通していたのは「たまたま」です。 「たまたま」を共通化すると某所で「駄目なDRY」と呼ばれた害を生みます。 以上、独り言でした。 コンフリクトはレビューや Approve の障害ではないと考えています。追加コミットのたびに Approve を繰り返すのも手間でしょうが(設定を変更しませんか？)、master に追従して際限なく rebase を繰り返すことはできません。Request を出している側が譲歩すべきなのかもしれませんが、一方的に負担することもやはり無理です。 |
|
ちょっとだけ雑談追記。 「同じ」に関して、「日時が同じ」と「フォーマットが同じ」を区別し損ねていました。 @berryzplus さんが書いたのは「日時が同じ(ひょっとしたらフォーマットも同じ)」であり、自分が書いたのは「フォーマットが同じ」である別物でした。 これを区別することでどういう違いが生じるかというと……
両方を同時に満たすためには、やはり、事前に文字列化はできませんでした。 事前の文字列化により必要な共通性(git log --format と $Oldest のフォーマット)がひとつところで読み取れなくなり、不必要な同一性(git log --since と $Oldest が日時に加えてフォーマットも同じ)が読み取りやすくなっただけでした。 |
|
ややこしいんですけど、 #796 は CMake を使わないための対応でなくて、テストプロジェクトを vs2017 ソリューションに組み込む対応で、vs2017が標準でサポートしている Test Adapter for Google Test の要件を満たすために NuGet パッケージを利用するようにしています。厳密には CMake を使わないための対応じゃなくって、むしろ #796 の対応の後に「ソリューション自体を CMake に対応させる」を考えています。ソリューションのCMake対応が完了すると、晴れてMakefileMakeとsakura_core/Makefileを消しても MinGW ビルド環境を維持できるようになります。 んで、この PR をマージしない理由ってなんなんでしたっけ？ｗ |
というのは
に対する反応ですね。誤解はしていないので大丈夫です。仮に MSVC でのビルドがソリューションファイルに一本化されたとしても、Makefile を使う MinGW の状況は変わりませんから、現状の CMake を使うインフラが必要です。 心配したのは「googletest で cmake 自体使用しないようにする」ことが目的化して、build-and-test.bat とそこから呼ばれる build-gnu.bat、ひいては MinGW ビルドのメンテナンスが切られることでした。 |
* Enabled only when env:cache has a value:enabled. This can be set by Web UI, which overrides env. vars set by appveyor.yml
宣言時に文字列変換まで済ませてしまうことにより、「.ToString("yyyy-MM-dd HH:mm:ss")」のフォーマットがズレるリスクを回避します。
ds14050
force-pushed
the
RestoreCommitterDate
branch
from
e451663
to
630c339
Compare
前回の同じ質問のときから状況は変わっていません……と思いましたが、２つある Unresolved がどちらも Outdated なのが違うかもしれません。 次に Approve があればさっさとマージしてしまいます。 このコメントの直前の force-push は最新の master へのリベースによるものです。appveyor.yml のコンフリクトを解消しました。失敗して変な差分が appveyor.yml に生じているということもないようです。 |
これが正しいあり方だとは思わないが、build-installer.bat が呼び出しに 際して暗黙の前提をいくつも置いているので、それに則って、postBuild.bat が期待に応えられるように修正する。
ds14050
force-pushed
the
RestoreCommitterDate
branch
from
6d3d4d3
to
19c8482
Compare
|
うっかり個人テスト用のコミットを PR ブランチにプッシュしてしまいました。しばらく WIP です。 |
ds14050
changed the title
|
ちょっと、根幹に、問題ががが……。 |
|
このPRは賞味期限切れだと思うので閉じておきます。 |
Closes #637 「ビルドキャッシュを使用するとすごく早いです。」
コミットメッセージ
Save build time by caching intermediates.
This can be set by Web UI, which overrides env. vars set by appveyor.yml
効果
省略されるのは変更されなかった .cpp ファイルのコンパイルと CMake によるプロジェクト生成(googletest)です。リンク時間は減らないのでリンク時コード生成が有効なリリースビルドでの効果は相対的に下がります。
tests/create-project.bat がビルドディレクトリの明示的削除を行っているので、CMake によるプロジェクト生成(unittests)を強制してキャッシュの効果を打ち消しています。
sakura/tests/create-project.bat
Lines 28 to 32 in 5524485
比較
条件
環境変数 cache が enabled という値を持っているときに限りキャッシュが有効になります。これは appveyor.yml で設定することができますし、アカウントの Web UI で設定した値で appveyor.yml の値を上書きすることもできます。
キャッシュ容量が有限であることと、クリーンな状態でビルドしたい要求があるだろうと考えて、リポジトリごとに提供するオプションです。
つまり、この PR をマージした時点でキャッシュが有効になるわけではないので、過度に慎重な判断は不要です。
副作用
キャッシュを削除する方便として appveyor.yml をキャッシュエントリに指定しています。これは PR をマージした時点で有効になります。
キャッシュされた appveyor.yml がビルドの邪魔になるケースでは、Web UI から appveyor.yml に代わるファイルの場所を指定する方法と、同じく Web UI により appveyor.yml ファイルを無視するチェックを入れる方法があります。
単純に空のキャッシュを指定しただけではキャッシュの削除が行われないために、あたりさわりのない有効なキャッシュエントリを指定することで、その他のキャッシュを削除する方便としています。そうしなければ環境変数でキャッシュを無効化した後もキャッシュが残存してしまいます。
キャッシュ容量の計量がアカウント毎ではなくプロジェクト毎リポジトリ毎であればキャッシュの残存を気にする必要はありませんが、そうではないようです。
Build cache | AppVeyor
らしいです。