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

HTMLヘルプのビルドステップをpowershellに移植してエラー耐性を上げる #1509

Merged

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Jan 17, 2021

PR の目的

#1271 Azure Pipelines/GitHub Actions で HTML Help をビルドできるようにする で導入された「OS言語が英語であっても日本語ロケールでないと動作できないレガシープログラムを動かすための裏技(bat実装)」をpowershellに移植して、ビルドステップの実行中に発生するエラーに対する耐性を上げることが目的です。

この変更により appveyor の疑似パイプラインが不要となるので、パイプラインを廃止して処理構造をシンプルにします。

カテゴリ

  • ビルド関連
    • Azure Pipelines
    • AppVeyor
    • GitHub Actions
    • ローカルビルド

PR の背景

ちょっとめんどくさい変更なので、経緯を頭から説明します。

サクラエディタにはHTMLヘルプが付いています。

HTMLヘルプは、HTMLファイルを「コンパイル」して作るものです。
コンパイルには当然「コンパイラ」が必要なんですが、これがショボくて苦労しています。

HTMLヘルプコンパイラの何がショボいか?

  1. HTMLでエンコーディング指定をしても認識されません。
  2. 生成されるchmの内容が、コンパイルを実行するロケールに左右されます。

サクラエディタのヘルプは日本語で書かれていますが、
このプロジェクトで使っているCIはすべて英語環境です。

つまり、サクラエディタのHTMLヘルプをCIで普通に実行すると、文字化けしてしまうわけです。

英語ロケールで動いているCIを使って、HTMLビルドを日本語ロケールで実行する方法は色々試してきました。

  1. Set-WInSysLocale で ロケールを日本語に変える。
    この方法にはOS再起動が必要なので、選択できるのはAppveyorのみ。
    動作が不安定で、かつ、やたらと時間がかかり、たまにタイムアウトする。
  2. Locale-Emulator で hhc.exe のプロセスロケールを日本語に変える。
    この方法にはOS再起動が不要なので、どのCIでも対応できそう。
    動作は不安定だが、起動時間は通常のプログラムと同等程度に改善、ただしたまにタイムアウトする。
    Locale-Emulator で起動させた場合の hhc.exe がたまにアクセス違反を起こしていた。

今回修正では、従来バッチで実装していた hhc.exe 起動部分を powershell に移植し、
発生した例外をキャッチして続行できるようにしています。

PR のメリット

  • HTMLヘルプのビルド成功率が上がります。
  • HTMLヘルプのビルド処理がわずかに簡素化されます。

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

  • HTMLヘルプのビルド成功率を 100% にする変更ではありません。
  • 周辺処理が超複雑なのでなんかおかしな変更を残してしまっている可能性があります。

仕様・動作説明

アプリの仕様・動作に変更はありません。

ビルドステップの仕様も #1271 と殆ど変えていません。

明確に違う点

  1. バッチでは sleep の代わりに ping していました。(CIで動かなかったから)
    powershell の Start-Sleep はCIでも使えそうなのでそちらで実装しました。
  2. ログファイルをテキトーな場所にコピーしてロック解除確認する処理を、それっぽい処理で置換しました。

PR の影響範囲

  • HTMLヘルプのビルド成功率に影響する変更です。

テスト内容

  1. ローカル環境でLEProcインストール済みの場合にビルドできることを確認しました。
  2. ローカル環境でLEProcインストールなしの場合にビルドできることを確認しました。
  3. Appveyor環境のCIビルドができることを確認しました。

関連 issue, PR

#1271

参考資料

https://docs.microsoft.com/ja-jp/previous-versions/windows/desktop/htmlhelp/microsoft-html-help-downloads

@AppVeyorBot
Copy link

@sanomari
Copy link
Contributor

趣旨としては賛成です。
#1469 (comment)

一目では判断できなかったのであとで見直します。

@k-takata k-takata added the CI appveyor など CI 関連 【ChangeLog除外】 label Jan 18, 2021
@k-takata
Copy link
Member

hhc.exe の終了を直接的に待つ方法があるといいんですけどねぇ。そうすれば、ファイルがロックされているかを確認する処理も不要になるのですが。

https://github.com/sakura-editor/sakura/pull/1509/checks?check_run_id=1716951864#step:6:902

$CompileLog is missing: D:\a\sakura\sakura\temphelp\macro\Compile.Log
$CompileLog is missing: D:\a\sakura\sakura\temphelp\macro\Compile.Log
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\macro\macro.chm
$CompileLog is missing: D:\a\sakura\sakura\temphelp\plugin\Compile.Log
$CompileLog is missing: D:\a\sakura\sakura\temphelp\sakura\Compile.Log
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\sakura\sakura.chm
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\sakura\sakura.chm
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\sakura\sakura.chm
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\sakura\sakura.chm
$CompiledHelp is still locked: D:\a\sakura\sakura\temphelp\sakura\sakura.chm

missing と locked のログが毎秒出るのはちょっと邪魔のような気もしましたが、まあそのままでもいいでしょう。

元のbatでは、コンパイルが30秒以内に終了しなかった場合(=おそらくhhc.exeがクラッシュ)は1回だけやり直すようにしてありましたが、今回のスクリプトではコンパイルが成功するまで何回でも繰り返すことになるのでしょうか。

元のbatではログファイルのロックが解除されたのをもってhhc.exeの処理が終わったとみなしています。これはhhc.exeの動作を確認したところ、chmの書き込みが終わってからログの書き込みが行われていたからです。今回のスクリプトでは、ログではなくchmの方のロック確認を行っていますが、変更した理由はありますか?

@berryzplus
Copy link
Contributor Author

missing と locked のログが毎秒出るのはちょっと邪魔のような気もしましたが、まあそのままでもいいでしょう。

なにかあったときに、時間がかかってると判断する材料になるかと考えて残しました。
リトライ回数が多いと鬱陶しいですが、まぁいいか、と。

元のbatでは、コンパイルが30秒以内に終了しなかった場合(=おそらくhhc.exeがクラッシュ)は1回だけやり直すようにしてありましたが、今回のスクリプトではコンパイルが成功するまで何回でも繰り返すことになるのでしょうか。

変更は意図的ですが、確信はないです。
リトライ上限を設けると、ビルドが成功しなくても処理が進んでしまうからです。

元のbatではログファイルのロックが解除されたのをもってhhc.exeの処理が終わったとみなしています。これはhhc.exeの動作を確認したところ、chmの書き込みが終わってからログの書き込みが行われていたからです。今回のスクリプトでは、ログではなくchmの方のロック確認を行っていますが、変更した理由はありますか?

これは明確に意図があったわけではないです。

@k-takata
Copy link
Member

sakura/build-chm.bat

Lines 43 to 53 in 3e59ee3

call :BuildChm %HHP_MACRO% %CHM_MACRO% || (echo error && exit /b 1)
call :BuildChm %HHP_PLUGIN% %CHM_PLUGIN% || (echo error && exit /b 1)
call :BuildChm %HHP_SAKURA% %CHM_SAKURA% || (echo error && exit /b 1)
copy /Y %TMP_HELP%\macro\*.chm %SRC_HELP%\macro\ || (echo error && exit /b 1)
copy /Y %TMP_HELP%\plugin\*.chm %SRC_HELP%\plugin\ || (echo error && exit /b 1)
copy /Y %TMP_HELP%\sakura\*.chm %SRC_HELP%\sakura\ || (echo error && exit /b 1)
copy /Y %TMP_HELP%\macro\*.Log %SRC_HELP%\macro\ || (echo error && exit /b 1)
copy /Y %TMP_HELP%\plugin\*.Log %SRC_HELP%\plugin\ || (echo error && exit /b 1)
copy /Y %TMP_HELP%\sakura\*.Log %SRC_HELP%\sakura\ || (echo error && exit /b 1)

ここの部分でchmをコピーした後、logもコピーしていますが、chmのみのロック確認で済ませてしまうと、タイミングによってはlogのコピーで失敗する可能性があります。

@berryzplus
Copy link
Contributor Author

ここの部分でchmをコピーした後、logもコピーしていますが、chmのみのロック確認で済ませてしまうと、タイミングによってはlogのコピーで失敗する可能性があります。

了解しました。
せっかく指摘もいただいていることですし、logのロック解除を待機するように修正します。
内容的に込み入ってるので修正を詰まずforce pushしてしまいます。

変更内容はこんな感じを予定しています。

変更前

  1. ($CompileLog | Is-Missing) を待機
  2. ($CompiledHelp | Is-Missing) を待機
  3. ($CompiledHelp | Is-Locked) を待機
  4. (Copy-Chm $CompiledHelp $Destination) でコピー試行

変更後

  1. ($CompiledHelp | Is-Missing) を待機
  2. ($CompiledHelp | Is-Locked) を待機
  3. ($CompileLog | Is-Missing) を待機
  4. ($CompileLog | Is-Locked) を待機
  5. (Copy-Chm $CompiledHelp $Destination) でコピー試行

@berryzplus berryzplus marked this pull request as draft January 18, 2021 09:36
@sonarcloud
Copy link

sonarcloud bot commented Jan 18, 2021

Kudos, SonarCloud 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

@AppVeyorBot
Copy link

@berryzplus berryzplus marked this pull request as ready for review January 18, 2021 10:13
Copy link
Contributor

@sanomari sanomari left a comment

Choose a reason for hiding this comment

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

内容を把握しました。
とくにダメそうな気配はありませんでした。


# to run our custom scripts instead of automatic MSBuild
build_script:
- cmd: cup vswhere
Copy link
Contributor

Choose a reason for hiding this comment

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

何故削除?と思いましたが、上に移動してました。

pip install openpyxl --user
cinst locale-emulator -y
cup vswhere
py.exe -m pip install openpyxl --user
Copy link
Contributor

