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

Fix/expand externals #681

Closed

Conversation

KageShiron
Copy link
Member

@KageShiron KageShiron commented Dec 5, 2018

解決: #81 #477
関連: #679
置き換えたい: #630

#81 で言った、展開したいというパターンのPRです。#679 #630 は取り下げて、こちらの方を推したいです。

メリットなどのまとめ

  • 外部モジュールを処理していた部分をほとんど単純なxcopyに差し替えられるため、非常にシンプルになる
    • postBuild.bat (処理がなくなったので削除)
    • zipArtifacts.bat
    • build-installer.bat
    • sakura-common.iss
  • 今後の外部モジュールの増減にも対処しやすい
  • ctags以外は更新は当面なさそう。ctagsは安定版が出るまで手作業で更新しても手間は対して変わらない。
  • バイナリのタイムスタンプはPEヘッダーに記録されているため、必要があればそれを参照すればOK
  • Win32とx64でファイルの重複がありますが、迂闊に共用するとわかりにくくなるだけと考えています

(確認はしたはずですが、インストーラやzipの中身が正しいか再チェックお願いします 🙏 )

@KageShiron KageShiron added the installer installer 関連 label Dec 5, 2018
@ds14050
Copy link
Contributor

ds14050 commented Dec 5, 2018

取り下げたいのは #679 で合っていますか?

@m-tmatma
Copy link
Member

m-tmatma commented Dec 5, 2018

* バイナリのタイムスタンプはPEヘッダーに記録されているため、必要があればそれを参照すればOK

ちょっとこれには同意しかねます。PEヘッダーは普通のユーザーは参照できません。(やり方を知りません)
普通のユーザーが簡単に識別できるために、タイムスタンプを保存するようにしています。

PEヘッダーを自由に確認できるユーザーを想定しているわけではないです。

@berryzplus
Copy link
Contributor

postBuild.batは削れないと思います。

これ削ると「必ずバッチを使ってビルドする」という前提になっちゃうのでビミョーです。
(いまはvc++のpostbuildでこのバッチを呼んでるからslnを普通に開いてビルドすればexeまではできます。)

@ds14050
Copy link
Contributor

ds14050 commented Dec 6, 2018

postBuild.batは削れないと思います。

これ削ると「必ずバッチを使ってビルドする」という前提になっちゃうのでビミョーです。

@berryzplus さん、postBuild.bat の目的はなんですか? #679 (comment) において berryzplus さんが書きました。

postBuild.bat で行われているがエディタのビルドとの関連が不明な bregonig.dll などの解凍処理も不要になります。

ビルド後すぐにテストで正規表現を使えるようにしたい、という趣旨で入れた解凍処理です。

インストーラの作成処理が postBuild.bat に依存することによりソリューションを使わない MinGW ビルドで例外処理が必要になることをそれへの返信として書きました。

しかしインストーラの作成でも postBuild.bat の生成物を利用しているので MinGW ビルドでインストーラを作成しようとするときにこの依存関係の不適切さが顕在化します。

postBuild.bat はビルドの必須手順ではないので「必ずバッチを使ってビルドする」ことになるというのは正しくなく、インストーラ作成における postBuild.bat への依存はむしろソリューションを使ったビルドを必須にしています。


最初は、今後必要になるかもしれないから postBuild.bat の中身を空にするだけにとどめておいて枠組みは維持してほしい、という意見だと予想しましたが違いましたね。

@KageShiron
Copy link
Member Author

KageShiron commented Dec 6, 2018

取り下げたいのは #679 で合っていますか?

コピペミスでした><。正しくは #630 です。

postBuild.batは削れないと思います。これ削ると「必ずバッチを使ってビルドする」という前提になっちゃうのでビミョーです。

外部モジュールのコピーは「インストーラビルド」の処理なので、それをbatで行ってる以上ここかなとは思いました。こだわりは無いので別の箇所でも構いませんが、pre-build.batに書いても良さそうです。

