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

find-tools.batにおけるMSBuildの探索動作を再検討する #1759

Closed
ghost opened this issue Jan 9, 2022 · 18 comments · Fixed by #1785 or #1806
Closed

find-tools.batにおけるMSBuildの探索動作を再検討する #1759

ghost opened this issue Jan 9, 2022 · 18 comments · Fixed by #1785 or #1806
Labels
environment 環境関連全般【ChangeLog除外】

Comments

@ghost
Copy link

ghost commented Jan 9, 2022

前書き

find-tools.batにおけるMSBuildの探索ロジックはおかしくないか、という話題が #1755 で出ました。
issue として話題を分割します。

質問

サクラエディタのビルドでは、処理に必要なツールを探索する find-tools.bat というバッチファイルがあります。
このバッチファイルが行う処理のうち、バージョン決定とMSBuildの探索行う部分は次のような動作です。

  1. バッチファイルに与えられた引数をチェック
    • 製品名(例:2017)が指定されている場合、対応するバージョン番号を使用するバージョンとして仮選択する
    • バージョン番号(例:15)が指定されている場合、それを使用するバージョンとして仮選択する
    • 「latest」が指定されている場合、インストールされている最新のビルドツールを探し、そのメジャーバージョンを仮選択する
    • それ以外の場合、バージョン15(VS2017)を仮選択する
  2. 仮選択したバージョンが正しいか確認する
    • 仮選択したバージョンのビルドツールを探し、もし見つからなかった場合は改めてインストールされている最新のビルドツールを探し、それのメジャーバージョンを仮選択する
  3. 仮選択したバージョンのMSBuildが存在するか調べる
    • 存在しなかった場合はVS2017のMSBuildを探し、改めてバージョン15を選択する
      • もしVS2017のMSBuildが見つからない場合でも、バージョンは15が選択される
  4. 選択されたバージョンをNUM_VSVERSIONに、見つかったMSBuildへのパスをCMD_MSBUILDに設定する

この動作について、以下の疑問があります。

  1. 指定したバージョンのビルドツールがインストールされていない場合、利用できる最新のビルドツールを探してバージョンを再選択しています。MSBuildはVisual Studioの必須コンポーネントですから、その後のMSBuildの探索でビルドツールと同じバージョンのものが見つからないということは起こりえるのでしょうか?
  2. 複数のVisual Studioがインストールされている環境で、あらかじめ利用するバージョンを固定しておきたいニーズが存在します。このために環境変数NUM_VSVERSIONをあらかじめ設定しても、find-tools.batの動作で上書きされてしまい、意図通りになりません。
    • ドキュメント上ではこの目的にはARG_VSVERSIONを設定することになっているが、これは本来バッチファイルの引数を保持するものであって、呼び出し時以外に設定するのはおかしいのではないでしょうか。
  3. バージョンを指定せずに呼び出したときに無条件で15(VS2017)を選択していたり、VS2017のMSBuildを最後の選択肢にしたりしているのはなぜでしょうか?
    • 将来VS2017への対応が廃止されると、この動作は合理性を失うのではないでしょうか。

参考資料

Visual Studio Community 2017 workload and component IDs | Microsoft Docs

C++によるデスクトップ開発(`Microsoft.VisualStudio.Workload.NativeDesktop`)ワークロードにおいて、
MSBuild(`Microsoft.Component.MSBuild`)は必須となっていますが、
ビルドツール(`Microsoft.VisualStudio.Component.VC.Tools.x86.x64`)は推奨という扱いです。
後者はインストールしないでおくこともできますが、サクラエディタのビルドには必須です(.vsconfigに記述した通りです)。
@ghost ghost added the question Further information is requested 【ChangeLog除外】 label Jan 9, 2022
@berryzplus
Copy link
Contributor

探索部分の3がおかしいような気がしましたが、おそらく仕様通りです。

仮選択したバージョンのMSBuildが存在するか調べる
存在しなかった場合はVS2017のMSBuildを探し、改めてバージョン15を選択する
もしVS2017のMSBuildが見つからない場合でも、バージョンは15が選択される

記憶があいまいですが、初期はvs2017同梱の「機能制限のあるvswhere」を使わなければならなかったため、vs2017用の探索ロジックとvs2019用の探索ロジックを分けていたように思います。

こういう感じ
1, vs2017同梱verのvswhereでは正しくパスを取得できないロジックで検索する。
2. パスが取れなかった場合は vswhere が古い(≒vs2017しかインストールされていない)と見做す。

ちゃんと確認してないので間違いあるかもです。

@dep5
Copy link
Contributor

dep5 commented Jan 9, 2022

まず現状の動作です

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
Copy link
Contributor

質問回答

  1. 見つからない、はありえないです。
    ただし、vswhereが期待通りに機能しないケースがありえるのでそういう構成にしたはずです。
  2. 不具合により機能してないと思います。
    個人的にはドキュメントも変な感じ(≒nealry equals 不具合)だと思っています。
  3. プロジェクト2年目くらいまでは「vs2017でのビルドは永遠にサポートする」というノリだったためです。
    雰囲気以上の理由はなかったんじゃないかと思います。

@dep5
Copy link
Contributor

dep5 commented Jan 9, 2022

