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

bregonig.dll のファイル内容とタイムスタンプを比較する #335

Merged

Conversation

m-tmatma
Copy link
Member

bregonig.dll のファイル内容とタイムスタンプを比較する
#332 のような不具合を防ぐ

確認手順

ビルド成功の確認

build-all.bat Win32 Release
build-all.bat x64 Release

ビルド失敗の確認 (Win32)

echo "" > installer\externals\bregonig\Win32\bregonig.dll
build-all.bat Win32 Release

ビルド失敗してエラー解消後のビルド成功の確認 (Win32)

git checkout installer\externals\bregonig\Win32\bregonig.dll
build-all.bat Win32 Release

ビルド失敗の確認 (x64)

echo "" > installer\externals\bregonig\x64\bregonig.dll
build-all.bat x64 Release

ビルド失敗してエラー解消後のビルド成功の確認 (x64)

git checkout installer\externals\bregonig\x64\bregonig.dll
build-all.bat x64 Release

@m-tmatma m-tmatma added the installer installer 関連 label Aug 13, 2018
@m-tmatma m-tmatma added this to the next release milestone Aug 13, 2018
@m-tmatma
Copy link
Member Author

#332 のような不具合を防ぐ

build-installer.bat で全部行っているので build-installer.bat が間違ってるのは検出できないかも。

ただ、直接登録した bregonig.dll とアーカイブの中の bregonig.dll の不一致は検出できるので
バージョンアップしたときの更新漏れは防げるはず。

@berryzplus
Copy link
Contributor

実害は発生してなかった認識ですが、
気付かないまま何年も・・・ってなったらヒサンですね。

@berryzplus
Copy link
Contributor

1点確認です。

bregonig.dll のファイル内容とタイムスタンプを比較する

ここに引っかかりました。
何故ファイル内容だけでなくタイムスタンプも比較するんでしょうか?

「タイムスタンプが一致したら内容も一致とみなす」な比較手法はよくあります。
内容の完全一致だと時間がかかるから、というのがその理由。
ファイル内容、かつ、タイムスタンプが一致で押さえたい例外ケースが思いつきませんでした。

内容が同じならタイムスタンプが違ってても問題ないですよね?(ここが認識違い?)

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.

変更目的と変更内容は大筋で問題ないと思います。
実現方法に疑問点あるため、一旦コメントだけつけます。


@rem compare file contents
set COMPARE_RESULT=0
fc %DLL_BREGONIG_0% %DLL_BREGONIG_1% 1>nul 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

fc /B path1 path2 な気がします。 →詳しくはコマンドプロンプトで help fc

)
copy %INSTALLER_RESOURCES_BRON_DLL%\*.dll %INSTALLER_WORK%\
Copy link
Contributor

Choose a reason for hiding this comment

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

横展ってわけではないですが /B バイナリモード指定をしたほうがよさそうです。

Copy link
Member Author

Choose a reason for hiding this comment

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

全体的に抜けているので、他のバッチファイルも含めて一括で対応します。

@berryzplus
Copy link
Contributor

berryzplus commented Aug 13, 2018

無人稼働前提なのに copycopy /Y になってなかったことに今更気付くなど。
copy /Y は 上書き確認メッセージの表示を抑制するオプション で、
指定するとコピー先フォルダにコピー元と同名のファイルがあった場合に問答無用で上書きします。
デフォルトだと「上書きコピーしますか?」と聞いてくれる仕様になっています。
現状、appveyorはクリーンな状態からビルドするので問題になってこなかったんだと思います。

本題と関係ない部分なので通しちゃおうと思っていますが、
どっかで一括対応が必要だなぁと思ってます。

@m-tmatma
Copy link
Member Author

内容が同じならタイムスタンプが違ってても問題ないですよね?(ここが認識違い?)

bregonig を bron412.zip とアーカイブのままで登録している理由の一つに
オリジナルの dll のタイムスタンプを保持するのがあります。

外部提供のバイナリファイルのタイムスタンプは重要な情報なので保持したいというが理由です。

#332 では、bron412.zip に入っている DLL ではなく、bregonig.dll が直接登録されている
ファイルが使われていたので、タイムスタンプが保持されませんでした。

@m-tmatma
Copy link
Member Author

無人稼働前提なのに copy が copy /Y になってなかったことに今更気付くなど。
copy /Y は 上書き確認メッセージの表示を抑制するオプション で、
指定するとコピー先フォルダにコピー元と同名のファイルがあった場合に問答無用で上書きします。
デフォルトだと「上書きコピーしますか?」と聞いてくれる仕様になっています。
現状、appveyorはクリーンな状態からビルドするので問題になってこなかったんだと思います。

コピー前にコピー先ディレクトリが存在していたら、
ディレクトリごと削除後、新たにディレクトリを作成してからコピーするので
ローカルで実行する場合でも問題はない認識です。

@m-tmatma
Copy link
Member Author

ローカルで実行する場合でも問題はない認識です。

安全側に倒すために /Y をつけたほうがいいですが、
全体的に抜けているので他の箇所も含めてまとめて対応します。

@m-tmatma
Copy link
Member Author

コピー前にコピー先ディレクトリが存在していたら、
ディレクトリごと削除後、新たにディレクトリを作成してからコピーするので
ローカルで実行する場合でも問題はない認識です。

if exist "%INSTALLER_RESOURCES%" rmdir /s /q "%INSTALLER_RESOURCES%"
if exist "%INSTALLER_WORK%" rmdir /s /q "%INSTALLER_WORK%"
if exist "%INSTALLER_OUTPUT%" rmdir /s /q "%INSTALLER_OUTPUT%"
if exist "%INSTALLER_RESOURCES_BRON%" rmdir /s /q "%INSTALLER_RESOURCES_BRON%"

の部分です。

@m-tmatma
Copy link
Member Author

/Y と /B に関して #338 を登録しました。

@m-tmatma
Copy link
Member Author

fc に /B をつける対応は push 済みです

@berryzplus
Copy link
Contributor

外部提供のバイナリファイルのタイムスタンプは重要な情報なので保持したいというが理由です。

了解です。「内容一致 && タイムスタンプ一致」にしている理由を把握しました。

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.

LGTMです。

@m-tmatma m-tmatma merged commit 1fa6953 into sakura-editor:master Aug 14, 2018
@m-tmatma m-tmatma deleted the feature/compare-bregonig-dll-and-zip branch August 14, 2018 11:34
@ds14050 ds14050 added the installer installer 関連 label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…egonig-dll-and-zip

bregonig.dll のファイル内容とタイムスタンプを比較する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installer installer 関連
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants