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

コマンドのエラー判定に if errorlevel を使うようにする #590

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Nov 1, 2018

コマンドのエラー判定に if errorlevel を使うようにする

なお、#559 (comment) のMinGWビルドのエラーをエラーとして検出できています。

このブランチで #559 適用前
https://ci.appveyor.com/project/m-tmatma/sakura/builds/19857732/job/9j6nv94ejb9qmdvl

このブランチで #559 適用後
https://ci.appveyor.com/project/m-tmatma/sakura/builds/19972548/job/rwxie98v4h7w5wj1

@ds14050
Copy link
Contributor

ds14050 commented Nov 2, 2018

なお、#559 (comment) のMinGWビルドのエラーをエラーとして検出できています。

これ、不思議です。99% のケースにおいてそういう違いが生じるようなものだとは思っていなかったので。


ところどころ popd が省略されているのはなにか考えの変化があったのでしょうか。


exit /b %errorlevel% より exit /b の方が安全みたいです。cmd.exe の終了コードは終了時点での errorlevel になるみたいですから、exit /B でパスできます。


見た感じ、論理が反転しているところはないみたいです。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 2, 2018

ところどころ popd が省略されているのはなにか考えの変化があったのでしょうか。

具体的にどこの話ですか?

exit /b %errorlevel% より exit /b の方が安全みたいです。cmd.exe の終了コードは終了時点での errorlevel になるみたいですから、exit /B でパスできます。

なにか参考資料はありますか?

@ds14050
Copy link
Contributor

ds14050 commented Nov 2, 2018

以下の popd です。


exit /B %ERRORLEVEL%

することはその時点で環境変数の %ERRORLEVEL% が本物の ERRORLEVEL になるという点で危険です。

exit /B が exit /B 0 とは違い ERRORLEVEL を変更しないことは実験で確かめました。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 2, 2018

以下の popd です。

* https://github.com/sakura-editor/sakura/pull/590/files#diff-a75839202fba4581171d2538752914f5L33

* https://github.com/sakura-editor/sakura/pull/590/files#diff-a75839202fba4581171d2538752914f5L36

どちらも対応する pushd がないので popd は不要です。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 2, 2018

exit /B %ERRORLEVEL%

することはその時点で環境変数の %ERRORLEVEL% が本物の ERRORLEVEL になるという点で危険です。

exit /B が exit /B 0 とは違い ERRORLEVEL を変更しないことは実験で確かめました。

どういう実験したのかを共有していただけますか?

@berryzplus
Copy link
Contributor

exit /b 0

前に議論した気がします。

たしか、バッチとして「1 or 0 を返す」の仕様にしたい、だったような。
それ自体は考え方として「あり」だと思いました。
現在もそれでいいと思っていますが、戻り値はなるべく上書きしないのが正だと考えています。

@ds14050
Copy link
Contributor

ds14050 commented Nov 3, 2018

どういう実験したのかを共有していただけますか?

面倒なので自分で考えてください。自分(ds14050)の主張が間違っていると確かめられたなら、それを教えてください。

exit /B が exit /B 0 とは違い ERRORLEVEL を変更しないことは実験で確かめました。

例外ケースをひとつだけ見つけました。

>type a.bat
call b.bat
exit /b
>type b.bat
exit /b 9

以上が準備です。ERRORLEVEL が 0 の状態で a.bat を呼び出す以下の4つの方法を試し、ERRORLEVEL を確認しました。

>cmd /C a.bat
>echo %ERRORLEVEL%
0
>cmd /C call a.bat
>echo %ERRORLEVEL%
9
>call a.bat
>echo %ERRORLEVEL%
9
>a.bat
>echo %ERRORLEVEL%
9

cmd.exe を通して call なしで直接 a.bat を呼び出した場合だけ、a.bat から ERRORLEVEL 変数が伝わってきませんでした。if errorlevel 1 で判定しても ERRORLEVEL は 0 のままでした。確認した例外はその1ケースです。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 3, 2018

