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

MSBuildの探索手順を変更する #1785

Merged
6 commits merged into from Feb 2, 2022
Merged

MSBuildの探索手順を変更する #1785

6 commits merged into from Feb 2, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 30, 2022

PR の目的

#1763 に含まれるfind-tools.batの仕様変更を取り込みます。

カテゴリ

  • 仕様変更
    • ビルド関連
      • ビルド手順

PR の背景

#1755 を発端とした「find-tools.batの探索ロジック」の話題に関して、複数のVSが混在している場合に生じる次のような事例を #1759 (話題をissueに分割しています)で報告しました。
GoogleTestのコンパイル実施時、googletest.build.cmdを呼び出すときに設定したNUM_VSVERSION環境変数(中身はVisualStudioVersionの値)が反映されないというものです。

ここでtargetsファイルの記述が意図しているのは「実行中のVSと同じもの」ですから、「実行中のVSと同じ意味のもの」では困ります。よって現在の記述が正しいということになるはずです。

逆に言えば、もともと設定されていたNUM_VSVERSIONが書き換えられるのは(値を間違えていない限り)よろしくない気がします。
例えば、VS2019でソリューションをビルドしているときの、テスト(test1.vcxproj)部分のビルドログですが

4>------ ビルド開始: プロジェクト: tests1, 構成: Debug Win32 ------
(~~中略~~)
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のツールセットを使っているようなログになります。

Originally posted by @kazasaku in #1759 (comment)

#1763 に含まれている変更でこのような事象を回避できることが確認できましたので、masterへの取り込みを提案するものです。

PR のメリット

  • 以前と比べてロジックがシンプルになります。
  • NUM_VSVERSION環境変数に設定する値を間違えない限り、設定したバージョンが確実に選択されます。
  • 異なるバージョンのVisual Studioを混在させている環境でも、実行中のVSバージョンが確実に選択されます。

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

変更点一覧

  • あらかじめ定義したNUM_VSVERSIONの値を考慮するようになりました。
  • あらかじめ定義されたARG_VSVERSIONの値は利用しないようになりました。
  • 連続して呼び出したとき、引数の値が前回実行時に決定したバージョンと異なる場合は以前の結果を取り消して再設定するようになりました。
  • 引数未指定時の既定値を15 (VS2017)からlatestに変更しました。
  • MSBuildの探索手順を見直しました。
  • CMakeジェネレータ名(CMAKE_G_PARAM)の生成方法を見直しました。

動作

従来MSBuildの探索時に実施していた引数チェックは、独立したサブルーチンとして探索処理の実施前に行うようになりました。次のように動作します。

  1. 引数が指定されていない場合は、NUM_VSVERSIONが定義されていればその値を利用し、そうでなければ「latest」を指定したものとみなします。
  2. 引数に製品名(例:2017)が指定されている場合はメジャーバージョンに変換します。
    「latest」を指定した場合は実行環境にインストールされている最新のメジャーバージョンを取得します。
  3. 指定したバージョンがインストールされているか確認し、見つからなければ引数の指定はなかったものとみなしてチェックをやり直します。
  4. NUM_VSVERSIONが指定されていた時は、その値とここまでのチェックで見つかったバージョンが同じであるか確認し、異なる場合は環境変数を初期化した上で引数チェックをやり直します。
  5. ここまでの処理で決定したバージョンに該当するMSBuildを探索し、CMakeのジェネレータ名を生成します。

MSBuildの探索とCMakeジェネレータ名の生成プロセスは次のように動作します。

  • NUM_VSVERSIONの値が15(VS2017)の場合
    • VS2017のインストールパスを取得し、そのサブディレクトリにMSBuild.exeが存在するか確認します。
    • ジェネレータ名にはVisual Studio 15 2017(固定値)を設定します。
  • NUM_VSVERSIONの値が15以外の場合
    • vswhereコマンドの-findオプションを使用して、選択したバージョンのMSBuild.exeへのパスを取得します。
    • NUM_VSVERSIONの値と取得したプロダクトバージョン(例:2019)からジェネレータ名を生成します。