普通のユーザーが簡単に識別できるために、タイムスタンプを保存するようにしています。

うーん、そこまで重視するユーザーはいるんですかね?必要ならPEヘッダなり別ファイルをもとに書き換えてもいいんですが。

@ds14050
Copy link
Contributor

ds14050 commented Dec 6, 2018

postBuild.bat についてちょっと早とちりだったかも。

単純に postBuild.bat のちょっとした手間を省くための機能を維持するために copy コマンドを2つ残してほしいということだったのでしょうか > @berryzplus さん

@berryzplus
Copy link
Contributor

単純に postBuild.bat のちょっとした手間を省くための機能を維持するために copy コマンドを2つ残してほしいということだったのでしょうか > @berryzplus さん

たぶんyesです。

postBuild.batの存在目的は、ビルドの後処理を行うことです。
「ビルド」は「cppをコンパイルしてデバッグできる状態にする工程」を指します。
「デバッグできる状態」にbregonig.dllのコピーは残したいので、postbuild.batは削れないと書きました。(少なくともコピーは残してほしいです。

外部資材をzipで含めている理由は、版管理のわずらわしさを軽減する目的だった気がします。
個別ファイルにバラして格納すると、ファイル単位で版元の更新を追わないといけなくなります。
zip単位なら、版元のバージョンアップのタイミングで配布物ごと差し替えればよいので管理がカンタンになるはず。

@berryzplus
Copy link
Contributor

外部モジュールのコピーは「インストーラビルド」の処理なので、それをbatで行ってる以上ここかなとは思いました。こだわりは無いので別の箇所でも構いませんが、pre-build.batに書いても良さそうです。

bregonig.dllのコピーを入れたときの話では、「インストーラ配布物を借りる」というニュアンスだったように思います。(過去PRに残ってるはずだけど探す気力がない・・・

借りるだけなのでインストーラの解凍パスと関係ない場所に展開してコピーする構造になっていたはずです。

タイムスタンプの話は・・・確かにそういう話も出たように思います。
でも、結局は版管理の便利さからzipで入れることになったような気がします。

@KageShiron
Copy link
Member Author

私が読み取った範囲では、以下のようになってるはず

  1. (postBuild.bat) installer/tempに展開
  2. (postBuild.bat) %PLATFORM%%CONFIGURATION%に実行ファイルのみをコピー
  3. (build-install.bat) .exeと.dllの指定でsakura.exeなどと一緒にコピー
  4. (build-install.bat) ライセンスファイルが足りないのでinstaller/tempからコピー
  5. (sakura-common.iss) インストーラに必要なファイルを取捨選択してインストーラに導入
  6. (zipArtifacts.bat) 必要なファイルを取捨選択してコピー

特にpostBuild.batはwin32とx64の差を吸収するためにわかりにくく、読み取りに苦労します。以下もzipの版管理よりbatが減らせるメリットのほうが大きいかなと思ってます。

  • ctags以外の更新はほとんどなさそうで、手間が生じなさそう
  • 実行ファイル+ライセンスだけなのでそれほど版管理の手間は変わらない
  • 配布物のディレクトリ構成が変わったらbatを書き直す手間がかかる

@ds14050
Copy link
Contributor

ds14050 commented Dec 7, 2018

実は自分も bregonig.dll のタイムスタンプは維持してほしい派です。

ファイル名が同じならタイムスタンプで新旧比較することが2番目の手段になります。存在するかどうかわからない プロパティ>詳細>製品バージョン(ファイルバージョン)を確認しようとはなかなか思いません。

bregonig.dll はわりとバージョンアップと機能追加があり、タイムスタンプの出番も多くなります。ユーザーが bregonig.dll の最新版をダウンロードしてきて上書きコピーしようとしたときに「古いファイルで新しいファイルを上書きしようとしています」と表示されるのは避けたい事態です。正しく対応できるユーザーは少ないでしょうし、その場合でも混乱は避けられません。

@m-tmatma
Copy link
Member

m-tmatma commented Dec 7, 2018

* 配布物のディレクトリ構成が変わったらbatを書き直す手間がかかる

確かにその場合、バッチファイルを変更する必要がありますが、
そのときにバッチファイルを書き換えたらいいと思います。

もしこの PR を適用した場合、配布物を更新するとき、かならず 手作業で
想定したフォルダ構造にコピーする必要があります。

その際、例えば 32bit 版のバイナリと 64bit版のバイナリを取り違える操作ミスが
発生するかもしれませんし、更新漏れが発生するかもしれません。

現状自動で出来てる作業をわざわざ手作業ですることを必須にするもので、
手間を増やすだけだと思います。

この PR はユーザー観点でも開発者観点でもデメリットしかないと思います。

現状のバッチファイルが複雑なので、簡単にメンテできるようにバッチファイルを
分割するなどリファクタリングする、というであれば大歓迎です。

@KageShiron
Copy link
Member Author

@ds14050

あー、dllをユーザーが差し替える場合は確かに。納得しました。

@m-tmatma
うーむ、個人的にはコミット前にダウンロード〜配置まで行ってくれるスクリプトを用意するとか、インストーラの中身を検証するテストを用意するという方向にしたくなってしまいます。。。更新もれとかはすでにappveyor.mdとかで発生してますし。

一方で、整理してバッチが十分簡潔になるならzipのままでもいいとは思います。ということで、改めて考えると以下のような感じが一番シンプルそう

  1. externalsにあるzipを無条件で全て解凍
  2. \%PLATFORM%\%CONFIGURATION%に全ての依存関係を配置
    • x84とx64で配置するファイルが違いすぎるので、いっそ別のファイルに分割する?
  3. インストーラもzipも\%PLATFORM%\%CONFIGURATION%にある中から使用するものを指定して作成
    • 現在は専用ディレクトリを作ってるのを廃止
    • common-sakura.issも、Compress-Archiveや7zipもファイル名指定可能

@m-tmatma
Copy link
Member

m-tmatma commented Dec 8, 2018

うーむ、個人的にはコミット前にダウンロード〜配置まで行ってくれるスクリプトを用意するとか、

配置を自動化するのなら、アーカイブの中身をコミットする必要はなくなります。
メリットと言えるのは、アーカイブの展開の時間が削れることぐらいしかないです。

更新もれとかはすでにappveyor.mdとかで発生してますし。

そのときはレビューで指摘していただければいいですし、
ドキュメントの更新漏れとアーカイブの更新漏れでは影響が全然違います。

@KageShiron
Copy link
Member Author

アーカイブにしろ展開後にしろ、バイナリのコミットは現状ではバージョン固定とダウンロード先が消える危険を消すために重要です。

アーカイブを展開するメリットはビルド時の流れが非常に単純でわかりやすくなることです。前のPRで私が「postBuild.batをいじったらcommon-sakura.issを変更しないといけないことに気づかなかった」といったやらかしを防げます。

といっても、このメリットもバッチの整理でどうとでもなりそうな気もします。一度zipをコミットする方針で再度用意してみたいのですが、↓の方針はどうでしょうか?
#681 (comment)

(行けそうな気がしていますが、なにか明確な理由や必然性があってインストーラやzipArtifacts専用のディレクトリにコピーしてるなら無駄PRになってしまうので予め確認)

@ds14050
Copy link
Contributor

ds14050 commented Dec 9, 2018

↓の方針はどうでしょうか?
#681 (comment)

ビルド完了と同時に配置が完了していて

  • すぐに関連 DLL が必要なテストができる。
  • sakura.exe と同じ場所から artifact に詰めるファイルが選別できる。
  • sakura.exe と同じ場所から インストーラに含めるファイルが選別できる。

ということですよね。すごくいいと思いますが何か穴があるでしょうか。

@berryzplus
Copy link
Contributor

このPRは賞味期限切れだと思うので閉じておきます。

@berryzplus berryzplus closed this Sep 5, 2020
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.

4 participants