Closed ghost closed 2 years ago
berryzplus
commented
2 years ago 探索部分の3がおかしいような気がしましたが、おそらく仕様通りです。
仮選択したバージョンのMSBuildが存在するか調べる 存在しなかった場合はVS2017のMSBuildを探し、改めてバージョン15を選択する もしVS2017のMSBuildが見つからない場合でも、バージョンは15が選択される
記憶があいまいですが、初期はvs2017同梱の「機能制限のあるvswhere」を使わなければならなかったため、vs2017用の探索ロジックとvs2019用の探索ロジックを分けていたように思います。
こういう感じ 1, vs2017同梱verのvswhereでは正しくパスを取得できないロジックで検索する。
ちゃんと確認してないので間違いあるかもです。
dep5
commented
2 years ago まず現状の動作です
find-tools.batにはARG_VSVERSIONを使って値を渡します。 NUM_VSVERSIONでは値を渡せません。NUM_VSVERSIONに設定しても意味はないです。
find-tools.batは初回ビルド時またはリビルド時に「1回だけ」呼ばれます。
コマンドラインではbuild-sln.bat で、 IDEではcompiletests.run.cmdの場所です 「1回だけ」呼ばれた後のことは考えなくていいということになります。
つぎにfind-tools.batでの処理です。 IDEでビルド時、targetsではNUM_VSVERSIONを渡しています。 また、手軽にはARG_VSVERSIONの設定もできません。 つまりfind-tools.batには何もわたりません。 VSVERSIONが渡らず、 NUM_VSVERSIONの初期値が15になります。 15がインストールされていると、VS2017のツールが設定されます
VSを一つだけインストールしている時バージョンが渡ってないのにエラーが起きないのは find-tools.bat内で改めて自動選択されているからです。 (少しだけ無駄な操作です)
解決するには find-tools.batでNUM_VSVERSIONを受け付けるか targetsでARG_VSVERSIONを渡すか、です。
find-tools.batを改修する場合 新規に受け付けるなら、値のチェックも必要な場合もあるかと思います。 NUM_VSVERSIONとARG_VSVERSIONどちらを優先するかというのも気になります。
targetsを編集する場合
ARG_VSVERSIONには"","clear" , "latest",2017,2019,2022,15,16,17が設定できます
clearは引数として使うのが普通だと思います。
15だからNUM_VSVERSIONを使わなければならないわけではありません。
ARG_VSVERSIONで大丈夫です
targetで環境変数を設定した場合、ローカル変数のような扱いになるようです compiletests.run.cmd が終了したら、環境変数は元通りで、ビルド続行ということです。 ARG_VSVERSIONに設定した値は維持されるということです。（compiletests.run.cmd 実行中以外）
berryzplus
commented
2 years ago 質問回答
dep5
commented
2 years ago 選択されたバージョンをNUM_VSVERSIONに、見つかったMSBuildへのパスをCMD_MSBUILDに設定する`
NUM_VSVERSIONはCMD_MSBUILDを設定するために使われており、ここ以外では使わていないようです。
不思議なのはARG_VSVERSIONを使うことに抵抗を感じる方がいることです。 ARGという名前だから引数として使うべきだ、ということなのでしょうか？
ghost
commented
2 years ago 「vswhereが期待通りにいかないことがあったから万が一に備えてこうしている」というで理解しました。 軽く調べた感じだと https://github.com/microsoft/vswhere/issues/180 とかありました。
ghost
commented
2 years ago NUM_VSVERSIONはCMD_MSBUILDを設定するために使われており、ここ以外では使わていないようです。
Bash上でgrepしました。
$ find . -type d -iname '.vs' -prune -o -type d -iname '.git' -prune -o -type d -iname 'googletest' -prune -o -type f -not -iname '*.md' -print | xargs grep -n -w -i 'NUM_VSVERSION'
./tests/compiletests.run.cmd:8:set /a NUM_VSVERSION_NEXT=NUM_VSVERSION + 1
./tests/compiletests.targets:20: <SetEnv name="NUM_VSVERSION" value="$(VsVersion)" prefix="false" />
./tests/googletest.targets:40: <SetEnv name="NUM_VSVERSION" value="$(VsVersion)" prefix="false" />
./tools/find-tools.bat:18: set NUM_VSVERSION=
./tools/find-tools.bat:70: && set "NUM_VSVERSION=%NUM_VSVERSION%" ^
./tools/find-tools.bat:165: set NUM_VSVERSION=15
./tools/find-tools.bat:167: set NUM_VSVERSION=15
./tools/find-tools.bat:169: set NUM_VSVERSION=16
./tools/find-tools.bat:171: set NUM_VSVERSION=17
./tools/find-tools.bat:175: set NUM_VSVERSION=%ARG_VSVERSION%
./tools/find-tools.bat:183: set NUM_VSVERSION=15
./tools/find-tools.bat:186: if "%NUM_VSVERSION%" == "15" (
./tools/find-tools.bat:188: ) else if "%NUM_VSVERSION%" == "16" (
./tools/find-tools.bat:190: ) else if "%NUM_VSVERSION%" == "17" (
./tools/find-tools.bat:199: set CMAKE_G_PARAM=Visual Studio %NUM_VSVERSION% %VS_PRODUCT_LINE_VERSION%
./tools/find-tools.bat:203: set /a NUM_VSVERSION_NEXT=NUM_VSVERSION + 1
./tools/find-tools.bat:204: for /f "usebackq delims=" %%v in (`"%CMD_VSWHERE%" -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property catalog_productLineVersion -version [%NUM_VSVERSION%^,%NUM_VSVERSION_NEXT%^)`) do (
./tools/find-tools.bat:213: set NUM_VSVERSION=%VSVERSION:~0,2%
./tools/find-tools.bat:217: set /a NUM_VSVERSION_NEXT=NUM_VSVERSION + 1
./tools/find-tools.bat:218: for /f "usebackq delims=" %%d in (`"%CMD_VSWHERE%" -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath -version [%NUM_VSVERSION%^,%NUM_VSVERSION_NEXT%^)`) do (
./tools/find-tools.bat:225: set /a NUM_VSVERSION_NEXT=NUM_VSVERSION + 1
./tools/find-tools.bat:226: for /f "usebackq delims=" %%a in (`"%CMD_VSWHERE%" -requires Microsoft.Component.MSBuild -find MSBuild\**\Bin\MSBuild.exe -version [%NUM_VSVERSION%^,%NUM_VSVERSION_NEXT%^)`) do (
./tools/find-tools.bat:246:for /f "usebackq delims=" %%a in (`"%CMD_VSWHERE%" -property installationPath -version [%NUM_VSVERSION%^,%NUM_VSVERSION_NEXT%^)`) do (compiletests.run.cmdのやつは #1758 で消えるので無視するとして、2か所で使われています。
不思議なのはARG_VSVERSIONを使うことに抵抗を感じる方がいることです。 ARGという名前だから引数として使うべきだ、ということなのでしょうか？
名前から連想できる使い方と実際の使い方が一致しているのは重要なことだと思います。
berryzplus
commented
2 years ago 不思議なのはARG_VSVERSIONを使うことに抵抗を感じる方がいることです。 ARGという名前だから引数として使うべきだ、ということなのでしょうか？
他のバッチ処理はどうなのか分からないですが、 *.targetsから環境変数経由で渡るバージョンは、 起動中のvisual studioのものが渡ってほしいです。
ARG_VSVERSIONを指定した場合、変換が入ります。 指定したのと「同じ意味」の値がNUM_VSVERIONに設定される仕様です。 「同じ意味」ではなく「同じ」が欲しいです。
「環境変素なのにARGなのは不適切じゃなかろうか？」は、それとは別の話だと思っています。
berryzplus
commented
2 years ago あらためてARG_VSVERSIONをgrepしてみたんですが、CIで使ってますね。（誤解解消）
CIのビルド環境にインストールされるvisual studioバージョンは通常1つなので、ymlには要らないように思いました。 https://github.com/actions/virtual-environments/blob/main/images/win/Windows2019-Readme.md
それを踏まえて、現在のバッチ構成を改善する案を思いつきました。
現状はこんな感じです。複数verが入ってる端末でvs2019ビルドしたい場合の例。
set ARG_VSVERSION=2019
build-sln.bat x64 Release※引数じゃないのに引数っぽい名前の環境変数を定義している。 ※setコマンドを知らないと詰みます（いや、それはない・・・ ※2行打たないといけないのが苦痛かも（笑
こういう風に打てるようにしたら分かりやすくなると思います。
build-sln.bat x64 Release 2019第３パラメータを追加して、内部変数 ARG_VSVERSION に代入します。 実際に引数なのであればARGが付いていても文句はないと思います。
修正量がボチボチ大きくなるのが懸念ですが、ちょっと見やすくなるはず。。。
berryzplus
commented
2 years ago 検証のために触ってみた過程で「以下の一文が機能しないことがある」と気付きました。 https://github.com/sakura-editor/sakura/blob/138ec8e53ad4b1d5c8b06d373be9130bc01658b8/tools/find-tools.bat#L70
関連するのは #1167 で、 バッチで出力させるパラメータが多過ぎる が原因なんではないかと思っています。
根治策は「要らんやつを削るぞ～」なんですけど、 基本的に「必要だから追加した」ものばかりなので、 選抜するのは精神的にかなりしんどいだろうと予想しています。
dep5
commented
2 years ago clearについては引数の形のみでしか設定できませんね。 環境変数としては渡せません。
find-tools.batを編集する場合の懸念点です。
NUM_VSVERSIONはget()があり、set()がない ARG_VSVERSIONはどちらもある、とも言えると思います。
NUM_VSVERSIONを受け付けるということは ARG_VSVERSIONと同等の扱いにするということです。
コマンドプロンプトで set NUM_VSVERSION=16 build-sln.bat x64 Release という風にVSVERSIONを指定できるようになるわけです。 set ARG_VSVERSION=latest などとするのも有効なままなので、場合によっては混在する可能性もあるでしょう。
NUM_VSVERSIONで意味のある値は15,16,17...だけですが set NUM_VSVERSION=2022 としてもエラーは起きません。 しかし、find-tools.bat の機能で独自にバージョン選択されるので、複数のVSを入れていないなら問題は起きません。 変換機能のあるARG_VSVERSIONに比べれば機能制限版だと言えます。 バージョンを指定する変数を2つも用意しなければいけない意味って何なんでしょうか？
find-tools.batを編集するなら、これからは NUM_VSVERSIONとARG_VSVERSIONのどちらを使ったか、混在していないか、混在しているならどちらを優先したいのか をコマンドラインを入力する側が考えることになります。
berryzplus
commented
2 years ago ARG_VSVERSION ＝ バージョンを「手入力」するためのもの。 NUM_VSVERSION ＝ 判定されたバージョンを保持しておくためのもの。
https://github.com/sakura-editor/sakura/issues/1759#issuecomment-1008783416 で書いた通り、NUM_VSVERSIONが正しく設定されない、がたまに発生するようです。これの原因は「プログラムの誤り」ではなさそうですが、期待通りに動かないのは不具合として修正すべきと思います。
dep5
commented
2 years ago kazasakuさん すいません。#1755 の延長で書いたので そこでパッチを当てたファイルについて「ここ以外」と書きました。
compiletests.run.cmdのやつは` #1758 で消えるので無視するとして、2か所で使われています。
「NUM_VSVERSIONに設定しても意味はないです。」と書いた通り、
その2か所の
<SetEnv name="NUM_VSVERSION" value="$(VsVersion)" prefix="false" />
この行は意味がなく、何も指定してないのと同じです。
したがって、find-tools.batが独自にNUM_VSVERSIONを決定しています。
これを修正したいというのが #1755 です。
名前から連想できる使い方と実際の使い方が一致しているのは重要なことだと思います。
ARG_VSVERSIONという変数名でなければ、問題ないという意味ですか？
k-takata
commented
2 years ago 検証のために触ってみた過程で「以下の一文が機能しないことがある」と気付きました。
バッチファイルの改行コードはどうなっていますか？ もしLFになっているようであれば #1761 で直ってほしいなと思います。
dep5
commented
2 years ago berryzplusさん
ARG_VSVERSIONを指定した場合、変換が入ります。
find-tools.batの:msbuildを追ってみてください %ARG_VSVERSION%が"","2017","2019"(,"2022")でなければ 「同じ」値が入ります。 この年号との比較が嫌だということですか？
CIのビルド環境にインストールされるvisual studioバージョンは通常1つなので、ymlには要らないように思いました。
一つしか入ってない環境でもARG_VSVERSIONを指定するのは意味があります。 ARG_VSVERSIONを設定しないとまずVS2017を探すからです。 将来別の環境に乗り換えてもARG_VSVERSIONが間違ってるとエラーにはなりません。
ARG_VSVERSION ＝ バージョンを「手入力」するためのもの。 NUM_VSVERSION ＝ 判定されたバージョンを保持しておくためのもの。
それはberryzplusさんの変数名から受ける印象ではないですか？
コードを見てみたところ、 ARG_VSVERSION ＝ バージョンを受け入れるためのもの NUM_VSVERSION ＝ 判定結果の「確認」用 なように思います。
ghost
commented
2 years ago ちょっと考え直してみました。ビルドはこういう過程を経ているように見えます。
ここでtargetsファイルの記述が意図しているのは「実行中のVSと同じもの」ですから、「実行中のVSと同じ意味のもの」では困ります。よって現在の記述が正しいということになるはずです。
逆に言えば、もともと設定されていたNUM_VSVERSIONが書き換えられるのは（値を間違えていない限り）よろしくない気がします。
例えば、VS2019でソリューションをビルドしているときの、テスト（test1.vcxproj）部分のビルドログですが
4>------ ビルド開始: プロジェクト: tests1, 構成: Debug Win32 ------
4>ビルドの開始時の環境:
（～～中略～～）
4>VisualStudioDir = C:\Users\Kohki Akikaze\Documents\Visual Studio 2019
4>VisualStudioEdition = Microsoft Visual Studio Community 2019
4>VisualStudioVersion = 16.0
4>VSAPPIDDIR = C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\
4>VSAPPIDNAME = devenv.exe
4>VSLANG = 1041
4>VSSKUEDITION = Community
4>windir = C:\WINDOWS
4>ZES_ENABLE_SYSMAN = 1
4>find-tools.bat
4>|- CMD_GIT=C:\Program Files\Git\cmd\git.exe
4>|- CMD_7Z=C:\source\apps\7z\7z.exe
4>|- CMD_HHC=C:\Program Files (x86)\HTML Help Workshop\hhc.exe
4>|- CMD_ISCC=
4>|- CMD_CPPCHECK=
4>|- CMD_DOXYGEN=
4>|- CMD_VSWHERE=C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe
4>|- CMD_MSBUILD=C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe
4>|- CMD_CMAKE=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe
4>|- CMD_NINJA=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja\ninja.exe
4>|- CMD_LEPROC=
4>|- CMD_PYTHON=C:\Windows\py.exe
4>|- CMAKE_G_PARAM=Visual Studio 17 2022
4>endNUM_VSVERSIONにはVisualStudioVersionの「16」が入っていたはずなのに、find-tools.batの処理で書き換わり、あたかもVS2022のツールセットを使っているようなログになります。
ghost
commented
2 years ago この年号との比較が嫌だということですか？
16と17の利用可能な二つのバージョンが入っていて、「俺は16を使いたいんだ」とします。 ARG_VSVERSIONだとそこから（年号指定の場合は変換した上で）それは本当に利用できるのか確認する処理が入ります。 …が、16を利用できることが自明ならそんな処理は必要ないでしょう。
一つしか入ってない環境でもARG_VSVERSIONを指定するのは意味があります。 ARG_VSVERSIONを設定しないとまずVS2017を探すからです。 将来別の環境に乗り換えてもARG_VSVERSIONが間違ってるとエラーにはなりません。
今の動作にこだわるならそういう解釈ができなくもないですが、疑問③に対していただいた回答ではこだわる理由はないように見受けられます。 何かの機会にその動作をやめればいいだけ。
ghost
commented
2 years ago https://github.com/sakura-editor/sakura/issues/1759#issuecomment-1009003455 レスを書いたことでようやく気が付きました。 直さなきゃいけないのは「VS2019上でのビルド時にVS2022のツールが選択されてしまうこと」ですね。
ghost
commented
2 years ago githash.targetsでNUM_VSVERSIONが設定されていないため、ログ出力が依然正しくないそうです（ #1806 ）。
よって本issueを再開します。
VS2019にて、githash.targetsから呼ばれたfind-tools.batのログ：
2>---- Make githash.h ----
2>find-tools.bat
2>|- CMD_GIT=C:\Program Files\Git\cmd\git.exe
2>|- CMD_7Z=C:\home\kazasaku\.local\bin\7z.exe
2>|- CMD_HHC=C:\Program Files (x86)\HTML Help Workshop\hhc.exe
2>|- CMD_ISCC=
2>|- CMD_CPPCHECK=C:\Program Files\Cppcheck\cppcheck.exe
2>|- CMD_DOXYGEN=
2>|- CMD_VSWHERE=C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe
2>|- CMD_MSBUILD=C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe
2>|- CMD_CMAKE=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe
2>|- CMD_NINJA=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja\ninja.exe
2>|- CMD_LEPROC=C:\home\kazasaku\.local\bin\LEProc.exe
2>|- CMD_PYTHON=C:\Windows\py.exe
2>|- NUM_VSVERSION=17
2>|- CMAKE_G_PARAM=Visual Studio 17 2022
2>end
前書き
find-tools.batにおけるMSBuildの探索ロジックはおかしくないか、という話題が #1755 で出ました。 issue として話題を分割します。
質問
サクラエディタのビルドでは、処理に必要なツールを探索する find-tools.bat というバッチファイルがあります。 このバッチファイルが行う処理のうち、バージョン決定とMSBuildの探索行う部分は次のような動作です。
NUM_VSVERSIONに、見つかったMSBuildへのパスをCMD_MSBUILDに設定するこの動作について、以下の疑問があります。
NUM_VSVERSIONをあらかじめ設定しても、find-tools.batの動作で上書きされてしまい、意図通りになりません。ARG_VSVERSIONを設定することになっているが、これは本来バッチファイルの引数を保持するものであって、呼び出し時以外に設定するのはおかしいのではないでしょうか。参考資料
Visual Studio Community 2017 workload and component IDs | Microsoft Docs