そのほか

#1763 そのままでは支障があったため、次の修正を加えています。

  • endlocalの呼び出しタイミングの変更
    • メインルーチン側でローカル化を終了させておかないと環境変数が初期化されません。
  • MSBuild探索時の参照変数の変更
    • ARG_VSVERSIONではなく、引数チェックで確定したNUM_VSVERSIONの値を参照するようにします。
  • コードフォーマットの統一(おまけ)
    • find-tools.batはスペースインデントされていますが、1行だけタブインデントになっていました😊

この変更に伴うドキュメントの更新は別途実施します。
【2022-02-01 追記】ドキュメント更新も実施します。

PR の影響範囲

  • 本体のビルドプロセス

テスト内容

  • NUM_VSVERSIONを定義していないか、無効なバージョンを設定している状態で、
    1. 引数なしで実行したとき、その環境で最新のVSが選択されること
    2. 無効なバージョンを指定したとき、その環境で最新のVSが選択されること
    3. 有効なバージョンを指定したとき、指定したバージョンのVSが選択されること
    4. 「latest」を指定したとき、その環境で最新のVSが選択されること
    5. 未指定を含む同一の引数で連続して呼び出したときに環境変数の設定が行われないこと
    6. 「clear」を指定したとき、設定された環境変数がNUM_VSVERSIONも含めて削除されること
  • NUM_VSVERSIONに有効なバージョンを設定した状態で、
    1. 引数なしで実行したとき、NUM_VSVERSIONに指定したバージョンのVSが選択されること
    2. 無効なバージョンを指定したとき、NUM_VSVERSIONに指定したバージョンのVSが選択されること
    3. 有効なバージョンを指定したとき、指定したバージョンのVSが選択されること
    4. 「latest」を指定したとき、その環境で最新のVSが選択されること
    5. 未指定を含む同一の引数で連続して呼び出したときに環境変数の設定が行われないこと
    6. 「clear」を指定したとき、設定された環境変数がNUM_VSVERSIONも含めて削除されること
  • ARG_VSVERSIONをあらかじめ定義した状態で実行し、その値が反映されていないこと

関連 issue, PR

#1755
close #1759
#1763
#1764

参考資料

berryzplus and others added 4 commits January 29, 2022 17:54
- 変数のローカル化をやめるタイミングの変更
- 探索処理では`ARG_VSVERSION`ではなく、値が確定している`NUM_VSVERSION`を参照する
- インデントに使用する文字を揃える
@ghost ghost mentioned this pull request Jan 30, 2022
@ghost
Copy link
Author

ghost commented Jan 30, 2022

#1763 に含まれる3つのコミットをcherry-pickして作成しました。
仕様説明は僕なりの理解に基づくものなので、いくらか齟齬があるかもしれません。

@AppVeyorBot
Copy link

Build sakura 1.0.4016 completed (commit a4fe340ed2 by @kazasaku)

@berryzplus
Copy link
Contributor

やっぱりなんかしら対応が必要ですよね。

そうじゃないかと思ってました。

#1763 を見返してみたら、かなりgdgdな印象を受けました。
実は、ちゃんと理解できてないのかも知れないです(笑

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ghost ghost marked this pull request as ready for review February 1, 2022 06:09
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.

良さそうに見えます。

ビルドスクリプトは、なんかあったら「何かおかしい」が比較的すぐに分かるので入れてしまえばよいと思います。

@ghost
Copy link
Author

ghost commented Feb 2, 2022

レビューありがとうございます。マージしてしまいます。
何かあればフォローアップしていきます。

@ghost ghost merged commit d3dfbf7 into sakura-editor:master Feb 2, 2022
@ghost ghost deleted the feature/change_msbuild_finding_process branch February 2, 2022 16:00
This pull request was closed.
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.

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