ビルドツールの検索を改善 #653

KageShiron · 2018-11-28T14:59:01Z

ビルド開始時にfind-tools.batを呼び出し、まとめて検索を行います。

以下のような出力が出ます。

find-tools.bat
├─ CMD_7Z=c:\a\bin\7z.exe
├─ CMD_HHC=C:\Program Files (x86)\HTML Help Workshop\hhc.exe
├─ CMD_ISCC=C:\Program Files (x86)\Inno Setup 5\ISCC.exe
├─ CMD_CPPCHECK=
├─ CMD_DOXYGEN=
└─ CMD_MSBUILD=C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\MSBuild.exe

探索アルゴリズムの改善

文中のコメントの通り、以下のような順で検索します。

CMD_7Zがセットされていれば利用する
パスが通っていればそれを使う
デフォルトのインストールパスで見つかればそれを使う
1～3で見つからなければCMD_7Z~~を削除(未定義にする)~~には何もセットしない

ただし、msbuildのみvswhereを使います。Enterpriseで正しく発見できることを確認しました。

その他

勘違いしていなければ、set HOGE=すると、環境変数HOGEは削除されます。if "%CMD_HHC%" == ""はif not defined CMD_HHCと同義で、こちらのほうが説明的です。

berryzplus · 2018-11-28T15:22:07Z

PRありがとうございます。

ただし、msbuildのみvswhereを使います。Enterpriseで正しく発見できることを確認しました。

vswhere ってデフォルトで入るものなんですね。
batで書かれたパスを見て見たら、入れた覚えのないvswhere.exeを見付けて驚きました。

berryzplus · 2018-11-28T15:28:28Z

内容については、これでappveyorが通るなら問題ないんじゃないかと考えています。

CIの完了を待ってから判断のつもりで、reviewコメント入れるのは早くて明日朝となる見込みです。
ちょっと変わった部分もあるので、できれば @m_tmatma さんの判断をあおいだほうがよいのかなと思ってます。

KageShiron · 2018-11-28T15:35:16Z

趣味が出る分野だと思うので、ご意見があればお願いします。

https://github.com/Microsoft/vswhere

vswhere is included with the installer as of Visual Studio 2017 version 15.2 and later, and can be found at the following location: %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe.

とのことなので、当面はこのパスに含まれる・・・と思います。（微妙にあてにならないけど)

tools/hhc/readme.md

find-tools.bat

ds14050 · 2018-11-28T16:03:48Z

従来との相違点はこんな感じでしょうか。

ユーザー定義の環境変数があればそれを使う。
msbuild の探索方法
見つからなかったコマンドがあらゆる機会に探索される。
tools/zip/*.bat の実行に前提条件が付いた。

思うところです。

tools/zip/*.bat の前提条件は以前の通りになくしてほしい。
バッチの tips は Wiki に残せないか。
find-tools.bat が引数を取って探索対象のプログラムを絞ってもいいのではないか。

明確な理由があるのでなければ１番目は特に対応してほしいです。

２番目は言い出しっぺがやれ案件です。

３番目は効率を気にするならやった方がいいかなという程度です。CMD_* という環境変数の * 部分をバッチの引数とすれば呼び出し元とのコンセンサスは簡単にとれます。さらにそれがバッチ内のラベル名と一致するなら処理の振り分けも簡単です。

m-tmatma · 2018-11-28T21:52:42Z

find-tools.bat

+:msbuild
+:: ref https://github.com/Microsoft/vswhere
+setlocal
+PATH=%PATH%;%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\;


%ProgramFiles% と %ProgramW6432% に対する指定が抜けています。

↑ 32bit OS で動かすときに必要です。

意図的に64bitのみを想定してます、ならそれはそれで良い気もします。

他の箇所は 32bit でも対応出来るようにしているのにここだけ対応しないのは、中途半端な対応ですし、変更前は対応していたのに、この対応で削る理由はないと思います

+1 の意味を補足しときます。

この対応で削る理由はない、についての「だよねー」の意味です。

vswhereのドキュメントに%ProgramFiles(x86)%とのみあったので、32bitOSは考慮漏れでした

m-tmatma · 2018-11-28T21:54:57Z

find-tools.bat

+PATH=%PATH%;%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\;
+for /f "usebackq tokens=1* delims=: " %%i in (`vswhere -latest -requires Microsoft.Component.MSBuild`) do (
+    if /i "%%i"=="installationPath" (
+        endlocal && set "CMD_MSBUILD=%%j\MSBuild\15.0\Bin\MSBuild.exe"


%%j はどこで定義されるのですか?
appveyor のログを見る限りはちゃんと動いているようですが。

これは確かforコマンドの仕様。複数トークンを処理する場合には、指定した文字から連番で変数が割当たります。iを指定してるので二個目ですね。

これは確かforコマンドの仕様。複数トークンを処理する場合には、指定した文字から連番で変数が割当たります。iを指定してるので二個目ですね。

なるほど、知りませんでした。
for /? に載ってますね。

ところで、vswhere msbuild.exe で検索したらこんなサイトがありました。
https://github.com/Microsoft/vswhere/wiki/Find-MSBuild

-property installationPath をつけて実行するともっと直接的に検索できるようです。

こちらのほうが良さそうなので、変更しました

m-tmatma · 2018-11-28T22:28:44Z

find-tools.bat で全部のツールを検索する場合、cppcheck や doxygen のインストールの順番に
よって最初に見つからない状態が存在しているのがわかりにくいと思うので、
一番最初に cppcheck や doxygen をインストールするようにするのがいいと思います。
(現状では build-all.bat でインストールしています)

install セクションというが appveyor.yml にあるのでそこでやるのがわかりやすいと思います。
https://www.appveyor.com/docs/build-configuration/#installing-additional-software

tools/zip/listzip.bat

m-tmatma · 2018-11-28T22:36:19Z

ツールに関するドキュメントを削っていますが、
かわりに find-tools.bat の説明をする find-tools.md が必要だと思います。

find-tools.bat でも tools/hhc/readme.md で解説していた tips を使っているので
find-tools.md に説明を更新した上で、tips を説明すればいいと思います。

m-tmatma · 2018-11-28T22:40:31Z

find-tools.bat

@@ -0,0 +1,89 @@
+:: ビルドツールのパスを見つける


文字コードが UTF-8 で保存しているのでローカル環境で実行すると
以下のように文字化けしてしまいます。文字化けしないように(バッチファイル内で)コンソールの文字コードを適切に設定するか、あるいは英語で記述するようにしてローカル環境と appveyor 上の両方で問題ないようにしてほしいです。

G:\gitwork\SakuraEditor\PR\sakura-PR653>find-tools.bat G:\gitwork\SakuraEditor\PR\sakura-PR653>縺､縺九ｌ縺ｰ縺昴ｌ繧剃ｽｿ縺・ '縺､縺九ｌ縺ｰ縺昴ｌ繧剃ｽｿ縺・' は、内部コマンドまたは外部コマンド、操作可能なプログラムまたはバッチファイルとして認識されていません。 G:\gitwork\SakuraEditor\PR\sakura-PR653>°繧峨↑縺代ｌ縺ｰCMD_7Z繧貞炎髯､縺吶ｋ '°繧峨↑縺代ｌ縺ｰCMD_7Z繧貞炎髯､縺吶ｋ' は、内部コマンドまたは外部コマンド、操作可能なプログラムまたはバッチファイルとして認識されていません。 find-tools.bat 笏懌楳 CMD_7Z=C:\Program Files\7-Zip\7z.exe 笏懌楳 CMD_HHC=C:\Program Files (x86)\HTML Help Workshop\hhc.exe 笏懌楳 CMD_ISCC= 笏懌楳 CMD_CPPCHECK=C:\Program Files\doxygen\bin\doxygen.exe 笏懌楳 CMD_DOXYGEN= 笏披楳 CMD_MSBUILD=C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\MSBuild.exe

とりあえず、Shift JISにしてみました。ダメそうなら英語のみにします。

chcp 65001がデフォの私の端末では逆に罫線が文字化けしますが、対処法がわからぬ

とりあえず、Shift JISにしてみました。ダメそうなら英語のみにします。

他のバッチファイルはややこしいことをしたくないので、全部英語にしています。

tree コマンドにこういうオプションがあります。

/A 拡張文字ではなく、ASCII 文字で表示します。

＋と＼(だが￥だ)と｜を使うみたいです。>罫線

とりあえず、Shift JISにしてみました。ダメそうなら英語のみにします。

とりあえず英語にしましょう。日本語対応は後で考えましょう。

m-tmatma · 2018-11-28T22:43:26Z

find-tools.bat

+
+:7z
+setlocal
+PATH=%PATH%;%ProgramFiles%\7-Zip\;%ProgramFiles(x86)%\7-Zip\;%ProgramW6432%\7-Zip\;


7-Zip の部分が繰り返し使われるので、変数で定義して共通化したい気がします。
HTML Workshop 等他のツールも同様です。

"7-zip" に 7zip という名前を付けることに意味はありません。

ちょっと逸れますが、パスはそれ自体が ID です。相対・絶対など複数の表記法はありますが実体としてパスは ID です。パスの一部に変数名という形で別名を付けるとアイデンティティが希釈されます。長いパス全体に短い別名を付けることに意味がないとは言いませんが、a\b\c\f.x に A\B\C\f.x という別名を付けたり AB\C\F という別名を付けることは難読化と変わりがありません。

つまりはこういうことです。

set Z7=7-zip と定義したが今度は Z7 という変数名を複数の場所で利用することになるので、この名前を繰り返さないで済むように set Z7VAR=Z7 と定義して Z7 を利用する場所では Z7VAR という名前を書くようにしよう。

m-tmatma さんはこういうことをしています。

ちなみに少し前に雑談場に貼ったリンク先で知りましたが、こういう多重展開テクニックがあるようです。※Z7VAR の中身は 7-zip ではなく Z7 という変数名であることに注目。

>set Z7=7-zip >set Z7VAR=Z7 >call echo %%%Z7VAR%%% 7-zip

コメントを勘違いされているようなので、コードで示すとこんな感じです。

:7z setlocal set APPDIR=7-Zip PATH=%PATH%;%ProgramFiles%\%APPDIR%\;%ProgramFiles(x86)%\%APPDIR%\;%ProgramW6432%\%APPDIR%\;

APPDIR の変数名は何でもいいです。 (setlocal の後なので変数はローカル化される)

このようにすることで HTML Help Workshop の場合や Inno Setup 5 の場合も
PATH の代入の行を共通化でき、かつパス名部分の重複を回避して共通化することができます。

たしかにそこまでは読んでいませんでした。「別名を付ける」ではなく「~~コードを共通化する~~共通のコードを利用する~~ひとつのコードを共有する」ことが目的であるなら意義を認めます。

この際書いてしまいますが

m-tmatma さんはこういうことをしています。(現在完了形)

が指し示しているのは https://github.com/sakura-editor/sakura/pull/378/files における project_name 変数のことです。

CMakeLists.txt においてプロジェクト名やターゲットは識別子です。パスとは違い十分に短い識別子です。これを変数をつかって匿名化することは何にも寄与しません。

本人がそうすることを批判はしませんが、他人に要求するなら対等な立場から反論を述べますし、自分が干渉されたなら全力で抵抗します。

何を指すのか掴めませんでした。

set Z7=7-zip と定義したが今度は Z7 という変数名を複数の場所で利用することになるので、この名前を繰り返さないで済むように set Z7VAR=Z7 と定義して Z7 を利用する場所では Z7VAR という名前を書くようにしよう。 m-tmatma さんはこういうことをしています。

です。事実と異なります。

その行為のナンセンスさを示すためのコメントが #653 (comment) です。7-Zip を繰り返すことと APPDIR を繰り返すことに違いはありません。

7-Zip の場合は短いですが、
例えば、Inno Setup 5\ISCC.exe のパスの場合は、Inno Setup 5 の部分は
例えば Inno Setup がバージョン6 になったら多分 Inno Setup 6 になるだろうし
アプリケーションのフォルダパスは 64bit 版でも 32bit 版でも変わらないだろうから
共通部分を共通化するという意図です。

CMakeLists.txt においてプロジェクト名やターゲットは識別子です。パスとは違い十分に短い識別子です。これを変数をつかって匿名化することは何にも寄与しません。

この記述は先頭で変数として project_name を定義しています。

それで何度も同じ記述を繰り返したくないから、${project_name} という
変数で参照するようにしています。

途中でプロジェクト名を変えたくなった場合とかに一箇所だけ変えれば
いいようにするものです。

あと、#378 に関して言いたいことがあれば、ここに書かずに、#378 に書いて
ここには #378 にコメント書きました、ということだけ書くのがいいかと思います。

この記述は先頭で変数として project_name を定義しています。

それで何度も同じ記述を繰り返したくないから、${project_name} という
変数で参照するようにしています。

途中でプロジェクト名を変えたくなった場合とかに一箇所だけ変えれば
いいようにするものです。

この理屈はわかります。すべての問題は余分の間接層で解決するそうですから。100 の間接層を導入すれば 100 の問題が解決するだろうという揶揄やり過ぎへの戒めが #653 (comment) に込められています。「途中でプロジェクト名を変えたくなった場合」は現実の問題ではありません。「プロジェクト名を変えたくなったが exe 名を変えたくない場合」には対応できません。ひとつの問題も解決する前にべつのやっかいごとを持ち込んでいます。

あと、#378 に関して言いたいことがあれば、ここに書かずに、#378 に書いて
ここには #378 にコメント書きました、ということだけ書くのがいいかと思います。

本人がそうすることは批判しないと書きました。#378 について他人のやり方をとやかく言うつもりはありません。しかしここでの 7-Zip に APPDIR という別名を付けてはどうかという他人への提案に対しては、反対意見もあるということを言わずにはいられません。

5e63ca1#r237291175

このコメント、改めて見てみると言ってることは分かる気がしました。
純粋に「それ今やるべき？」というツッコミがないでもないですが、複数のツールを対象に似たような判定を作っていくうえで、コピペできる判定のセットを作っておくと増やすのに便利だと思います。

記述内容的に「感想」に近いコメントなので
やるかどうかはPRを出している人の判断に委ねられると思ってます。

「要望」なのか「感想」なのかはなるべく分かるようにしていけるといいなと思います。
（と、コメントの大半が「レビューとは関係ありません」なぼくが言うとアレですが 😄）

m-tmatma · 2018-11-28T22:50:19Z

他の find-xxx.bat は削除したのに find-doxygen.bat が残ってるのは消し忘れ?

m-tmatma · 2018-11-28T22:54:48Z

find-tools.bat で全部のツールを検索する場合、cppcheck や doxygen のインストールの順番に
よって最初に見つからない状態が存在しているのがわかりにくいと思うので、
一番最初に cppcheck や doxygen をインストールするようにするのがいいと思います。
(現状では build-all.bat でインストールしています)

install セクションというが appveyor.yml にあるのでそこでやるのがわかりやすいと思います。
https://www.appveyor.com/docs/build-configuration/#installing-additional-software

これだけで単体で PR を投げるのでもいいと思います。
変更内容がすごく少なくなるのでわかりやすいかもしれません。
(別 PR にする場合、この PR より先にマージすることが前提です)
(今の構成がわかりにくいということですが。)

berryzplus

ビルドは通ったけどままならん感じですね。
ぼくも気になったコメントに困ったちゃんマークを付けました。

基本的にok、何点か修正要となった認識です。

m-tmatma · 2018-11-29T03:40:12Z

あと、以前のバッチファイルに関して、markdown で説明していた箇所も合わせて更新してほしいです。

- FORCE_POWERSHELL_ZIPオプションを廃止し、該当ドキュメントを除去 - find-tools.batを1度しか呼び出さないように修正 - zip関連バッチでfind-toolsを呼ぶように - find-doxygenの消し忘れ - find-tools.batをShift JISに

KageShiron · 2018-11-29T15:31:52Z

すぐに修正できそうなところだけ一旦修正しました。

初回のみ呼び出すように

find-tools.bat が引数を取って探索対象のプログラムを絞ってもいいのではないか。

考えましたが、バッチが複雑になりそうな方法しか思いつかなかったので、FIND_TOOLS_CALLEDが未定義の場合のみ検索するようにしてみました。set FIND_TOOLS_CALLED=で再度実行できます。

cppcheckとdoxygen

#653 (comment)
cppcheckとdoxygenはchocolateyで導入するほうが良さそうです。後ほど別のPRで出します。

ドキュメント

バッチファイルの説明やtipsなんかはWikiやdocsディレクトリとかに移してもいい気がします。細々したドキュメントがあると、FORCE_POWERSHELL_ZIPのように調べ忘れがでそうです。

m-tmatma · 2018-11-30T13:09:38Z

バッチファイルの説明やtipsなんかはWikiやdocsディレクトリとかに移してもいい気がします。細々したドキュメントがあると、FORCE_POWERSHELL_ZIPのように調べ忘れがでそうです。

逆だと考えています。

markdown でリポジトリに入れることでレビュープロセスを通るので、
更新漏れがあった場合にレビューアーの誰かが気づく(はずな) ので
更新漏れがあっても修正できます。

wiki に書くとレビュープロセスを通らないので更新されないまま見過ごされて
最新の状況を反映してないドキュメントになる可能性が高いです。

FORCE_POWERSHELL_ZIP が wiki に書いてあったら、更新できたと思いますか?

もし wiki に FORCE_POWERSHELL_ZIP に関して記載する場合、
コードから FORCE_POWERSHELL_ZIP の処理がコードから削除されたときに
wiki には FORCE_POWERSHELL_ZIP の設定が導入される前のバージョン、有効なバージョン、
削除されたバージョンに関して説明を行う必要があり、逆に手間が増えませんか?

k-takata · 2018-11-30T23:23:31Z

find-tools.bat

+setlocal
+PATH=%PATH%;%ProgramFiles%\7-Zip\;%ProgramFiles(x86)%\7-Zip\;%ProgramW6432%\7-Zip\;
+:: where�̏o�͂�1�s�ڂ�CMD_7Z�ɑ��
+for /f "usebackq delims=" %%a in (`where 7z`) do ( 


whereはwhere $ENV:fileと書くことで、PATHの代わりにENV環境変数から検索ができます。
PATHを汚さないためだけにsetlocalを使う必要はありません。

たぶん where $ENV:file と書く方がカッコいいと思います。
直すの大変そうなので、別PRで後日対応でもいい気はします。

よく考えると PATH という特別な変数に変更を加える必要がなくなるとはいえ、現在の実行環境に新たな変数を定義することになるわけで、それなら setlocal は常に付けてスコープを最小限にするのがお行儀のいいスタイルではないかと思います。

berryzplus

ログに問題っぽいものを見付けたのでコメント入れました。
対応可能なら対応してから入れたいです。

対応難しいようならそのまま入れていい気もしますが、まずはご確認ください。

build-chm.bat

find-tools.bat

berryzplus · 2018-12-01T15:07:57Z

find-tools.bat

+setlocal
+PATH=%PATH%;%ProgramFiles%\7-Zip\;%ProgramFiles(x86)%\7-Zip\;%ProgramW6432%\7-Zip\;
+:: where�̏o�͂�1�s�ڂ�CMD_7Z�ɑ��
+for /f "usebackq delims=" %%a in (`where 7z`) do ( 


たぶん where $ENV:file と書く方がカッコいいと思います。
直すの大変そうなので、別PRで後日対応でもいい気はします。

berryzplus

失礼。commentで残すつもりがapproveになっていたので取消。

KageShiron · 2018-12-02T04:34:23Z

#653 (comment)

Wikiでのバージョン管理は確かにそうですね・・・

docsディレクトリに移すのはどうでしょうか？

(個人的には)ファイルが同じディレクトリにたくさんあると探しにくい
画像やサンプルファイルなどをプログラムと混ざらずに管理できる

同じファイル名で説明文があるのもそれはそれで見やすいので、難しいところかもしれません。(それを含めてファイル先頭に説明を書いて見たものの、バッチファイルの文字コードで苦しむリスクの方がたかそうなので削除するつもりです)

m-tmatma · 2018-12-02T06:29:07Z

#653 (comment)

Wikiでのバージョン管理は確かにそうですね・・・

docsディレクトリに移すのはどうでしょうか？

(個人的には)ファイルが同じディレクトリにたくさんあると探しにくい

画像やサンプルファイルなどをプログラムと混ざらずに管理できる

同じファイル名で説明文があるのもそれはそれで見やすいので、難しいところかもしれません。(それを含めてファイル先頭に説明を書いて見たものの、バッチファイルの文字コードで苦しむリスクの方がたかそうなので削除するつもりです)

いっぱいファイルがあると見にくいということなら
これまでのように、バッチファイルも含めて、tools ディレクトリ、あるいはそのサブフォルダに
移してはいかがでしょうか？

そうすれば、ファイルが増えても、一つのフォルダあたりのファイル数を抑えられますし
バッチファイルと同じフォルダに同名のドキュメントがある状態を維持できます。

バッチファイルと同名でドキュメントがあるとドキュメントを最新にする努力をしやすいように思います。
別のディレクトリにあるとドキュメントが存在すること自体気づかないかもしれない。

m-tmatma · 2019-01-09T12:23:49Z

tools/zip/listzip.bat

-	@echo -------------------------------------------------------
-	@echo ---- you can make this faster by installing 7-zip. ----
-	@echo -------------------------------------------------------
+echo examining %SRCZIP%.


zip 系のバッチファイルで、ここだけ echo の前の @ マークが消えてますが、
間違って削除しました?

多分ミスです。ところで、@echo off以降は@をつけても特に意味はないと認識してるんですが、この@は何か意図があるものですか？

もともと別のバッチを call で呼んでいるので
バッチファイルの中の処理に依存しないように
するためだったと思います。

パスの検索用のバッチファイルって削除するとう結論でした？

m-tmatma · 2019-01-09T12:28:54Z

tools/find-tools.md

+デフォルトパスにインストールしていない場合や別の実行ファイルを使いたい場合、実行前にパスを通しておくかCMD_7Zなどの対応する環境変数に絶対パスをセットしておいてください。
+
+## 外部ツールの一覧
+|      ツール名      |   環境変数   | ソフトのデフォルトパス |  ファイル名  |


この説明では ソフトのデフォルトパス が何を意味するのかわからないと思います。
具体例を書けばいいと思います。

あと、具体例を書くのであれば、doxygen を使ったほうがいいと思います。
理由は、ソフト名とアプリのインストールパスが異なるので説明がわかりやすくなると思います。

m-tmatma · 2019-01-09T12:30:18Z

tools/find-tools.md

+事前に環境変数の`FORCE_POWERSHELL_ZIP`を1にセットすることで、7zの検索をスキップできます。
+7z未インストールの環境で[PowerShellによるzipの処理](zip/readme.md)が正しく行われるかを検証する際に活用できます。
+
+### 参照


この参照は msbuild に対するものなので、MSBuild の章の下にぶら下げたほうがいいです。

m-tmatma · 2019-01-09T12:32:43Z

tools/find-tools.md

+4. 1～3で見つからなければCMD_MSBUILDには何もセットしない
+
+## zipの処理に7zではなくPowerShellを強制する
+事前に環境変数の`FORCE_POWERSHELL_ZIP`を1にセットすることで、7zの検索をスキップできます。


FORCE_POWERSHELL_ZIP に対して build.md に対するリンクを張ったほうがいいと思います。

m-tmatma · 2019-01-09T12:33:19Z

tools/find-tools.md

+| Inno Setup 5       | CMD_ISCC     | Inno Setup 5           | ISCC.exe     |
+| cppcheck           | CMD_CPPCHECK | cppcheck               | cppcheck.exe |
+| doxygen            | CMD_DOXYGEN  | doxygen\bin            | doxygen.exe  |
+| MSBuild            | CMD_MSBUILD  | 特殊                   |              |


msbuild のファイル名は空にする必要はないと思います。

m-tmatma · 2019-01-09T12:34:23Z

tools/find-tools.md

+
+1. CMD_7Zがセットされていればそれを使う
+2. パスが通っていればそれを使う
+3. 以下のディレクトリ内の「ソフトのデフォルトパス」で見つかればそれを使う


以下のディレクトリのサブディレクトリを順番で探すという説明があったほうかいいと思います。

元々の停止理由が解消されたので。

m-tmatma · 2019-01-09T21:02:53Z

あと気になったのは find のバッチファイルって結構時間がかかりましたが、もともとこんなに時間かかりました？7秒ぐらいかかったと思います

KageShiron · 2019-01-13T11:45:38Z

レビュー受けた箇所直しました。

@KageShiron KageShiron dismissed ds14050’s stale review via 5efac82 8 minutes ago

あれ、このメッセージなんだろう・・・、なんか変な操作をしてしまってたらすいません

あと気になったのは find のバッチファイルって結構時間がかかりましたが、もともとこんなに時間かかりました？7秒ぐらいかかったと思います

AppVeyorのログを見る感じ、find-tools.batは2秒前後で終わってるっぽいです。タイミング問題とかはあるかもしれませんが、2回以上は無駄処理しないはずなので時間はそれほどじゃまにならないはず

m-tmatma · 2019-01-13T11:55:35Z

tools/find-tools.md

+
+## zipの処理に7zではなくPowerShellを強制する
+事前に環境変数の`FORCE_POWERSHELL_ZIP`を1にセットすることで、7zの検索をスキップできます。
+7z未インストールの環境で[PowerShellによるzipの処理](zip/readme.md)が正しく行われるかを検証する際に活用できます。[build.md](../build.md#powershell-によるzipファイルの圧縮解凍内容確認の強制)も参照してください。


7z未インストールの環境で試すためではなく、7z インストール済みの環境で
powershell での zip 処理を行うためです。

「7z未インストールの環境をエミュレートする」的な意味合いで書いてましたが、そうとも読み取れますね。紛らわしくないように訂正しました。

m-tmatma · 2019-01-13T11:57:16Z

externals.bat は使用されていないバッチファイルですが、
別の PR に入れるはずのものが混じってますか?

m-tmatma · 2019-01-13T11:58:30Z

AppVeyorのログを見る感じ、find-tools.batは2秒前後で終わってるっぽいです。タイミング問題とかはあるかもしれませんが、2回以上は無駄処理しないはずなので時間はそれほどじゃまにならないはず

再度ローカルで実行したら 1 秒ほどになりました。

ds14050 · 2019-01-13T12:00:39Z

@KageShiron KageShiron dismissed ds14050’s stale review via 5efac82 8 minutes ago

新しいコミットが approving review を無効にしたってことだと思います。無効になるようにリポジトリが設定されています。

m-tmatma · 2019-01-13T12:03:32Z

新しいコミットが approving review を無効にしたってことだと思います。無効になるようにリポジトリが設定されています。

そうです。
"Dismiss stale pull request approvals when new commits are pushed" の設定です。
https://dev.classmethod.jp/tool/github/protect-branch/

KageShiron · 2019-01-13T14:24:29Z

approving previewはそういうことなんですね。変なことしてなくてよかった。

externals.batは除いたはずなのに、誤操作っぽいです・・・

m-tmatma · 2019-01-14T01:00:07Z

細かい指摘ですが、

tools/zip/unzip.bat では
if not defined CMD_7Z (
としていますが、

tools/zip/listzip.bat では
if "%CMD_7Z%" == "" (
としているでどちらかに合わせたいです。

berryzplus · 2019-01-14T04:43:03Z

細かい指摘ですが、

tools/zip/unzip.batでは
if not defined CMD_7Z (
としていますが、

tools/zip/listzip.batでは
if "%CMD_7Z%" == "" (
としているでどちらかに合わせたいです。

今後の課題でいいのかな？・・・

なんとなくnot defined ~のがカッコいい気がします 😄

m-tmatma · 2019-01-14T06:35:39Z

該当の箇所をこの PR で変更しているので、
修正しておきたいです。

KageShiron · 2019-01-14T07:38:45Z

修正しました。
意味論としてif not definedのほうがわかりやすいと思ってます

m-tmatma · 2019-01-14T08:59:32Z

対応ありがとうございます。

KageShiron · 2019-01-14T15:56:42Z

(一応確認)一応2名からレビューもらったということで、マージして大丈夫でしょうか？
1日ほど待って特に意見がなければマージします。

ビルドツールの検索を改善

Fix: Merge find-*.bat to find-tools.bat

70a0870

ds14050 reviewed Nov 28, 2018

View reviewed changes

tools/hhc/readme.md Show resolved Hide resolved

ds14050 reviewed Nov 28, 2018

View reviewed changes

find-tools.bat Outdated Show resolved Hide resolved

m-tmatma reviewed Nov 28, 2018

View reviewed changes

tools/zip/listzip.bat Show resolved Hide resolved

m-tmatma reviewed Nov 28, 2018

View reviewed changes

berryzplus requested changes Nov 29, 2018

View reviewed changes

KageShiron added 3 commits November 29, 2018 23:38

FIX: CMD_CPPCHECK -> CMD_DOXYGEN

e59c67d

Fix: find msbuild bug

d584564

k-takata reviewed Nov 30, 2018

View reviewed changes

ds14050 mentioned this pull request Nov 30, 2018

☕ PRで盛り上がった時のコーヒーブランチ ☕ sakura-editor/management-forum#51

Open

berryzplus approved these changes Dec 1, 2018

View reviewed changes

berryzplus previously requested changes Dec 1, 2018

View reviewed changes

m-tmatma mentioned this pull request Dec 2, 2018

SonarQube (SonarCloud) を調べる #510

Closed

m-tmatma reviewed Jan 9, 2019

View reviewed changes

ドキュメント修正

5efac82

KageShiron dismissed ds14050’s stale review via 5efac82 January 13, 2019 11:34

m-tmatma reviewed Jan 13, 2019

View reviewed changes

ファイル誤追加と紛らわしい文面を訂正

9bbda9b

definedのスタイル

fd205ca

m-tmatma approved these changes Jan 14, 2019

View reviewed changes

KageShiron merged commit 4c982ea into sakura-editor:master Jan 16, 2019

KageShiron deleted the fix/find-batch branch January 16, 2019 08:03

m-tmatma added this to the next release milestone Feb 3, 2019

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#653 from KageShiron/fix/find-batch

b971b2e

ビルドツールの検索を改善

		@@ -0,0 +1,89 @@
		:: ビルドツールのパスを見つける

ビルドツールの検索を改善 #653

ビルドツールの検索を改善 #653

Conversation

KageShiron commented Nov 28, 2018 • edited Loading

探索アルゴリズムの改善

その他

berryzplus commented Nov 28, 2018

berryzplus commented Nov 28, 2018

KageShiron commented Nov 28, 2018

ds14050 commented Nov 28, 2018

m-tmatma Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Nov 28, 2018 • edited Loading

m-tmatma commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

ds14050 Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Nov 28, 2018

m-tmatma commented Nov 28, 2018 • edited Loading

berryzplus left a comment

Choose a reason for hiding this comment

m-tmatma commented Nov 29, 2018

KageShiron commented Nov 29, 2018

初回のみ呼び出すように

cppcheckとdoxygen

ドキュメント

m-tmatma commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

KageShiron commented Dec 2, 2018

m-tmatma commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Jan 9, 2019

KageShiron commented Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Jan 13, 2019

m-tmatma commented Jan 13, 2019

ds14050 commented Jan 13, 2019

m-tmatma commented Jan 13, 2019

KageShiron commented Jan 13, 2019 • edited Loading

KageShiron commented Nov 28, 2018 •

edited

Loading

m-tmatma Nov 28, 2018 •

edited

Loading

m-tmatma commented Nov 28, 2018 •

edited

Loading

ds14050 Nov 30, 2018 •

edited

Loading

ds14050 Nov 29, 2018 •

edited

Loading

ds14050 Nov 29, 2018 •

edited

Loading

ds14050 Nov 30, 2018 •

edited

Loading

ds14050 Nov 30, 2018 •

edited

Loading

m-tmatma commented Nov 28, 2018 •

edited

Loading

KageShiron commented Jan 13, 2019 •

edited

Loading

KageShiron commented Jan 13, 2019 •

edited

Loading