Program Files や Program Files (x86) のパスを決め打ちせずに環境変数を参照する

[m-tmatma]

#395: Program Files や Program Files (x86) のパスを決め打ちせずに環境変数を参照する

以下の環境変数を元に以下の順番で各種プログラムのパスを検索する

• %ProgramFiles%
• %ProgramFiles(x86)%
• %ProgramW6432%

tools/hhc/readme.md にバッチファイルを組む、使う上での注意事項を記載している

[m-tmatma]

“Resolve conversation” ボタンを押してみました。
https://help.github.com/articles/about-pull-request-reviews/#resolving-conversations

[k-takata]

この行はもう不要ですよね。（他のファイルも同様）

[m-tmatma]

削除しました。

[berryzplus]

“Resolve conversation” ボタンを押してみました。

まだまだ知らない機能がいっぱい・・・

[berryzplus]

ぼくとしてはmsbuildを外してもらえればLGTM出せます。

Msbuildにパスが通っていない場合の対処に疑問があるため、これについては別件としたいです。

[m-tmatma]

このコミットの意図が分かりません。

他ツールと異なり、vs2017は必ず32bit側のProgramFilesにインストールされます。

↑ このコメントに対応した修正です。

ぼくとしてはmsbuildを外してもらえればLGTM出せます。

この PR は『Program Files や Program Files (x86) のパスを決め打ち』に対する修正です。
他のエディションに対する対応は別件という認識です。必要なら別に対応すれば
いいと思います。

『Program Files や Program Files (x86) のパスを決め打ち』に対する修正で
msbuild を外すのは、あえてこの PR の対応を不完全にするだけのように思います。

msbuild を除外する理由の説明は不十分だと思います。

[m-tmatma]

#399 (comment)
にもコメント書いています。

[berryzplus]

変更前のバッチファイルが誤っている気がします。

https://www.appveyor.com/docs/build-phase/

このバッチの存在目的は「appveyorでのビルドを管理しやすくする」だと思っています。
ローカルビルド手順は「sakura.slnを開いてビルド」の認識です。
このバッチは appveyor 専用、使ったとしても「 appveyor と同じ順序のビルド」を検証する人だけです。

[m-tmatma]

ローカルビルド手順は「sakura.slnを開いてビルド」の認識です。

それは、ソリューションだけをビルドする場合です。

HTML ヘルプやインストーラもまとめて含めてローカルでビルドするときには
一連のバッチファイルを使います。

[berryzplus]

ぼく言ってること分かりにくいですか？

[m-tmatma]

中途半端な変更は許容したくないです。

@berryzplus さんの提案を受け入れると、逆にこの PR は中途半端になって価値を失います。

この PR は、パスを決め打ちしているのを Program Files 等の環境変数を使うようにする対応です。

なんのためにするかというと例えば 32bit OS や、C ドライブ以外にWindows をインストールした場合など
Program Files のパスがC ドライブ以外に存在する場合に、一連のバッチファイルが問題なく動作する
ようにすることです。

しかし、 @berryzplus さんの要求のように msbuild のパスに関して、Program Files 等の環境変数を
使用せず、パス決め打ちだと32bit OS や、C ドライブ以外にWindows をインストールした場合などに
build-sln.bat のビルドだけ失敗することになります。

このバッチは appveyor 専用、使ったとしても「 appveyor と同じ順序のビルド」を検証する人だけです。

そこが重要です。インストーラの開発をする人は必要な依存関係を一気にビルドする必要がありますが、appveyor に使っている build-all.bat を流せば、インストーラ作成まで一気にできます。
その際、その開発者が 32bit OS を使っている場合、プロジェクトのビルドの段階で失敗することになります。

[KENCHjp]

一応インストーラ開発環境作る為に（だけじゃないけども）、
一連のバッチをローカル実行でも動くようにしてもらった認識があります。

[berryzplus]

う～む。状況はなんとなく把握しました。

その際、その開発者が 32bit OS を使っている場合、プロジェクトのビルドの段階で失敗することになります。

それは困りますね。
ただ、ビルドには64bitOSを使って欲しいです。
エディタ本体の起動とは違って、ビルドを複数環境に対応させる意義には疑問を持ちました。

登場する環境変数の意味がちゃんと共有できてない気がしてきました。
コマンドプロンプトで set コマンドを叩くと、環境変数を一覧表示できます。
関係する変数をまとめるとこんな感じです。

環境変数	値	入ってるもの	備考
ProgramFiles	C:\Program Files	起動した cmd.exe の program files	可変
ProgramFiles(x86)	C:\Program Files (x86)	32bit の program files	不変
ProgramW6432	C:\Program Files	ネイティブ の program files	不変
PROCESSOR_ARCHITECTURE	AMD64	起動した cmd.exe の architecture	可変
PROCESSOR_ARCHITEW6432	AMD64	ネイティブ の architecture	不変

「可変」は、起動したcmd.exe が32bitか64bitかで変わる部分です。

[m-tmatma]

ただ、ビルドには64bitOSを使って欲しいです。
エディタ本体の起動とは違って、ビルドを複数環境に対応させる意義には疑問を持ちました。

そんなに技術的に難しいわけではなく、かつ実現方法が見えているのに
敢えて 32bit OS に対応させないようにするのは同意できません。

以前、 #148 で、git のリポジトリ外でビルドしたときにビルドエラーが発生すると
いう問題が発生して #191 で修正したことがありました。

これは git リポジトリ外でビルドした場合、GIT_SHORT_COMMIT_HASH という定数が
定義されないことによってビルドエラーになるというものでした。

サクラエディタの開発メンバー視点では、git リポジトリの中でビルドする前提だったので
git リポジトリ外でビルドするのを想定してなかったから、対応しません、と対応を
拒否することもできたかもしません。

しかし、#148 (comment)
で ZIPでソース一式ダウンロードしてビルドする人もいる という理由で修正しました。

同様の視点で、64bit OS でかつ、C ドライブに Windows がインストールされた環境
以外でも対応すれば(対応できるように汎用的に作れば)いいと思います。

[berryzplus]

これは git リポジトリ外でビルドした場合、GIT_SHORT_COMMIT_HASH という定数が
定義されないことによってビルドエラーになるというものでした。

この話はあんまり関係ないのかな、と思いました。
たとえ発案者が変更を拒否したとしても、別の人が要請して変わることもあると思ってます。
変更拒否のインパクトってたぶんそんなに大きくない気がします。

で、色々状況把握した結果、やはり 2287d4c の追加コミットの辻褄が合わないように思います。
これがなければ全部一律で「なんかあれば別で。」の筋が通りそうです。

[m-tmatma]

で、色々状況把握した結果、やはり 2287d4c の追加コミットの辻褄が合わないように思います。

#399 (review)
の

他ツールと異なり、vs2017は必ず32bit側のProgramFilesにインストールされます。

に対応するものです。

ProgramW6432 はネイティブの ProgramFiles なのでここに入っている可能性はないからです。

ほかと合わせるために ProgramW6432 を確認しても問題はないので、
0d32d1a で revert しました。

[m-tmatma]

問題なければ approve お願いします。
問題あれば指摘お願いします。

[berryzplus]

LGTMです。

対応ありがとうございます。環境変数の判定順序が少し気になっていたのですが、そちらはまた気の向いたときに対応したいと考えています。しばらくこれで行きましょう。