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

ビルドキャッシュを使用するとすごく早いです。 #637

Closed
ds14050 opened this issue Nov 23, 2018 · 19 comments
Closed

ビルドキャッシュを使用するとすごく早いです。 #637

ds14050 opened this issue Nov 23, 2018 · 19 comments
Labels
CI appveyor など CI 関連 【ChangeLog除外】

Comments

@ds14050
Copy link
Contributor

ds14050 commented Nov 23, 2018

AppVeyor にはビルドキャッシュという機能があり、Job 毎にビルドをまたいで中間生成物を再利用できます。フリー版では 1 GB の容量がありますが、サクラエディタでは1JOBあたり 70MB の中間生成物が生まれます。PR ブランチに限りデフォルトで無効になっているようです。

適切に依存関係を設定して、不必要にファイルを新しくしなければ 、githash.h をインクルードしている CDlgAbout.cpp と、コミットで手を入れた .cpp ファイルを再コンパイルするだけでエディタとテストが完成します。

master...ds14050:cmakebuild ブランチの「AppVeyor integration (Use build cache).」というコミットで実験しましたが、現在の master で 37 分かかっているものが 20 分で済みました。

プロジェクト設定に不手際がなければ、エディタとテストのビルド産物の場所と、Funccode_enum.h、Funccode_define.h の場所をキャッシュに指定するだけです。Funccode_enum.h のキャッシュを忘れると StdAfx.h が新しくなるので全てが再コンパイルされてしまいます。

ひとつだけ仕込みが必要なのが、Git がファイルのタイムスタンプを保持してくれないために更新日時がチェックアウト日時になってしまい、ソースファイルが必ずキャッシュした中間生成物よりも新しくなってしまうことです。対処するスクリプトが先のコミットに含まれています。

おそらく sakura-editor/sakura/master のビルドよりも、開発中のブランチで役に立つ内容だと思います。AppVeyor アカウントでキャッシュを設定するだけでビルドが早くなるように準備が整った状態に持って行きたいと考えています。

@m-tmatma
Copy link
Member

CI はクリーン環境でビルドしたいと思います。

理由は、ビルド用のバッチファイル等ビルド周りでバグがあったときに
キャッシュを利用してビルドする場合に、発見が遅れたりキャッシュ利用環境で
しか発生しないビルドエラーや実行時の問題が発声したり余計なリスクを持ち込む
可能性があるためです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 27, 2018

CI はクリーン環境でビルドしたいと思います。

ですからオプションにしてデフォルトでオフなんです。

しかしクローンした個人のリポジトリでトピックブランチを開発中には早さが欲しくなります。Issue 本文にはこう書きました。

おそらく sakura-editor/sakura/master のビルドよりも、開発中のブランチで役に立つ内容だと思います。AppVeyor アカウントでキャッシュを設定するだけでビルドが早くなるように準備が整った状態に持って行きたいと考えています。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 27, 2018

政治の世界ではこういうのは低いハードルで導入してからなし崩しに対象を拡大していく意図があるんだろうと読まなければいけないらしいですが、論理の世界では書かれていないことを前提に持ち込むことは禁止です。もちろん私は論理の世界の住人です。

@beru beru added the CI appveyor など CI 関連 【ChangeLog除外】 label Nov 27, 2018
@m-tmatma
Copy link
Member

しかしクローンした個人のリポジトリでトピックブランチを開発中には早さが欲しくなります。Issue 本文にはこう書きました。

おそらく sakura-editor/sakura/master のビルドよりも、開発中のブランチで役に立つ内容だと思います。AppVeyor アカウントでキャッシュを設定するだけでビルドが早くなるように準備が整った状態に持って行きたいと考えています。

そのことも反対の理由の一つです。

ビルドする条件によって、実際のビルドの前提条件が異なるので、
ある条件では問題が発生するが、別の条件では問題が発生しないという
ことが発生するからです。

KISSの原則 が念頭にあります。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 28, 2018

  • ビルドが遅いのは潜在的な未来の問題ではなく今存在している問題です。
  • 前提条件の違いは公式のリポジトリとクローンしたリポジトリの間の違いであって、片方のリポジトリに着目した場合に違いは存在しません。
    公式のビルド結果以外を気にかける理由はありません。

@berryzplus
Copy link
Contributor

appveyorのビルドをキャッシュするのには否定的です。

この考え方は条件的で、
もしサクラエディタが設計的に完成されたアプリであったなら
賛成できる余地があったと思っています。

ちょっとしか変わらないなら有効だけど、
大きく変わるならマズいと思っています。
キャッシュに使える領域の想定ですが、 #514 とかが関係してそうです。
認識する限り、公式資料の制限値は既にオーバーしてるはずです。

ビルドが遅いのは現実の話で「なる早対応が必要」というのは同意できます。
解決策として「キャッシュを使う」のに、単に疑問を持っています。
代案はあっちに書くべきだと思うので、 #427 に書きます。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 28, 2018

もしサクラエディタが設計的に完成されたアプリであったなら賛成できる余地があったと思っています。

公式リポジトリの話はしていません。開発者のイテレーション速度の話です。

ちょっとしか変わらないなら有効だけど、大きく変わるならマズいと思っています。

何について話していますか。

キャッシュに使える領域の想定ですが、 #514 とかが関係してそうです。

根拠のない話です。

ここと PR( #650 )に書きましたが、キャッシュ容量は 1 GB で、中間生成物は 70 (MB/JOB) × 6 JOB = 420 MB です。そしてこの容量をサクラエディタアカウントで消費しようとは計画していません。

@berryzplus
Copy link
Contributor

もしサクラエディタが設計的に完成されたアプリであったなら賛成できる余地があったと思っています。

公式リポジトリの話はしていません。開発者のイテレーション速度の話です。

もう少し詳しくお願いします。
ぼくは appveyor でのビルドの話(=公式リポジトリの話)だと思ってみています。
m_tmatmaさんもたぶん同じだと思います。

ちょっとしか変わらないなら有効だけど、大きく変わるならマズいと思っています。

何について話していますか。

ソースコードの変更量について「ちょっと変わる」場合と「大きく変わる」場合を話しています。
キャッシュの話なので、変更内容によって効果は大きく変わります。

キャッシュに使える領域の想定ですが、 #514 とかが関係してそうです。

根拠のない話です。

sakura-editorのリポジトリでキャッシュが使える根拠がない、と言ってますか?
その通りだと思います。容量を使いきっているので使える保証はないと思っています。

ここと PR( #650 )に書きましたが、キャッシュ容量は 1 GB で、中間生成物は 70 (MB/JOB) × 6 JOB = 420 MB です。そしてこの容量をサクラエディタアカウントで消費しようとは計画していません。

だから、なんで sakura-editor/sakura へのPRでローカルビルドの話がでてくるのか説明してください。

個人的なリポジトリ上にブランチ切って派生させたら済む話ではないの?
ぼくgithub詳しくないので、そうしたらあかん理由を想像できないです。
ある程度考えて書いてると思うんですが、公式に反映させる気がないのにPRする理由がわからんです。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 28, 2018

ソースコードの変更量について「ちょっと変わる」場合と「大きく変わる」場合を話しています。
キャッシュの話なので、変更内容によって効果は大きく変わります。

何がまずいのかがやはりわかりません。ソースコードの変更量が多かったりヘッダを変更した場合には再コンパイル対象が増えてキャッシュの効果は減るでしょうが、そのことがまずい理由がありません。

sakura-editorのリポジトリでキャッシュが使える根拠がない、と言ってますか?

違います。berryzplus さんが書きましたが、アーティファクトのサイズとキャッシュ容量には関係がないと言っています。使える領域を「想定」してはいません。AppVeyor の公式ドキュメントを参照して示しています。

個人的なリポジトリ上にブランチ切って派生させたら済む話ではないの?
ぼくgithub詳しくないので、そうしたらあかん理由を想像できないです。
ある程度考えて書いてると思うんですが、公式に反映させる気がないのにPRする理由がわからんです。

説明します。クローンしたリポジトリには公式リポジトリの appveyor.yml が付いてきます。自分個人が動かしている AppVeyor もこの appveyor.yml に従ってエディタをビルドします。これが遅いです。ブランチを切るたびに目的とは異なる appveyor.yml への修正コミットを積めばたしかに個人でキャッシュを利用することができますが、できれば省きたい作業です。

この Issue の目的は開発環境を整えることです。以前に githash.h が常に新しくなるためにほとんどのソースコードが再コンパイルされることが問題になり修正されました。これを AppVeyor でのビルドにも適用しようというものです。AppVeyor は開発ツールです。特にローカルで Visual Studio のビルドとリビルドを使い分けられない自分には欠かせないツールです。

@berryzplus
Copy link
Contributor

主題について、了解しました。

sakura-editor/sakuraリポジトリへのPR前の確認をできるだけ省力化するための試み、ということで。
(言葉尻、「ちょっと違う」が指摘ありそうですが、そんな感じと理解しました。)

意味合い的に、ビルドキャッシュを使うようにする、という内容ではなく、
ビルドキャッシュを使える環境を整える、というものなわけですね。
少し前のコメントで「sakuraリポジトリでは有効にしません」と言ってたのが繋がりました。

たぶん、今の状況は前提のアリ/ナシでNG判断で中身見てない状況だと思いますので、
そこが解決すれば先に進んでいくんじゃないかと思っています。

そういう話なら入れてもいいんじゃないかと思いますが、みなさんどうでしょう?

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 29, 2018

意味合い的に、ビルドキャッシュを使うようにする、という内容ではなく、
ビルドキャッシュを使える環境を整える、というものなわけですね。

やっとたどり着いてもらえましたか(感謝)。Issue 本文に書いた以下の内容を繰り返すのはこれが2回目です。これ以上のことは書いていません。

おそらく sakura-editor/sakura/master のビルドよりも、開発中のブランチで役に立つ内容だと思います。AppVeyor アカウントでキャッシュを設定するだけでビルドが早くなるように準備が整った状態に持って行きたいと考えています。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 29, 2018

よく見ると PR のタイトルが良くなかったですね。修正しました。

PR の本文にはこう書いていたので予想できない反発でした。

条件

環境変数 cache が enabled という値を持っているときに限りキャッシュが有効になります。これは appveyor.yml で設定することができますし、アカウントの Web UI で設定した値で appveyor.yml の値を上書きすることもできます。

キャッシュ容量が有限であることと、クリーンな状態でビルドしたい要求があるだろうと考えて、リポジトリごとに提供するオプションです。

つまり、この PR をマージした時点でキャッシュが有効になるわけではないので、過度に慎重な判断は不要です。

@ds14050
Copy link
Contributor Author

ds14050 commented Nov 30, 2018

@berryzplus さんが書きました。

たぶん、今の状況は前提のアリ/ナシでNG判断で中身見てない状況だと思いますので、
そこが解決すれば先に進んでいくんじゃないかと思っています。

そういう話なら入れてもいいんじゃないかと思いますが、みなさんどうでしょう?

他に最低1人の賛同者が欲しいと思っています> @sakura-editor/sakura-developers

@m-tmatma
Copy link
Member

m-tmatma commented Dec 1, 2018

一旦反対意見は白紙にしておきます。感情的になった面は否めません。
他の方の判断を入れるのがいいと思います。

@KENCHjp
Copy link
Member

KENCHjp commented Dec 1, 2018

CIの話なので初めからじっくり読みました。

・本家MasterにPRされるものはキャッシュは使わない(今まで通り)
・分家(Fork)側で、コード修正中はソース修正はそれなりに繰り返すので、キャッシュを有効化できるようにして効率化を図る。(1コミットで40分待つのはつらい)

ですかね?

リスクとしてキャッシュを使うようにした場合と本家PRの差がそこで顕著化する可能性があるので分家のAppVeyor が正常で本家側で問題が起こった時に切り分けが必要になってくるってかんじですかね?

作業の流れとしては、いったんローカルでソース修正、終わったら分家側プッシュ、生成物に問題なければ本家へPR、問題がでたら、再度ローカルでソース修正、終わったら分家側プッシュ(ここでキャッシュが利く?)、問題解決できれば本家へPR。

最後の本家へPRした後の生成物に問題が出る可能性がゼロではないんじゃないかというのが、 @m-tmatma さんの気になるポイントですよね。

この場合、分家側で、キャッシュ無でリコンパイルして、確認要って感じですよね?
分家側でのAppVeyor コンパイルの回数が多い場合にはキャッシュが有効で、問題が出れば、分家側キャッシュ無で再度コンパイルして再現するか確認すればいい話かな。

最後本家側は今まで通りキャッシュ無コンパイルするから問題ないし、分家側の生成物と必ずしも本家側が一緒にならないリスクは、分家側だけが負うリスクなのかなと(アセンブラレベルでマッチングできるんでしたよね?、それは分家側がやればこのリスクはなくなりますよね)。

レビューする側は、本家側の生成物で問題があれば、それを単にチェックすればよくて(今まで通り)、分家側生成物では問題ない場合は、PRした人の責任で調べる(いわるゆ「おま環」ってやつですかね?)運用って感じであってますかね?

この認識であってれば、ビルドキャッシュで作業がはかどるなら全然問題ないと思います。
(長くなった。。。)

@ds14050
Copy link
Contributor Author

ds14050 commented Dec 7, 2018

本日戻って参りました! (他の Issue/PR には数日前から顔を出していましたが……)

一旦反対意見は白紙にしておきます。

意思表明ありがとうございます。自分は一人で納得して ACK を出すのを忘れがちなので見習おうと思います。

@KENCHjp さん

すべてその通りでございます。

一度や二度で話が通じないことが多かったので自分の文章はさぞ読みにくかったろうと思います。自分で自分の日記を読み返してさえ「こいつは何を言ってるんだ」と話の飛び具合についていけなくなりますから。

おそらく想像が及んでいないだろう部分を補足すると、キャッシュは何十回何百回というレベルで効いてきます。自分が動かしている AppVeyor のビルドナンバーは730を数えています。

そしてこの後ですが、この Issue を実現する手段としてすでに PR #650 を出しています。内容については @berryzplus さんが OK を出していますから、@KENCHjp さんに GO サインだけ出してもらえると後の不具合はこちらで責任を持ちます。@m-tmatma さんに白紙撤回以上のことを要求することは自分にはできかねます。

@KENCHjp
Copy link
Member

KENCHjp commented Dec 7, 2018

@KENCHjp さんに GO サイン

GO!

@ds14050
Copy link
Contributor Author

ds14050 commented Dec 8, 2018

@KENCHjp さんをだしにして(笑) m-tmatma さんから #650 の方でコメントをもらいました。実効性のある GO (approve) は berryzplus さんが出してくれるようです。

本当にありがとうございました。

@berryzplus
Copy link
Contributor

興味ある人も居ないようなので閉じておきます。

必要あれば新たにissue立ててください。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI appveyor など CI 関連 【ChangeLog除外】
Projects
None yet
5 participants