Choose a reason for hiding this comment

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

PIP実行は、こういう書き方ができるんですね。

Copy link
Member

Choose a reason for hiding this comment

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

PATH の設定にかかわらず、py.exe で実行されるデフォルトのバージョンの python に対してインストールしたい場合は、こう書くことになります。
py -3.8-64 -m pip などのようにすれば、指定したバージョンに対して pip を実行できます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

前にここで教えてもらいました 😃


artifacts:
- path: 'sakura-*-Chm.zip*'

# run only for Release
Copy link
Contributor

Choose a reason for hiding this comment

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

成果物アップロードはreleaseのみらしいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

はい、そういう設定になってます。

@@ -6,6 +6,7 @@ Makefile text eol=lf
sakura_rc.h text eol=crlf
*.rc text working-tree-encoding=utf-16le-bom eol=crlf
*.rc2 text working-tree-encoding=utf-16le-bom eol=crlf
*.ps1 text working-tree-encoding=utf-16le-bom eol=crlf
Copy link
Contributor

Choose a reason for hiding this comment

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

これの影響範囲がちょぴっとだけ気になりました。
シフトJISやUTF-8で書いたps1をコミットしようとすると怒られるようになりますよね?

別にいいですよね~と思ったのでスルーしちゃいますけど(笑)。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

従来UTF-8でコミットされていたps1がUTF-16でチェックアウトされるようになります。
失敗時はメッセージが出るので実害はないと思います。
(個人の主義に反する可能性はありますが。)

}

# コンパイル済みHTMLのコピー処理
# Azure Pipelinesがたまにコピー失敗する対策として作成
Copy link
Contributor

Choose a reason for hiding this comment

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

この関数を使ってコピーしてるのはchmだけなので、
Azure PipelinesではCompiel.Logがたまにコピー失敗するだろうってことですよね。

Compile.Logは何に使うものなんですかね。

Copy link
Member

Choose a reason for hiding this comment

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

単なる hhc.exe の実行ログです。Compile.log は artifact としてアップロードされるようになっているので、ログを見たい場合には artifact をダウンロードすることになります。

Copy link
Contributor Author

@berryzplus berryzplus Jan 19, 2021

Choose a reason for hiding this comment

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

Azure PipelinesではCompiel.Logがたまにコピー失敗するだろうってことですよね。

はい、そうです。

Compile.Logは何に使うものなんですかね。

ビルドがうまく行かなかったときの原因調査に使えるかも知れません。使えないかも知れません。
基本的には使い道のないファイルである認識です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

単なる hhc.exe の実行ログです。Compile.log は artifact としてアップロードされるようになっているので、ログを見たい場合には artifact をダウンロードすることになります。

あれ?10hours ago なのにさっき見たとき表示されていませんでした。
タイミングかなぁ・・・

@k-takata
Copy link
Member

ふと疑問に思ったのですが、今回のスクリプトで実際に hhc.exe のクラッシュをキャッチできるのでしょうか。(leproc.exe のクラッシュはキャッチできるのでしょうが。)
leproc.exe は cmd.exe を起動して即終了 → cmd.exe が hhc.exe を起動 → hhc.exe がクラッシュ

もし hhc.exe のクラッシュをキャッチできないのであれば、結局タイムアウトの30秒を待ってからリトライすることになるので、バッチファイルからの実質的な変更点はリトライ回数を増やしただけということになってしまいますが…(あ、AppVeyorの構成変更もあったか)

else
{
#kick LEProc.exe for a legacy command.
Start-Process $env:CMD_LEPROC "$env:COMSPEC /C `"$hhc $HtmlHelpProject`""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ローカルでのテスト中にここで例外が起きてリトライすることは確認できました。

Copy link
Member

Choose a reason for hiding this comment

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

それならOKです。

}
catch [System.AccessViolationException]
{
echo "LEProc.exe has crashed"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/sakura-editor/sakura/pull/1509/files#r560093658 で例外が起きるとここにきてリトライします。

@berryzplus
Copy link
Contributor Author

ふと疑問に思ったのですが、今回のスクリプトで実際に hhc.exe のクラッシュをキャッチできるのでしょうか。(leproc.exe のクラッシュはキャッチできるのでしょうが。)
leproc.exe は cmd.exe を起動して即終了 → cmd.exe が hhc.exe を起動 → hhc.exe がクラッシュ

とりあえず、発生した例外をキャッチして続行する挙動は確認しています。https://github.com/sakura-editor/sakura/pull/1509/files#r560093658
ただ、指摘されている通り例外発生元が hhc.exe であった場合にはキャッチしようがありませんね。
その辺は、今後動かしてみて問題が出たら対処していく感じでいきたいと思っています。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
なんかありそうな気もしますけどとりあえずマージしてしまいます。

@berryzplus
Copy link
Contributor Author

このPRで行った変更が消えてしまっているようです。
https://github.com/berryzplus/sakura/commits/master/appveyor.yml

おそらく、対応が必要。

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