選択されたバージョンをNUM_VSVERSIONに、見つかったMSBuildへのパスをCMD_MSBUILDに設定する`

NUM_VSVERSIONはCMD_MSBUILDを設定するために使われており、ここ以外では使わていないようです。

不思議なのはARG_VSVERSIONを使うことに抵抗を感じる方がいることです。
ARGという名前だから引数として使うべきだ、ということなのでしょうか?

@ghost
Copy link
Author

ghost commented Jan 9, 2022

「vswhereが期待通りにいかないことがあったから万が一に備えてこうしている」というで理解しました。
軽く調べた感じだと microsoft/vswhere#180 とかありました。

@ghost
Copy link
Author

ghost commented Jan 9, 2022

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
Copy link
Contributor

berryzplus commented Jan 10, 2022

不思議なのはARG_VSVERSIONを使うことに抵抗を感じる方がいることです。
ARGという名前だから引数として使うべきだ、ということなのでしょうか?

他のバッチ処理はどうなのか分からないですが、
*.targetsから環境変数経由で渡るバージョンは、
起動中のvisual studioのものが渡ってほしいです。

ARG_VSVERSIONを指定した場合、変換が入ります。
指定したのと「同じ意味」の値がNUM_VSVERIONに設定される仕様です。
「同じ意味」ではなく「同じ」が欲しいです。

「環境変素なのにARGなのは不適切じゃなかろうか?」は、それとは別の話だと思っています。

@berryzplus
Copy link
Contributor

あらためてARG_VSVERSIONをgrepしてみたんですが、CIで使ってますね。(誤解解消)

sakura/azure-pipelines.yml

Lines 109 to 114 in 25281cd

- template: ci/azure-pipelines/template.job.build-unittest.yml
parameters:
name: VS2019
vmImage: 'windows-2019'
displayName: VS2019
ARG_VSVERSION: 16

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

第3パラメータを追加して、内部変数 ARG_VSVERSION に代入します。
実際に引数なのであればARGが付いていても文句はないと思います。

修正量がボチボチ大きくなるのが懸念ですが、ちょっと見やすくなるはず。。。

@berryzplus
Copy link
Contributor

検証のために触ってみた過程で「以下の一文が機能しないことがある」と気付きました。

&& set "NUM_VSVERSION=%NUM_VSVERSION%" ^

関連するのは #1167 で、
バッチで出力させるパラメータが多過ぎる
が原因なんではないかと思っています。

根治策は「要らんやつを削るぞ~」なんですけど、
基本的に「必要だから追加した」ものばかりなので、
選抜するのは精神的にかなりしんどいだろうと予想しています。

@dep5
Copy link
Contributor

dep5 commented Jan 10, 2022

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
Copy link
Contributor

berryzplus commented Jan 10, 2022

ARG_VSVERSION = バージョンを「手入力」するためのもの。
NUM_VSVERSION = 判定されたバージョンを保持しておくためのもの。

#1759 (comment) で書いた通り、NUM_VSVERSIONが正しく設定されない、がたまに発生するようです。これの原因は「プログラムの誤り」ではなさそうですが、期待通りに動かないのは不具合として修正すべきと思います。

@dep5
Copy link
Contributor

dep5 commented Jan 10, 2022

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
Copy link
Member

検証のために触ってみた過程で「以下の一文が機能しないことがある」と気付きました。

バッチファイルの改行コードはどうなっていますか?
もしLFになっているようであれば #1761 で直ってほしいなと思います。

@dep5
Copy link
Contributor

dep5 commented Jan 10, 2022

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
Copy link
Author

ghost commented Jan 10, 2022

ちょっと考え直してみました。ビルドはこういう過程を経ているように見えます。

  1. VSでsakura.slnを開くか、build-sln.batを実行してビルドを開始する
  2. ビルドの過程でtargetsファイルに記述されたコマンドが実行される
    • 実行中のVSバージョンをNUM_VSVERSIONに設定した状態でcmdファイルが呼ばれる
  3. cmdファイルから必要なツールを探すためにfind-tools.batが呼ばれ、ここで②で設定されたNUM_VSVERSIONが参照/変更される

ここで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>end

NUM_VSVERSIONにはVisualStudioVersionの「16」が入っていたはずなのに、find-tools.batの処理で書き換わり、あたかもVS2022のツールセットを使っているようなログになります。

@ghost
Copy link
Author

ghost commented Jan 10, 2022

この年号との比較が嫌だということですか?

16と17の利用可能な二つのバージョンが入っていて、「俺は16を使いたいんだ」とします。
ARG_VSVERSIONだとそこから(年号指定の場合は変換した上で)それは本当に利用できるのか確認する処理が入ります。
…が、16を利用できることが自明ならそんな処理は必要ないでしょう。

一つしか入ってない環境でもARG_VSVERSIONを指定するのは意味があります。
ARG_VSVERSIONを設定しないとまずVS2017を探すからです。
将来別の環境に乗り換えてもARG_VSVERSIONが間違ってるとエラーにはなりません。

今の動作にこだわるならそういう解釈ができなくもないですが、疑問③に対していただいた回答ではこだわる理由はないように見受けられます。
何かの機会にその動作をやめればいいだけ。

@ghost
Copy link
Author

ghost commented Jan 10, 2022

#1759 (comment)
レスを書いたことでようやく気が付きました。
直さなきゃいけないのは「VS2019上でのビルド時にVS2022のツールが選択されてしまうこと」ですね。

@ghost
Copy link
Author

ghost commented Feb 24, 2022

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

@ghost ghost reopened this Feb 24, 2022
@ghost ghost linked a pull request Feb 24, 2022 that will close this issue
@ghost ghost added environment 環境関連全般【ChangeLog除外】 and removed question Further information is requested 【ChangeLog除外】 labels Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
environment 環境関連全般【ChangeLog除外】
Projects
None yet
3 participants