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

マルチエンジン:VVPPに7zを使えるように #1253

Merged
merged 31 commits into from
Apr 8, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Mar 15, 2023

内容

タイトル通り。vvppに7zを使えるようにします。

関連 Issue

スクリーンショット・動画など

(なし)

その他

Mac/Linuxは未検証です。

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner March 15, 2023 09:52
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team March 15, 2023 09:52
@sevenc-nanashi sevenc-nanashi marked this pull request as draft March 15, 2023 09:52
@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review March 15, 2023 11:19
@sevenc-nanashi
Copy link
Member Author

@PickledChair
Copy link
Member

PickledChair commented Mar 15, 2023

Mac で動作確認してみましたが、vvpp を読み込めないようでした。

まず、sharevox の vvpp を一度解凍して、7z で圧縮し直したもので試してみました( 7z a sharevox_engine-macos-x64-0.2.0.vvpp sharevox_engine-macos-x64-0.2.0 しました。やり方が間違っていたらすみません……!)。その時は以下のようなダイアログが出ました:

preview-7z-error

その時のエラーログは以下です(/Users/suitcase/engines 以下に vvpp を配置しています)。一時ディレクトリのようなもの(実際に存在しているかどうかは不明)の中のファイルを開こうとしている感じでしょうか……?:

[23:12:04.674] [error] Failed to install /Users/suitcase/engines/sharevox_engine-macos-x64-0.2.0.vvpp, Error: ENOENT: no such file or directory, open '/Users/suitcase/Library/Application Support/voicevox/vvpp-engines/.tmp/1678889466823/engine_manifest.json'

また、元々の zip 版の vvpp でも試してみましたが、こちらも上手く行きませんでした。「追加中」の表示のまま次に行かなくなりました。エラーログも確認したのですがこちらはエラーの報告が出力されていませんでした……

@sevenc-nanashi
Copy link
Member Author

うーん。7zがzipファイルだけを含んだ7zアーカイブになってそう?
自分はExplzhでvvppを7zに変換しました。

@@ -222,6 +339,7 @@ export class VvppManager {
force: true,
});
log.info(`Engine ${engineId} deleted successfully.`);
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

割れ窓の修正。

@sevenc-nanashi
Copy link
Member Author

よくよく考えたら:普通のzipも7zで解凍しちゃった方がコード量も減るし速くなるので良さげ?

@PickledChair
Copy link
Member

PickledChair commented Mar 16, 2023

7z 版の vvpp に関しては、zip 解凍後の sharevox の vvpp を以下のように圧縮し直すと上手く行きました。前回上手く行かなかったのは、解凍後のディレクトリ構造が想定と違ったからのようでした。

cd sharevox_engine-macos-x64-0.2.0
7z a ../compressed.7z . -r
cd ..
mv compressed.7z sharevox_engine-macos-x64-0.2.0.vvpp

一方、なぜか元々の zip 版 vvpp の方は読み込めないままです。こちらは処理は大して変わっていないように見えるのですがなぜでしょう……。エラーログが出ていないので詳細が不明です。vvpp-engines/.tmp ディレクトリ以下にも展開中の様子が見られませんでした。Windows では こちらからダウンロードした vvpp を正常に使用できますか?

また、Linux/macOS に関しては、システムに p7zip がインストールされている想定の実装でしょうか? もしそうでしたら、どちらの OS でも基本的にデフォルトでは p7zip がインストールされていないので、通常のユーザー環境では動作しないと思いました。 失礼しました、Windows 以外でも 7z が同梱されるようになっていますね

@Hiroshiba
Copy link
Member

@sevenc-nanashi

よくよく考えたら:普通のzipも7zで解凍しちゃった方がコード量も減るし速くなるので良さげ?

zip側の依存減るし、普通にありだと思います。
7zr.exeじゃない7z.exe辺りがが必要になりそうですが、容量だけちょっと心配です。

@madosuki
Copy link
Contributor

Windowsでもzip版vvppをインストールできませんね。
試したみた感じでは、非同期で実行されるMultiStreamが終了する前にfinallyに到達しcloseが行われているぽかったのでclose処理をawait new Promise((resolve, reject))の後に行うようにしたらインストールに成功しました。

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Mar 16, 2023

7zr.exeじゃない7z.exe辺りがが必要になりそうですが、容量だけちょっと心配です。

必要でした。
7zr.exeは565KB、7za.exeは1229KBでした。

@sevenc-nanashi
Copy link
Member Author

試したみた感じでは、非同期で実行されるMultiStreamが終了する前にfinallyに到達しcloseが行われているぽかった

結局使わなくなりました。ありがとうございます

build/7za.dll Outdated Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

  • ライセンス表記周りも必要かもです
  • mac・linuxで動く周りが気になりました。
    • たぶん動くんですが、Actions上の7zコマンドをパクってくることになって、なんのバイナリかわからなくてライセンス表記できない・・・。
  • あとたぶんdllはいらない気がします
    • 不安な場合はwindows サンドボックス上にインストールしてみると動くか確かめられるかも

大いなる力には大いなる責任が伴うので、こういう辺りしっかり作らないといけないのが大変ですが、もしよかったら・・・!

@sevenc-nanashi
Copy link
Member Author

ライセンス表記周りも必要かもです

環境に合わせた7z(7za、7zz、p7zip)を落とすスクリプトを作り、
vendored/7zみたいなところに落とし、
generateLicensesにそこからライセンスを取ってこさせるようにしてみます。

(build/のスクリプトをTypeScriptにしたい…)

@sevenc-nanashi
Copy link
Member Author

レビュー反映しましたー。

Copy link
Member

Choose a reason for hiding this comment

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

vendoredディレクトリですが、なんのことか初学者の方にもわかりやすいようにthird_party辺りが良いかもと思いました。
雑に前例を探してみた感じ、pytorchがthird_partyでした。https://github.com/pytorch/pytorch

Copy link
Member Author

Choose a reason for hiding this comment

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

多分、初学者は判らなくても中のREADME.mdを見ると思います(GitHub上ではサブフォルダでもREADMEが出てくれる)

多分このフォルダの一番正しい名称はdownloaded_dependenciesだと思います

build/installer.nsh Outdated Show resolved Hide resolved
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@sevenc-nanashi
Copy link
Member Author

レビューを反映しましたー。

vite.config.ts Outdated Show resolved Hide resolved
なんで7-Zipは7z形式以外で7za.exeを配布してないんですか???????
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

いろいろ書きましたがほぼLGTMです!

@sevenc-nanashi
Copy link
Member Author

軽いドキュメント追加しましたー。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

なのですが、ちょっと全体的に煩雑度が上がっている印象があり、もしかしたらもっとスマートなアーキテクチャを組めるかもとちょっとだけ思いました。

ということでレビューお願いしたいです・・・! @y-chan
(全体で900行程度の差分ですが、lock.jsonの差分が600行なのでそこまでヘビーじゃないはず・・・!)

@Hiroshiba Hiroshiba requested a review from y-chan April 4, 2023 22:58
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

@y-chan さんお忙しそうなのと、問題ないと思うのでマージします!!

@Hiroshiba Hiroshiba merged commit 9557b1e into VOICEVOX:main Apr 8, 2023
@Hiroshiba
Copy link
Member

7zがパッケージに内包されるか試すためにreleases作ってみます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants