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

appveyorのcppcheckとdoxygenインストールをchocolateyで行うように #702

Closed

Conversation

KageShiron
Copy link
Member

@KageShiron KageShiron commented Dec 13, 2018

関連: #653
cppcheckとdoxygenをappveyor.ymlのinstallセクションで行います。

また、cppcheckを1.86にアップデートしました。

@m-tmatma
Copy link
Member

appveyor のアカウントはお持ちでしょうか?

簡単にセットアップできるので、PR を投げる前に自分の fork に対する appveyor の
プロジェクトで 最低限 Win32 Release だけでもいいのでビルドが通るのを
確認していただけないでしょうか?

今回の PR の場合、appveyor でしかテストできない修正なので
一旦自分のappveyor アカウントでのテストが必要と考えます。

@KageShiron
Copy link
Member Author

KageShiron commented Dec 14, 2018

設定してみました。次回以降appveyor絡みは自分のほうで通ってからPR投げますね
(cinst忘れはコミット前に直した気がするんだけどなぁ うまく入ってなかったのか・・・ :thinking:)

@berryzplus
Copy link
Contributor

確認ですが、このPRの目的ってなんでしょうか?

読み取った内容を以下列挙しますので、誤りや足りない部分があれば補足をお願いします。

  • cppcheckをインストールするための独自バッチ externals\cppcheck\install-cppcheck.bat を削る
    (バッチファイル大杉の軽減)
  • パッケージ管理ツール chocolatey を導入する
  • cppcheckのインストールを appveyor.yml に直書きすることで依存モジュールをより明確にする
  • chocolatey 導入効果の検証として cppcheck のバージョンを上げる
    (インストーラをブランチに取り込むやり方の場合、対象バージョンを変えるのは大変)
  • ブランチに取り込んでいた cppcheck の msi を削る
    (インストーラは chocolatey が管理してくれるので不要になった)

なんとなく、cppcheckインストールに関するログが増えた気がしてます。
オプションで抑制できるはずなので Release のログは抑制させたい感じです。
Debugはそのままでよいです。って、環境で分けるのが難しければ出しっぱでいいです。

@KageShiron
Copy link
Member Author

  • doxygenもchocolateyでインストールするように
  • ログ削減のためcinstのオプションに--limitoutputを追加(エラー時はちゃんとエラーを吐くはず)

doxygenのインストール処理がないのでappveyorにデフォで入るようになったのかと思い込んでました・・・
rebaseする前にupstreamをfetchし忘れてたかorz

@berryzplus
#653 (comment) のコメントのようにcppcheckとdoxygenを事前にインストール完了しておきたいという考えもあります。

@KageShiron KageShiron changed the title appveyorのcppcheckインストールをchocolateyで行うように appveyorのcppcheckとdoxygenインストールをchocolateyで行うように Dec 15, 2018
@KageShiron
Copy link
Member Author

doxygenのchocolateyパッケージが死んでる・・・
パッケージのメンテナの修正を一旦待ってみます

@berryzplus
Copy link
Contributor

#653 (comment) のコメントのようにcppcheckとdoxygenを事前にインストール完了しておきたいという考えもあります。

なんでcppcheck単独なのか謎に思ってました。

doxygenのchocolateyパッケージが死んでる・・・

インストーラをリポジトリに取り込む選択をした背景には、ダウンロード失敗によるトラブルを回避する意図もあったらしいです・・・

技術的にできるかどうか分かりませんが、本家が落ちている場合の代替取得元を用意しておくことで失敗率を下げることができる気がします。chocolatey もあんまり詳しくはないですが、この辺りも余裕があれば考えておきたいです。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

削り残しみっけたのでコメント入れました。
削除対象のファイル名でgrepしておくと上げる前に安心できる気がします。

appveyor.md Outdated Show resolved Hide resolved
appveyor.md Outdated Show resolved Hide resolved
appveyor.md Outdated Show resolved Hide resolved
@m-tmatma
Copy link
Member

doxygenのchocolateyパッケージが死んでる・・・
パッケージのメンテナの修正を一旦待ってみます

https://help.appveyor.com/discussions/problems/863-doxygen-installed-via-chocolatey-not-found に以下のコメントがあります。

Portable package works:

install:
  - choco install doxygen.portable

これだとどうですか?

@KageShiron
Copy link
Member Author

今回はdoxygen.portableも同じくftp.stack.nlというダウンしてるサーバーからダウンロードしようとするのでだめです。

ローカルにchocolateyのパッケージを置いておくか、mygetなどで独自にホスティングするということも考えられますができれば避けたい・・・
GitHubでは動いてるみたいなので、一度ここにissue立ててメンション飛ばしてみようか・・・

@m-tmatma
Copy link
Member

m-tmatma commented Dec 16, 2018

appveyor に doxygen と cppcheck をプリインストールしてもらうのが、簡単でいいのですが。

@KageShiron
Copy link
Member Author

doxygenの公式サイトが異様に遅かったのですが、キャッシュするようにしたので大丈夫だと思います。c:\choco\に設定するとうまくキャッシュできるのが確認できました。

https://ci.appveyor.com/project/KageShiron/sakura/builds/21214003?fullLog=true#L117

@m-tmatma
Copy link
Member

キャッシュが効いている状態で PR のリビルドかけてみた。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/21224707

@m-tmatma
Copy link
Member

master のビルド → 36分
このPR → 2時間7分

@berryzplus
Copy link
Contributor

なんとなくリビルド欠けてみたんですが・・・ 2回目 2h48m
運悪く遅かった、という雰囲気ではなさそうです。

なんか遅くなる要素あるんでしたっけ?

90分遅延として6環境で割ると、15分ずつ延びてる計算になるのかな。
パッケージたくさん落としてきたって15分もかからないような・・・。

@k-takata
Copy link
Member

ログの時間を見ればよいでしょう。行番号のところにマウスカーソルを合わせると時間が出ます。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/21229840/job/lkbdabei14w3yclo?fullLog=true#L122

Progress: 100% - Completed download of c:\choco\doxygen.install\1.8.14.20181217\doxygen-1.8.14-setup.exe (44.41 MB).
Download of doxygen-1.8.14-setup.exe (44.41 MB) completed.

この2行の間で20分以上掛かっています。キャッシュが効いてなさそうですね。

@KageShiron
Copy link
Member Author

KageShiron commented Dec 27, 2018

リビルドだとかからないんじゃないでしょうか 🤔
量の問題ではなく、異常にdoxygen公式のサーバーが遅いです。(手元で落としても同一)

#702 (comment)
ではキャッシュが聞いてインストーラが走る15秒程度で終わってます。

なのでリビルドではなく追加でコミットによるビルドではキャッシュが効くはずですが、、、
正直これをmasterに入れるのがちょっと怖いので、今回は見送りでも良いかもしれません。

@m-tmatma
Copy link
Member

これ以上 doxygen の公式サーバーの負荷を上げないためにも、この PR は適用しない方がいい気がします。

@k-takata
Copy link
Member