どういう実験したのかを共有していただけますか?

面倒なので自分で考えてください。自分(ds14050)の主張が間違っていると確かめられたなら、それを教えてください。

その面倒というのは、実験したけど、それを記録していないから、書くのが面倒という意味ですか?

現状のやり方に問題があり、それを変えるように主張するなら、どういう論理でそのような
主張になるのかを説明すべきです。でないとその主張が正しいのかを検証できません。

することはその時点で環境変数の %ERRORLEVEL% が本物の ERRORLEVEL になるという点で危険です。

何が危険ですか?

@ds14050
Copy link
Contributor

ds14050 commented Nov 3, 2018

その面倒というのは、実験したけど、それを記録していないから、書くのが面倒という意味ですか?

上で4通りの方法を書きましたが、a.bat の中で b.bat を呼び出すのにも4通り考えられます。4×4 通りを網羅した実験をするのが面倒であるし、それによって「exit /B が exit /B 0 とは違い ERRORLEVEL を変更しない」ことの反例が存在しないことの証明にもならないので、自身で反例を見つけていただくしかないということです。自分は1つ見つけました。それを除けばほぼ間違いないようです。

することはその時点で環境変数の %ERRORLEVEL% が本物の ERRORLEVEL になるという点で危険です。

何が危険ですか?

書いてある通りです。環境変数としての ERRORLEVEL と本物の ERRORLEVEL の区別が曖昧になることを、自分は最初から嫌っていました>#580 (review)

「危険」というのは言葉の綾で、直ちに実害があるというニュアンスはありません。

@ds14050
Copy link
Contributor

ds14050 commented Nov 3, 2018

>cmd /V:ON
>type b.bat
@exit /B 9
>b.bat && echo !ERRORLEVEL!
9
>call b.bat && echo !ERRORLEVEL!

>b.bat || echo !ERRORLEVEL!

>call b.bat || echo !ERRORLEVEL!
9

取得できる ERRORLEVEL は変わりないのに call がある場合とない場合で論理が逆になるようです。

論理演算子に続く echo が実行されなかった場合に絞って echo !ERRORLEVEL! を実行させてみましたが、取得できる値はやはり 9 (非正常終了)でした。

>call b.bat & echo !ERRORLEVEL!
9
>b.bat & echo !ERRORLEVEL!
9

これが何の話かというと、call のあるなしで ERRORLEVEL が伝わってこない場合があったので、call とは何か、という調査の一環です。まだわかりません。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 4, 2018

たしか、バッチとして「1 or 0 を返す」の仕様にしたい、だったような。
それ自体は考え方として「あり」だと思いました。

%errorlevel% さえ確認できればいいので、%errorlevel% の値を表示して
1 を返すようにしようと思います。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 4, 2018

%errorlevel% さえ確認できればいいので、%errorlevel% の値を表示して
1 を返すようにしようと思います。

修正しました。

@ds14050
Copy link
Contributor

ds14050 commented Nov 5, 2018

%errorlevel% さえ確認できればいいので、%errorlevel% の値を表示して
1 を返すようにしようと思います。

0 か否かが本質的に重要だという考えが抜け落ちていました。必要以上に拘りすぎていたようです。

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 7, 2018

https://ci.appveyor.com/project/sakuraeditor/sakura/builds/20114618/job/it5jp1v8883bch0j
でまた #584 が発生しました。

原因調査のために この PR 入れたいです。
再レビューお願いします。

Copy link
Contributor

@ds14050 ds14050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ポチッとな。

@m-tmatma m-tmatma merged commit 3de8e1f into sakura-editor:master Nov 8, 2018
@m-tmatma m-tmatma deleted the feature/check-errorlevel branch November 8, 2018 12:18
@KENCHjp KENCHjp added the CI appveyor など CI 関連 【ChangeLog除外】 label Dec 5, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…rlevel

コマンドのエラー判定に if errorlevel を使うようにする
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI appveyor など CI 関連 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants