複数のVisual Studioを入れたときの問題に対応 #1806

dep5 · 2022-02-21T14:19:48Z

PR の目的

複数のVisual Studioをインストールしている場合
ビルド時に違うバージョンのcmakeなどが選択される問題を修正します
~~修正案をいただいたので併記しています。(#1806 (comment))~~

カテゴリ

~~- 不具合修正~~

ビルド関連
~~- ビルド手順~~
~~- ローカルビルド~~
- その他

PR の背景

tools/find-tools.batの探索順が変更になり
NUM_VSVERSION -> VS2022 -> VS2019 -> VS2017
このような優先順で選ばれるようになりました
複数インストールしている場合、
下位のバージョンを使うにはNUM_VSVERSIONを設定する必要があります。
IDEでは使用中のバージョンを (targetsファイルを使って) 環境変数NUM_VSVERSIONに設定する必要があります。

VS2017も入っている環境で、VS2017のツールを探索した際、CMakeとNinjaのみ異なるバージョンのツールが選択されます。

set NUM_VSVERSION=15
find-tools.bat
C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -property installationPath -version [15,)
|- CMD_MSBUILD=C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\MSBuild.exe
|- CMD_CMAKE=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe
|- CMD_NINJA=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja\ninja.exe

#1807 (comment)

PR のメリット

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

~~呼び出し順を変えるので何か不具合があるかもしれません。~~

仕様・動作説明

例としてVS2022とVS2017をインストールした環境の話です。

１．
IDE/コマンドライン
Visual Studio 2017でビルドすると違うバージョンのツールが選択される
-> tools/find-tools.batでのNUM_VSVERSION_NEXTの設定忘れです。

#1785 で、VS2017とそれより後のバージョンで、探索処理を行うサブルーチンを分けました。
VS2019以降の場合、Visual Studio Locatorの引数で、選択したバージョン（NUM_VSVERSION）と、それに1加えた値（NUM_VSVERSION_NEXT）を使って、そのバージョンのMSBuildを探すようになりました。
一方、VS2017の場合は、Visual Studio Locatorの引数に固定値（[15,16)）を用います。
このため、VS2017ではNUM_VSVERSION_NEXT変数が定義されません。

さて、CMakeおよびNinjaの探索を行う処理でも、Visual Studio Locatorを使っていますが、引数に選択したバージョンとそれに1加えた値を利用しています。
ここで本来NUM_VSVERSION_NEXTを定義しなければなりませんでしたが、CMakeの探索処理をfind-tools.batに実装した際に定義の漏れがありました。
#1785 で変更する以前はMSBuild探索時に定義された値が再利用されていたため影響はありませんでしたが、 #1785 でサブルーチンを整理した結果、NUM_VSVERSION_NEXTが定義されないコードパスが生じ、定義漏れの影響が顕在しました。

このPRでは、漏れている定義を追加して、VS2017との混在環境でもCMake/Ninja探索時に正常に動作するようにします。

２．
IDEのみ
sakura_core\githash.h が存在しない場合
Visual Studio 2017 IDEでビルドすると違うバージョンのツールが~~選択される。~~選択されたログが出力される。
-> NUM_VSVERSIONを設定しないままtools/find-tools.batが呼ばれている。

また、ビルド時にgit関係のヘッダファイルを生成するgithash.batを実行するターゲットでは、NUM_VSVERSIONの設定がありません。
このため、githash.batの実行ログでは異なるバージョンのツールが選択されているように見えます。
このPRでは、当該ターゲットでもあらかじめNUM_VSVERSIONを設定するようにして、使用しているVisual Studioと同じバージョンのツールが選択されるようにします。

sakura\githash.targets に環境変数NUM_VSVERSIONを設定する方法と
sakura\sakura.vcxproj の呼び出し順でtests\compiletests.targets を優先させる方法があると思います。

依存関係が無ければ順を変えるほうが
環境変数NUM_VSVERSIONを設定する箇所を
tests\unittests\tests1.vcxproj - tests\googletest.targets - googletest.build.cmd
sakura\sakura.vcxproj - tests\compiletests.targets - compiletests.run.cmd
の二つに固定できるのでいいと思います。

PR の影響範囲

~~いくつかファイルの呼び出し先を追ってみた感じでは、依存関係が無いように思いましたが、どうでしょうか？~~

テスト内容

2つ以上インストールした最新でないほうのVSを起動し、ビルドを行う。
IDEのテストではsakura_core\githash.hを削除した状態でもテストする。

テスト1

手順

参考資料

ghost · 2022-02-21T14:36:48Z

NUM_VSVERSIONの値は以下の記述により設定されています。

sakura/tests/compiletests.targets

Line 20 in 0c8db74

sakura/tests/googletest.targets

Line 38 in 0c8db74

ghost · 2022-02-21T14:39:07Z

0c8db74 をVisual Studio 2019 でビルドしたときのログ（抜粋）です。

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>find-tools.bat
4>|- CMD_GIT=C:\Program Files\Git\cmd\git.exe
4>|- CMD_7Z=C:\home\kazasaku\.local\bin\7z.exe
4>|- CMD_HHC=C:\Program Files (x86)\HTML Help Workshop\hhc.exe
4>|- CMD_ISCC=
4>|- CMD_CPPCHECK=C:\Program Files\Cppcheck\cppcheck.exe
4>|- CMD_DOXYGEN=
4>|- CMD_VSWHERE=C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe
4>|- CMD_MSBUILD=C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe
4>|- CMD_CMAKE=C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe
4>|- CMD_NINJA=C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja\ninja.exe
4>|- CMD_LEPROC=C:\home\kazasaku\.local\bin\LEProc.exe
4>|- CMD_PYTHON=C:\Windows\py.exe
4>|- NUM_VSVERSION=16
4>|- CMAKE_G_PARAM=Visual Studio 16 2019
4>end
4>-- The C compiler identification is MSVC 19.29.30140.0
4>-- The CXX compiler identification is MSVC 19.29.30140.0

上記の通り、CMD_CMAKEにはVS2019付属のものが選択されています。

AppVeyorBot · 2022-02-21T14:49:24Z

✅ Build sakura 1.0.4055 completed (commit 949c4af701 by @dep5)

ghost · 2022-02-21T14:50:19Z

sakura\githash.targets に環境変数NUM_VSVERSIONを設定する

githash.batにおけるfind-tools.batの呼び出しは、 Git for Windows の所在確認が目的です。
CMake/MSBuildは同バッチファイル内では使われませんので、NUM_VSVERSIONの指定は不要です。

ghost · 2022-02-21T14:53:25Z

なお、混在環境におけるツール選択のトラブルは #1785 でロジックを見直したことにより解消されています。

ghost · 2022-02-21T15:31:07Z

tools/find-tools.batでのNUM_VSVERSION_NEXTの設定忘れです。

#1785 よりも前から存在した漏れみたいなのでフォローアップ対応（ #1807 ）を行います。

berryzplus

ぱっと見で良く分からなかったのでスルーしましたが、
修正内容はイケてそうに見えました。

berryzplus · 2022-02-22T15:11:03Z

sakura/sakura.vcxproj

  <Import Project="..\sakura\githash.targets" />
  <Import Project="..\sakura\funccode.targets" />
  <Import Project="..\sakura\ExtractArchives.targets" />
-  <Import Project="..\tests\compiletests.targets" />


この行の修正根拠がわかりませんでした。

（IDEでは）記載順にtargetsが呼ばれるようなので
githashでfind-tools.batが呼ばれる前に
compiletestsでfind-tools.batを呼んでしまう、ということです。
これによってgithash.hの有無で動作が変わるのを防げます。

何か大事なことを(ぼくが)読み取れて無さそうな気がします。

githashでfind-tools.batが呼ばれる前に
compiletestsでfind-tools.batを呼んでしまう、ということです。

compieltestsの記述準は、元々githashの後で、このPRで入れ替えようとしてるように見えています。

「githashで呼ぶ前にcompiletestsで呼んでしまうとマズい」と言ってるように見えますが、読んだらマズいなら変えなくてよいのではないかと思いました。何が抜けてるんでしょう・・・

これによってgithash.hの有無で動作が変わるのを防げます。

githash.hを生成できない状況（≒ソースzipからのビルド）について、正しくビルドできる想定をするのは何か痴愚用な気がします。

ものすごく重要なことに思い当たりました。
ここの書き順を変えても、処理順には影響しないです。。。。

レス付けるために現状のコードを確認したところ、
現時点では確かに、書き順を入れ替えたらcompiletestsを先に実行させることができることに気付きました。
ただし、これは「githashの実行タイミングの指示が誤っているため」なのですっごく微妙です。。。（詳細は後述

berryzplus · 2022-02-22T15:12:30Z

tools/find-tools.bat

@@ -257,6 +257,7 @@ exit /b
    exit /b

 :cmake
+set /a NUM_VSVERSION_NEXT=NUM_VSVERSION + 1


この修正は妥当だと思います。

dep5 · 2022-02-23T13:42:05Z

コメントありがとうございます。

いつもこのようにビルドしているのでテスト条件に書き忘れていました。
すいません。
#1755

IDEのソリューションエクスプローラーで
sakuraプロジェクトを右クリックした場合に起きます。

サクラエディタソース内ではfind-tools.batを呼び出す前にその時必要な該当変数の確認がされています。
つまりサクラエディタ1回のビルドでfind-tools.batは1回しか呼び出されません。(変数が設定済みなので)

サクラエディタソース内でfind-tools.batで設定されるツールは
VSバージョンごとに用意されているもの(CMake)とそれ以外(git)があります。
find-tools.batを初回に呼び出すときにすべてのツール設定に必要な情報を設定しておく必要があります。

Gitを設定する場合でもCMake用にNUM_VSVERSIONの指定が必要です。

dep5 · 2022-02-23T13:42:52Z

kazasakuさん

1.sakura_core\githash.hを削除してから
2.VSを二つ以上入れた環境で最新版でないIDEからビルドした際の問題です。

2022と2019の混在環境でgithash.hを削除後のビルドログとみてよいですか？

ログ（抜粋）というのはログの冒頭部分という意味ですか？
それとも、今回のPRに関係ない部分を編集しているということですか？
というのも、
「ビルドの開始時の環境:
...
VSAPPIDDIR
VSAPPIDNAME
...
」
というのを見たことがない気がして…。
ログを全部確認したいという意味ではないです。

dep5 · 2022-02-23T14:41:14Z

質問なのですが
#1807
同じ内容のPRを出すのはどういう意味があるのですか？
ドラフトを外せなくなるし、こちら側のコミットを修正するか、
PRを出し直さないといけなくなるので、困るのですが…。

ghost · 2022-02-23T15:44:04Z

kazasakuさん

1.sakura_core\githash.hを削除してから 2.VSを二つ以上入れた環境で最新版でないIDEからビルドした際の問題です。

2022と2019の混在環境でgithash.hを削除後のビルドログとみてよいですか？

ログ（抜粋）というのはログの冒頭部分という意味ですか？それとも、今回のPRに関係ない部分を編集しているということですか？というのも、「ビルドの開始時の環境: ... VSAPPIDDIR VSAPPIDNAME ... 」というのを見たことがない気がして…。ログを全部確認したいという意味ではないです。

はい。ここに貼ったものは

VS2019とVS2022の混在環境で、VS2019（＝古い方）でビルドしたもの。
githast.hは事前に削除済み
4つあるプロジェクトのうち、tests1プロジェクトの部分の冒頭
- 4番目のプロジェクトなので、行頭に「4>」が付いている
ビルド対象はmasterブランチであり、このPRのブランチではない

…です。

なお、「ビルドの開始時の環境」は、VSのツールバーにある「ツール」メニューから、「オプション」→「プロジェクトおよびソリューション」→「VC++プロジェクトの設定」と開き、「ビルド - ログで環境を表示」で出力を切り替えられます（既定では無効）。
VisualStudioVersionの値を確認したいので出力させるようにしています。

ちなみに、ログ全文は約900行あり、「find-tools.bat」で検索すると3行ヒットします。
それぞれgithash.targets・compiletests.targets・googletest.targetsからの呼び出しによります。
どれも想定通りに機能しているように見えます。

付録： 90d6766 をビルドしたときのログ全文
vs2019_90d67663_x86_dbg.txt

ghost · 2022-02-23T16:05:54Z

質問なのですが #1807 同じ内容のPRを出すのはどういう意味があるのですか？ドラフトを外せなくなるし、こちら側のコミットを修正するか、 PRを出し直さないといけなくなるので、困るのですが…。

PRの目的が完全に異なっています。ゆえに別物です。

このPRの目的は「ビルド時に違うバージョンのcmakeなどが選択される問題を修正」ですが、僕が見る限りそのような事象が生じているようには見えません。
そのような事象が生じたときの完全なビルドログを公開してほしいです。
~~実際に生じているという確信が得られれば #1807 は取り下げます。~~

berryzplus · 2022-02-23T16:29:53Z

#1807 はこのPRの修正の一部切り出しですが、目的・修正内容ともに妥当だと思います。

対して、このPRは#1807に加えて「よく分からない修正」を含んでいます。

今頃になって全貌が把握できてきたのですが、
find-tools.batを呼び出すタスクの実行時には SetEnv で NUM_VSVERSION を設定しておく必要がある
という見解なら、そのように修正したらいいと思います。

SetEnvしてるのはcompiletestsだからこれを最初に実行しよう、だから「なんで？」になりました。

ghost · 2022-02-23T16:54:35Z

find-tools.batを呼び出すタスクの実行時には SetEnv で NUM_VSVERSION を設定しておく必要がある

その見解に基づいてgithash.targetsにSetEnvを追加するだけ、という話だったら何も言わずにapproveしてましたね。
githash.targetsにSetEnvがないこと（＝ログ上ではバージョン選択ミスが起きているように見える）と、NUM_VSVERSION_NEXTの定義漏れは別の問題ですし。

補足：#1807 (comment)

berryzplus · 2022-02-24T01:09:43Z

問題はいくつかありそうです。

find-tools.bat を呼び出すバッチは、 SetEnv で visual studio のバージョンをバッチに渡してやる必要がある。
ビルド工程で最初に実行される「find-tools.batを呼び出すタスク githash」に SetEnv の定義がない。
find-tools.bat へのバージョン指定は、１回目の呼出以外は意味がない。
find-tools.bat 内で cmake を検索するときに、未定義の環境変数 NUM_VSVERSION_NEXT を参照している。 find-tools.batが一時利用する変数の宣言を追加する #1807

問題が何で、原因が何で、の説明ができたら、直すだけです。
誰がやっても構わない修正だと思っています。
問題に気付けたのは @dep5 さんのおかげなので、このPRを修正して進めるのがベターだと思っています。

今回の原因に対して「ビルド順を変える」は違う気がします。

find-tools.batを呼ぶには定義が必要なんだから対象タスク全部を修正するんじゃね？
　↓でも、使われるのは最初の定義だけじゃんね？
じゃあ、最初のタスクだけの修正にしようぜ！
　↓いま書いてある定義はどうする？
使われなくなるなら削っちゃおうぜ！

というような論理の組み立てになるので、
「誰がやっても同じ」にはならないですが、
「誰がやってもいい」と思います。

ghost · 2022-02-24T08:49:06Z

ちょっと整理。

Visual Studio 2022に対応する #1755 でfind-tools.batでのNUM_VSVERSION環境変数の取り扱いについて問題が指摘される
- 話題は find-tools.batにおけるMSBuildの探索動作を再検討する #1759 に分離して検討。
MSBuildの探索手順を変更する #1785 で、すでにあらかじめ設定したNUM_VSVERSIONの値が無視されることに対して、ロジックの変更で対処
- find-tools.batにおけるMSBuildの探索動作を再検討する #1759 は解決されたとみなしてクローズ。
もともとNUM_VSVERSIONを設定していないgithash.targetsによる呼び出しは現時点でそのまま。
- @dep5 さんは多分ここを気にしている。

このPRは次の点を指摘するものだと考えます。

githash.targetsにNUM_VSVERSIONの設定がないので、ログ上ではVS2022のツールを選択した旨が依然として出力されている。
- つまり find-tools.batにおけるMSBuildの探索動作を再検討する #1759 が未解決だと指摘している。
- （ただし、githash.bat内ではそれらは使用されないので動作上の影響はないはず。）

ゆえに必要なのは「githash.targetsでも設定してから呼び出す」ようにして、 #1759 を閉じることだと思います。

NUM_VSVERSION_NEXTの件は #1785 よりも前からある問題であり、 #1759 とは無関係です。
（git blameした限りでは、CMakeの探索をfind-tools.batに追加したときの手落ちのようでした。）
なので同時対応しない方がわかりやすいと思います。

dep5 · 2022-02-24T13:06:32Z

kazasakuさん
完全ログをありがとうございます

49行め

1>VisualStudioEdition            = Microsoft Visual Studio Community 2019
1>VisualStudioVersion            = 16.0

126行め

2>---- Make githash.h ----
2>find-tools.bat
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

バージョンが違いますよね。これを修正したいのです。
githash.targetsの呼び出し順を変更して、修正できるか試してもらえませんか。

これも最近の変更とは関係ない以前からあるバグですのでまとめちゃってもいいかと思います。

dep5 · 2022-02-24T13:23:35Z

berryzplusさん

ビルド工程で最初に実行される「find-tools.batを呼び出すタスク githash」に SetEnv の定義がない。

githashは一度作成されるとリビルドでは作り直されないようです。
ソースを展開するところから始めるようなケースでは sakura\githash.targets
修正しながらビルドをするようなケースでは tests\compiletests.targets
2つのルートを想定しないといけないので、この際順を変えてしまったらいいのでは、と思います。

順を変えることで不具合が起きたり、元の呼び出し順に何か理由があれば、githash.targetsの変更でも構いません。

berryzplus · 2022-02-24T14:16:29Z

githashは一度作成されるとリビルドでは作り直されないようです。

この「一度作成されると呼び出されない」が間違ってるように思っています。
また、リビルドで再作成されない原因は githash.h がCleanタスクで削除する対象になっていないからです。
#1757 で対応をやりかけましたが、推奨しない手順でビルドした場合に問題があることが指摘されて、
説明がめんどくさくなったので取り下げています。

2つのルートを想定しないといけないので、この際順を変えてしまったらいいのでは、と思います。

論理構造的には、find-tools.bat を呼び出す「すべてのタスク」で SetEnv したほうが良いと思います。
ただし、最初に実行されるタスクが1つに決まるなら、1つだけの修正でもよいはずです。
「すべて vs １つか２つ」だったら後者を選択したくなるのが人情ではないかと思います。

MsBuildのタスク実行順は、属性定義により相対的に決まります。

sakura/sakura/githash.targets

Lines 8 to 9 in 90d6766

    
           AfterTargets="SelectClCompile" 
        
           BeforeTargets="ClCompile">

sakura/tests/compiletests.targets

Line 14 in 90d6766

    
           <Target Name="RunCompileTests" DependsOnTargets="MakeCompileTestBuildDir" BeforeTargets="ClCompile" Inputs="@(CompileTestTarget)" Outputs="$(CompileTestBuildDir)CMakeCache.txt">

現時点では「ClCompileの前」が共通するので、先に定義したものが先に実行されます。
しかし、タスクの意味合いが違うので、実行順を入れ替えると変です。

githashタスク　ソースファイル githash.h を生成します。
ソースファイルはコンパイル開始前までに完了させたい。
compileTestsタスク　ソースファイルが妥当かどうかを検証します。
テンプレートクラスの互換チェックなのでビルド前に完了させたい。

ソースを生成する前に、ソースコードのチェックをしていいの？　ということになります。

ghost · 2022-02-24T16:11:28Z

@dep5

49行め

1>VisualStudioEdition            = Microsoft Visual Studio Community 2019
1>VisualStudioVersion            = 16.0

126行め

2>---- Make githash.h ----
2>find-tools.bat
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

バージョンが違いますよね。これを修正したいのです。 githash.targetsの呼び出し順を変更して、修正できるか試してもらえませんか。

3個あるターゲットのうち、NUM_VSVERSIONをあらかじめ定義していないgithash.targetsからの呼び出し時のログも対応したいのでしょう？
僕の言ってることと何が違いますか？？
そうしたいならそうすればいいです…とすでに申し上げています。

これも最近の変更とは関係ない以前からあるバグですのでまとめちゃってもいいかと思います。

PR本文に書き方が正しくないので受け入れたくないです。
この点を問題の原因と捉えているようですが、 #1807 のコメントで説明した通り違っています。
PRの目的と関連がないが同時にやるというなら、「本問題とは無関係ですが同時対応します」と書けば誤解されずに済みました。
そう書かなかった以上、僕は分離を求めています。

dep5 · 2022-02-24T16:14:03Z

berryzplusさん

リビルドで再作成されない原因は githash.h がCleanタスクで削除する対象になっていないからです。

そういう経緯だったんですね。以前、ビルドの失敗が続いて、
githash.hを削除したらうまくいくようになりました。

ソースを生成する前に、ソースコードのチェックをしていいの？　ということになります。

compileTestsの前にgithash.hを含めてすべてのファイルをそろえておくために、この順になっているのですね。順序を変えるのはやめました。

ghost · 2022-02-24T16:21:40Z

リビルドで再作成されない原因は githash.h がCleanタスクで削除する対象になっていないからです。

そういう経緯だったんですね。以前、ビルドの失敗が続いて、 githash.hを削除したらうまくいくようになりました。

#1757 で、僕はこう書きました。

プロジェクトごとにビルド/クリーンしているような場合は大丈夫なんでしょうか？

これはツールバーの「ビルド」メニューにある「（プロジェクト名）をビルド」、あるいはソリューションエクスプローラーで、プロジェクトを右クリックして表示されるメニューの「ビルド」をクリックしてビルドしている場合を指しています。本PRのコメントから察するに @dep5 さんのビルドのやり方は多分これなんじゃないかと推測します。

sonarcloud · 2022-02-24T16:29:45Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

ghost

ソースを生成する前に、ソースコードのチェックをしていいの？　ということになります。

compileTestsの前にgithash.hを含めてすべてのファイルをそろえておくために、この順になっているのですね。順序を変えるのはやめました。

PR本文は並べ替えることを前提に書かれているので本文の修正が必要です。

AppVeyorBot · 2022-02-24T16:32:20Z

✅ Build sakura 1.0.4061 completed (commit 9ebaf755e4 by @dep5)

dep5 · 2022-02-24T16:50:41Z

kazasaku さん

#1806 (comment)

4>VisualStudioEdition            = Microsoft Visual Studio Community 2019
4>|- CMD_CMAKE=C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe

ではバージョンが一致していますね。
このPRの再現はできていません。(再現する必要はないですが)

#1806 (comment)

1>VisualStudioEdition            = Microsoft Visual Studio Community 2019
2>|- CMD_CMAKE=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe

では異なったバージョンです。
このPRの再現が出来ています。

わたしは今まで行頭の数字は何だろうと思っていたのですが。プロジェクトを示しているのですね。
VisualStudioVersionの値の確認方法も今まで不思議だったので教えてくれてありがとうございます。

ログではなく、選ばれるツールのバージョンを想定通りにしたいです。
1回しか呼ばれないから初回にすべて対応する必要があります。

わたしはIDEからのビルドがメインですので、ビルド時の問題に対応という意味で一体なのですが、
コマンドラインでのビルドがメインの方はそう思わないかもしれません。
個人的にはコミット履歴から、PRに飛んだ時にある程度情報がまとまっていてほしいのですが…。
PR本文に追記しました。

ghost · 2022-02-24T16:54:11Z

#1807 を捨ててこちらで進めます。

@dep5
この後のコメントに、あなたが遭遇している問題の説明と、その原因、そして解決に必要な対処の説明を書きます。
これをPR本文の各節に書いてください。
やり方：ハンバーガーアイコンからEditを押す

無理そうなら引き取ります。

ghost · 2022-02-24T17:05:39Z

PR の背景

VS2017とVS2022が混在した環境で、VS2017のツールを探索した際、CMakeとNinjaのみ異なるバージョンのツールが選択されます。

（ここに #1807 (comment) と同じログを挿入）

仕様・動作説明

#1785 で、VS2017とそれより後のバージョンで、探索処理を行うサブルーチンを分けました。
VS2019以降の場合、Visual Studio Locatorの引数で、選択したバージョン（NUM_VSVERSION）と、それに1加えた値（NUM_VSVERSION_NEXT）を使って、そのバージョンのMSBuildを探すようになりました。
一方、VS2017の場合は、Visual Studio Locatorの引数に固定値（[15,16)）を用います。
このため、VS2017ではNUM_VSVERSION_NEXT変数が定義されません。

さて、CMakeおよびNinjaの探索を行う処理でも、Visual Studio Locatorを使っていますが、引数に選択したバージョンとそれに1加えた値を利用しています。
ここで本来NUM_VSVERSION_NEXTを定義しなければなりませんでしたが、CMakeの探索処理をfind-tools.batに実装した際に定義の漏れがありました。
#1785 で変更する以前はMSBuild探索時に定義された値が再利用されていたため影響はありませんでしたが、 #1785 でサブルーチンを整理した結果、NUM_VSVERSION_NEXTが定義されないコードパスが生じ、定義漏れの影響が顕在しました。

このPRでは、漏れている定義を追加して、VS2017との混在環境でもCMake/Ninja探索時に正常に動作するようにします。

また、ビルド時にgit関係のヘッダファイルを生成するgithash.batを実行するターゲットでは、NUM_VSVERSIONの設定がありません。
このため、githash.batの実行ログでは異なるバージョンのツールが選択されているように見えます。
このPRでは、当該ターゲットでもあらかじめNUM_VSVERSIONを設定するようにして、使用しているVisual Studioと同じバージョンのツールが選択されるようにします。

dep5 · 2022-02-25T15:00:32Z

find-tools.batについてです。(IDE)
ソース内での使用例は以下のような感じで、環境変数を設定済みなら2回目は呼ばれないと今まで思っていました。

if not defined CMD_GIT call "%~dp0..\tools\find-tools.bat"
if not defined CMD_VSWHERE call %~dp0..\tools\find-tools.bat
if not defined CMD_NINJA call %~dp0..\tools\find-tools.bat

.bat（バッチファイル）のforコマンド解説。 - Qiita

for /f "usebackq tokens=1 delims==" %%i in (set) do echo %%i

こちらから拝借した環境変数を列挙するコマンドを該当行の上に追記してみたところ
find-tools.batを2回目に呼び出す直前にfind-tools.batが設定する変数は何もありませんでした。
1回目 - sakura\githash.bat
2回目 - tests\compiletests.run.cmd

したがって、初回のみ対応すればよいとしましたが
IDEから呼ばれる度に環境変数を設定する必要があるようですね。
訂正します。テストが不十分でした。
#1806 (comment)

ghost · 2022-02-25T15:13:08Z

修正案をいただいたので併記しています。(#1806 (comment))

🤔

berryzplus · 2022-02-25T15:28:03Z

このPRはどこに向かいます？
目的が明確で達成手段が妥当と判断できればどんな内容でも構わないと思っています。

ステータス的には request changes です。
PR本文の説明が実態と違っていて混乱を招きそうです　という趣旨の指摘が入ってる認識っす。。。

berryzplus · 2022-02-25T15:47:03Z

ぼくが個人的に「あかんやろ・・・」と思った点を、参考までに書き残しておきます。

カテゴリ

「事象の捉え方は個人の自由」なのですが、認識が誤っている気がします。

不具合修正

ビルド関連

ビルド手順

ローカルビルド

アプリの不具合ではないので「不具合修正」は正しくないです。
「ビルド手順」は変更されないので、これも正しくありません。
「ローカルビルド」は正しいですが、MSVCビルド全般に影響する変更なのでやや微妙です。
個人的には「その他」が最も適切じゃないか？という見解です。

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

呼び出し順を変えるので何か不具合があるかもしれません。

「呼出し順」は変えない変更になった認識です。
「変更によって何か不具合が出るかもしれません。」は、当たり前なので「自明」として省略でもよいです。

PR の影響範囲

~~いくつかファイルの呼び出し先を追ってみた感じでは、依存関係が無いように思いましたが、どうでしょうか？~~

最初に突っ込むべきでしたが、ここは「どうでしょうか？」を書く場所じゃないです。
この項目には想定の影響範囲を書いておき、
「テスト内容」が十分かどうかを判断できるようにします。

「この修正、自分の中では完璧っす！」を誤解なく伝えるための項目なので、
ここに疑問文を書かれると「なんでやねん！」のドツキ漫才になってしまいます。

ただ、「わかりません（キリッ」なら、それはそれでもよいです。

dep5 · 2022-02-27T13:59:57Z

berryzplus さん
ローカルでしかテストできず、
ビルド中の不具合修正なので選択しました。
結局当初の想定した原因と違っていたのが
カテゴリは用意された選択肢の中で必要なものを残すタイプですが
「アプリの不具合修正」と明記するか、ビルド関連のなかに「不具合修正」を用意しておいてほしいですね。
不具合修正をその他と書くのは抵抗を感じます・・・。

このPRで書いた当初一番教えてほしかったのが、
依存関係についてだったのですが、どこに書くべきでしたか？

本文中の不釣り合いな文章が打ち消されたので。

berryzplus · 2022-02-27T15:14:15Z

カテゴリは用意された選択肢の中で必要なものを残すタイプですが
「アプリの不具合修正」と明記するか、ビルド関連のなかに「不具合修正」を用意しておいてほしいですね。
不具合修正をその他と書くのは抵抗を感じます・・・。

ではテンプレ修正しましょう。
すぐやれる人がいたらお願いしたいです。
来週末までくらいで良ければ自分がPR作ります。

berryzplus · 2022-02-27T15:23:07Z

このPRで書いた当初一番教えてほしかったのが、
依存関係についてだったのですが、どこに書くべきでしたか？

このコメントの意味がわかりませんでした。

依存関係　「ビルドの依存関係」という意味で使う言葉かと思います。
MsBuildの設定ファイル内では DependsOn 属性で指定しますが、あんまり使ってないです。
実行順序のことを指しているなら、こないだ書いた通り相対的に決まるものなのでやや説明しづらいです。

「どのPRで作り込んだバグです」的な依存関係を書く場所はない気がします。
強いて言えば「PRの背景」がそういう説明の記述位置になると思ってます。

この辺も、具体的な改善案があれば試してみてよいと思います。

ghost · 2022-02-28T01:06:11Z

ではテンプレ修正しましょう。
すぐやれる人がいたらお願いしたいです。

「こうしたいです」という構想はあるので、用意しておきます。

ghost

承認しておきます。
本文の書き方は他のissueやPRを手本に勉強しましょう。

berryzplus · 2022-02-28T06:56:58Z

問題ないとおもうのでマージしておきます。
何かあれば別PRで対処していきましょう。

dep5 · 2022-02-28T12:37:09Z

berryzplusさん

IDEでfind-tools.batが初回のみ呼ばれるという誤った思い込みをしていたので、
当初の想定で話すと、
sakura\sakura.vcxprojの中に以下の記述があります。

  <Import Project="..\sakura\githash.targets" />
  <Import Project="..\sakura\funccode.targets" />
  <Import Project="..\sakura\ExtractArchives.targets" />
  <Import Project="..\tests\compiletests.targets" />

funccodeでgithashが呼ばれていたのでfunccodeのビルドにgithashが必要なのはわかりました。
compiletestsではgithashかfunccodeの呼び出しがなさそうだったので
どのような順にしてもよいと当初は判断しました。
しかし、 compiletestsの前にすべてのファイルを用意しなければいけないので、誤りだったわけです。
質問に答えてもらえてよかったです。
(find-tools.batがその都度呼ばれるのでもともとこの対策自体は意味がなかったわけですが。)

DependsOn 属性についてではなく、targetsそれぞれにビルドを進めていくうえで
ほかのtargetsのビルドを呼び出すことがあるのか、という意味で依存関係と使いました。
今回でいうと compiletests.targetsのビルドを進めて、
githash,funccode,ExtractArchivesを先にビルドを済ませておく必要があるかという質問でしたが、
この個所以外でも何か依存関係があるかもしれないと思って、ぼかした表現を使いました。

berryzplus · 2022-03-01T04:00:11Z

もう答えが出てそうですが・・・

今回でいうと compiletests.targetsのビルドを進めて、
githash,funccode,ExtractArchivesを先にビルドを済ませておく必要があるかという質問でしたが、
この個所以外でも何か依存関係があるかもしれないと思って、ぼかした表現を使いました。

githash　ソースコード生成
funccode　ソースコード生成（githashには依存しない）
ExtractArchives　ビルド後のアーカイブ展開処理

です。

こうしてみるとExtractArchivesの記述位置がおかしいです。

このあたりを修正してもアプリ機能に影響がないので「動けばいいかぁ・・・」で放置し続けています。

複数のVisual Studioを入れたときの問題に対応

11e78f4

ghost mentioned this pull request Feb 21, 2022

find-tools.batが一時利用する変数の宣言を追加する #1807

Closed

berryzplus reviewed Feb 22, 2022

View reviewed changes

ghost mentioned this pull request Feb 24, 2022

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

Closed

ghost linked an issue Feb 24, 2022 that may be closed by this pull request

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

Closed

ビルド順の変更をやめ、targetsに環境変数を設定する

c1b0547

ghost previously requested changes Feb 24, 2022

View reviewed changes

dep5 marked this pull request as ready for review February 25, 2022 15:02

ghost approved these changes Feb 28, 2022

View reviewed changes

berryzplus merged commit c8a7041 into sakura-editor:master Feb 28, 2022

ghost mentioned this pull request Mar 2, 2022

PRテンプレートで変更対象とカテゴリを分けて書くようにする #1814

Closed

複数のVisual Studioを入れたときの問題に対応 #1806

複数のVisual Studioを入れたときの問題に対応 #1806

Conversation

dep5 commented Feb 21, 2022 • edited Loading

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

テスト1

関連 issue, PR

参考資料

ghost commented Feb 21, 2022 • edited by ghost Loading

ghost commented Feb 21, 2022

AppVeyorBot commented Feb 21, 2022

ghost commented Feb 21, 2022

ghost commented Feb 21, 2022 • edited by ghost Loading

ghost commented Feb 21, 2022

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus Feb 22, 2022

Choose a reason for hiding this comment

dep5 Feb 23, 2022

Choose a reason for hiding this comment

berryzplus Feb 23, 2022

Choose a reason for hiding this comment

berryzplus Feb 24, 2022

Choose a reason for hiding this comment

berryzplus Feb 24, 2022

Choose a reason for hiding this comment

berryzplus Feb 22, 2022

Choose a reason for hiding this comment

dep5 commented Feb 23, 2022 • edited Loading

dep5 commented Feb 23, 2022

dep5 commented Feb 23, 2022

ghost commented Feb 23, 2022 • edited by ghost Loading

ghost commented Feb 23, 2022 • edited by ghost Loading

berryzplus commented Feb 23, 2022

ghost commented Feb 23, 2022 • edited by ghost Loading

berryzplus commented Feb 24, 2022

ghost commented Feb 24, 2022 • edited by ghost Loading

dep5 commented Feb 24, 2022

dep5 commented Feb 24, 2022

berryzplus commented Feb 24, 2022

ghost commented Feb 24, 2022 • edited by ghost Loading

dep5 commented Feb 24, 2022

ghost commented Feb 24, 2022

sonarcloud bot commented Feb 24, 2022

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

AppVeyorBot commented Feb 24, 2022

dep5 commented Feb 24, 2022

ghost commented Feb 24, 2022 • edited by ghost Loading

ghost commented Feb 24, 2022 • edited by ghost Loading

PR の背景

仕様・動作説明

dep5 commented Feb 25, 2022 • edited Loading

ghost commented Feb 25, 2022

berryzplus commented Feb 25, 2022

berryzplus commented Feb 25, 2022

カテゴリ

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

PR の影響範囲

dep5 commented Feb 27, 2022 • edited Loading

berryzplus commented Feb 27, 2022

berryzplus commented Feb 27, 2022

ghost commented Feb 28, 2022

ghost left a comment

Choose a reason for hiding this comment

berryzplus commented Feb 28, 2022

dep5 commented Feb 28, 2022 • edited Loading

berryzplus commented Mar 1, 2022

dep5 commented Feb 21, 2022 •

edited

Loading

ghost commented Feb 21, 2022 •

edited by ghost

Loading

ghost commented Feb 21, 2022 •

edited by ghost

Loading

dep5 commented Feb 23, 2022 •

edited

Loading

ghost commented Feb 23, 2022 •

edited by ghost

Loading

ghost commented Feb 23, 2022 •

edited by ghost

Loading

ghost commented Feb 23, 2022 •

edited by ghost

Loading

ghost commented Feb 24, 2022 •

edited by ghost

Loading

ghost commented Feb 24, 2022 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading

ghost commented Feb 24, 2022 •

edited by ghost

Loading

ghost commented Feb 24, 2022 •

edited by ghost

Loading

dep5 commented Feb 25, 2022 •

edited

Loading

dep5 commented Feb 27, 2022 •

edited

Loading

dep5 commented Feb 28, 2022 •

edited

Loading