chocoからのインストールは諦めて、SourceForge (https://sourceforge.net/projects/doxygen/files/rel-1.8.14/) からダウンロードしてインストールするとか、AppVeyorにプリインストールをお願いしてみるとか…。
(そもそも毎回doxygenを実行する必要あるの?という気もしますが。)

@ds14050
Copy link
Contributor

ds14050 commented Dec 27, 2018

プルリクエストに対してデフォルトではキャッシュが無効に設定されているようです。PR ブランチは不特定の他人のコードだからとかそういう理由かと想像していますが。

この PR の適用を諦めるのには同意。doxygen の実行を AppVeyor のビルドマトリクスから除きたいのもたしか。

@KageShiron
Copy link
Member Author

KageShiron commented Dec 27, 2018

選択肢としてはざっくりこんなもんでしょうか?

  1. 現状を完全維持
  2. installセクションでインストールするように書き換える
    • doxygenはzip版のほうが早いかも
  3. SourceForgeからダウンロードして展開するようにする
    • その場合、cppcheckはchocolateyからで良さそう
  4. ChocolateyのパッケージメンテナにSource Forgeにダウンロード先を変えてもらうように頼む
  5. doxygen公式にdoxygen.nlが遅いと連絡する
  6. doxygenのCIビルドをやめる

@m-tmatma
Copy link
Member

m-tmatma commented Dec 27, 2018

  1. 別のci で実行する
  2. 実行頻度を下げる

@berryzplus
Copy link
Contributor

Sourceforgeだと解決策にならないかもという懸念…

@KageShiron
Copy link
Member Author

お手軽さのchocolatey 対 ダウンロード時間不要&安定ビルドの現状

というのが一つなので、SourceForgeから落としてきて展開するのはメリットが薄いんですよね。
#514 も含め、必要なら各自でビルドするとでも良いかもしれません。

@KageShiron
Copy link
Member Author

KageShiron commented Dec 31, 2018

doxygen.install 1.8.15 がレビュー待ち状態になっていました。SourceForge.netからのダウンロードに変更されており、手元で試したところ問題のない速度(十数秒程度)になっていました。
https://chocolatey.org/packages/doxygen.install/1.8.15

個人的に他のマージの前提issueになってるので、早めに解決したいのですがどうしましょうか?

  • doxygenの扱いを変更するかどうか
    • 個人でビルドしてもらう or 別CIにする or Releaseのみにするなど、頻度を落とす
  • ファイルを現状のままレポジトリに入れておくか、Chocolatey経由にするか

私としては、doxygen→CIではビルドしないように。cppcheck→chocolatey経由。を提案します。

ただ、私個人はまだC++の方に手をつけられていないので、doxygenを活用されている方の意見を重視したいです。

@m-tmatma
Copy link
Member

個人的に他のマージの前提issueになってるので、

単にインストール方法が変わるだけなので、他の PR には影響はないんじゃないかと思っていますが、
具体的にどの PR に影響しますか?

@KageShiron
Copy link
Member Author

このPR → #653#681 関連 の順にマージにしたいです。
直接的には依存していないのですが、すでに枝が広がっていてこれ以上ごちゃごちゃしたくないという思いが強いです。内容によっては連鎖的に他のPRに修正が及ぶので・・・

@m-tmatma
Copy link
Member

m-tmatma commented Jan 2, 2019

このPR → #653#681 関連 の順にマージにしたいです。

この PR は #653 (comment)#653 (comment)
コメントに対応するものですよね?

インストールを chocolatey 経由でやると、時間がかかる問題が発生したから
PR を適用するまでに時間がかかってしまっているのだと思います。

面倒ですが、ちょっとずつ段階を経って、やればいいんじゃないかと思います。
単純にdoxygen と cppcheck をインストールする処理を install セクションに
移動してやる PR をなげて、マージしたら #653 をマージできると思います。

2つ以上の変更を一つの PR でやろうとすると 1つでも問題があった場合に
変更が全く適用ができなくなるので、後の計画にも支障ができます。
面倒で遠回りに見えるかもしれませんが、結局は速く実現できるということに
なると思います。

chocolatey 経由でやるというPR と install セクションでインストールするというPRを
別々に行うようにすれば、chocolatey で時間がかかる問題の影響をうけなかった
はずなので #653 の対応をもっと早くマージできるのだと思います。

#681 に関してはタイムスタンプは維持してほしいとは思っていますが、
この PR とは別件なので 別途再度議論すればいいと思っています。

@KageShiron
Copy link
Member Author

元々はChocolateyでつまづくのが想定外だったのでPR分けませんでした・・・

現状では遅い問題が解決してるので、以下のように変更しても問題はありません。これで全員がOKだと思うなら、別途わけるまでもなくすぐに適用可能です。

install:
   - cinst cppcheck --cache-location=c:\choco\ --version 1.86 --limitoutput -y
   - cinst doxygen.install --cache-location=c:\choco\ --version 1.8.15--limitoutput -y

一方で、複雑なPRになりそうだとか、もう少し話し合いたいとかであれば以下のような感じでPRを別途出そうと思ってました。

install:
    - externals\cppcheck\install-cppcheck.bat
    - externals\cppcheck\install-doxygen.bat

ということで、皆さんの方針確認だけしたかったのです。


#681 に関してはタイムスタンプは維持してほしいとは思っていますが、この PR とは別件なので 別途再度議論すればいいと思っています。

こちらは、新たな方針で作り直そうとしています。
#681 (comment)

その際にzip.batに手をつける可能性があって、同時並行で作業を進めるとコンフリクトで二度手間三度手間で時間を浪費しそうな予感がしています。ということで、緊急でもないので順次書いていくかと思ってます。

@m-tmatma
Copy link
Member

m-tmatma commented Jan 2, 2019

元々はChocolateyでつまづくのが想定外だったのでPR分けませんでした・・・

問題が発生した時点で、PR をわけたらどうですか? と提案すればよかったのかもしれません。

一方で、複雑なPRになりそうだとか、もう少し話し合いたいとかであれば以下のような感じでPRを別途出そうと思ってました。

Chocolatey に関して現時点で見つかっていない問題が マージ後に見つかった場合とかに
この PR を一時的に取り消す必要がある場合に Chocolatey 対応だけを取り消すことが
できるので、別 PR にしたほうがいいんじゃないかと思います。

また 別 PR にすることで https://github.com/sakura-editor/changelog-sakura に別項目として
出るのでいいんじゃないかと思います。

@ds14050
Copy link
Contributor

ds14050 commented Jan 3, 2019

doxygen.install 1.8.15 がレビュー待ち状態になっていました。SourceForge.netからのダウンロードに変更されており、手元で試したところ問題のない速度(十数秒程度)になっていました。

これは昨日のログです>https://ci.appveyor.com/project/ds14050/sakura-clone/builds/21337460

http://doxygen.nl/files/doxygen-1.8.14-setup.exe からのダウンロードに 23 (分/Job) かかっています。現在はどういう状況なのでしょうか。

追記 バージョンが 1.8.14 と 1.8.15 と違いました。この PR の条件ではまだ遅いということなのでしょう。

キャッシュが効かないとき、ダウンロードサーバーが遅いとき、悪条件が重なった場合とはいえ、合計で3時間以上かかるのはつらすぎます。

一方で、複雑なPRになりそうだとか、もう少し話し合いたいとかであれば以下のような感じでPRを別途出そうと思ってました。

install:
   - externals\cppcheck\install-cppcheck.bat
   - externals\cppcheck\install-doxygen.bat

これは自分も考えました。install-*.bat は AppVeyor でしか仕事をしないのでバッチファイルの中身を appveyor.yml に直書きするつもりでしたが。

@m-tmatma
Copy link
Member

m-tmatma commented Jan 3, 2019

一方で、複雑なPRになりそうだとか、もう少し話し合いたいとかであれば以下のような感じでPRを別途出そうと思ってました。

単純に #742 みたいな PR を作ってマージしとけば #653 の検討をしつつ
この PR の検討をできるんじゃないかと思いました。

#742 だったら、作成するのに 30 分ほどで対応できました。

@m-tmatma
Copy link
Member

m-tmatma commented Jan 3, 2019

一方で、複雑なPRになりそうだとか、もう少し話し合いたいとかであれば以下のような感じでPRを別途出そうと思ってました。

単純に #742 みたいな PR を作ってマージしとけば #653 の検討をしつつ
この PR の検討をできるんじゃないかと思いました。

#742 だったら、作成するのに 30 分ほどで対応できました。

補足しておくと、対応するのに時間がかからないものならば、
特に反対意見や問題が起こりにくそうな方法でさっさと対応しておいて
作業の基盤となる環境を構築した上で反対意見や問題が出るかもしれない作業を
すればいいという意見です。

もちろん、作業にすごく時間がかかるものならば、無駄にならないように
先に議論するのはいいと思います。また実際に作業内容を PR の形で
見せることで受け手にも理解しやしやすくなると思います。

@KageShiron
Copy link
Member Author

#742 ありがとうございます。現在帰省中でWindows環境が仮想マシンしかないのでなかなか作業がやりにくい・・・

@ds14050
1.8.15でダウンロード元がSourceForge.netになって改善しています。今後Chocolatey.orgやSourceForge.netが将来的に不安定になる可能性は当然あるので(GitHubも時々落ちますし)そのリスク評価をどうするかは話し合いかなと思います。

#742 で余分なバッチ等が消えたのでChocolateyを採用する理由はだいぶ薄くなりました。

@KageShiron
Copy link
Member Author

#742 でこちらのPRのメリットが薄れたと思うので、一旦取り下げます。他にメリットなどがあればまた新たに話し合いましょう。

@KageShiron KageShiron closed this Jan 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants