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.

Sign up for GitHub

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

Jump to bottom

VVPPをデフォルトエンジンに指定可能にし、未インストール時にインストールするか聞くようにする #2270

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

VVPPをデフォルトエンジンに指定可能にし、未インストール時にインストールするか聞くようにする #2270

wants to merge 50 commits into from

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Sep 20, 2024 •
edited
Loading

内容

タスクの第3弾です。

.envのVITE_DEFAULT_ENGINE_INFOSの定義を変更すれば実際に起動時にVVPPをインストールするか尋ねられます。
定義は例えばこんな感じ。（executionFilePathを消し、typeとlatestUrlを足す。）

VITE_DEFAULT_ENGINE_INFOS=`[
    {
        "type": "downloadVvpp",
        "name": "VOICEVOX Engine",
        "uuid": "074fc39e-678b-4c13-8916-ffca8d505d1d",
        "executionEnabled": true,
        "executionArgs": [],
        "host": "http://127.0.0.1:50021",
        "latestUrl": "https://raw.githubusercontent.com/VOICEVOX/voicevox_blog/master/src/data/latestDefaultEngineInfos.json"
    }
]`

起動後に別のPRを実行しようとすると、vvpp版と今までのエンジンが重なって挙動が変になるはず 😇
そのときはAppData\Roaming\voicevox-devのvvpp-enginesから直接VOICEVOX Engineを消してください 🙇

エンジンのアップデート可能だとしてもまだ再インストールは求められないようになっています。
またGPU版がある場合はそちらをインストールするようになってます。

デフォルトの挙動は一切変わってないはずです。
READMEでの案内はまだ書いていません。後々に書く予定。

関連 Issue

ref #1194

その他

こんな感じで.envを書いて; でコメントアウトを切り替えるとVVPPインストール版と通常起動版を切り替えられるはず。

; ; 今のmainブランチの現状の通常版
; VITE_DEFAULT_ENGINE_INFOS=`[
;     {
;         "name": "VOICEVOX Engine",
;         "uuid": "074fc39e-678b-4c13-8916-ffca8d505d1d",
;         "executionEnabled": true,
;         "executionFilePath": "エンジンディレクトリ/run.exe",
;         "executionArgs": [],
;         "host": "http://127.0.0.1:50021"
;     }
; ]`

; ダウンロード版
VITE_DEFAULT_ENGINE_INFOS=`[
    {
        "type": "downloadVvpp",
        "name": "VOICEVOX Engine",
        "uuid": "074fc39e-678b-4c13-8916-ffca8d505d1d",
        "executionEnabled": true,
        "executionArgs": [],
        "host": "http://127.0.0.1:50021",
        "latestUrl": "https://raw.githubusercontent.com/VOICEVOX/voicevox_blog/master/src/data/latestDefaultEngineInfos.json"
    }
]`

@Hiroshiba Hiroshiba requested a review from a team as a code owner September 20, 2024 16:02
@Hiroshiba
Copy link
Member Author

実装方針メモ

  • main.tsで、.envからVVPPアプデ確認用URLを読み込む
  • Controllerで、指定されているVVPPがインストール済みかどうか確認する
  • main.tsで、なかったら開発者側に確認ダイアログを出す
    • コールバックでもいいかもだけど、将来的に別ウィンドウでの gui を出すのであればコールバックでは実現が難しくなるので、多分確認フェーズと実行フェーズは関数を分けた方がいい
  • Controllerで、VVPPをダウンロードし、インストールして使えるようにする
    • 関数を分けるか未定

@Hiroshiba Hiroshiba marked this pull request as draft September 20, 2024 16:37
@Hiroshiba Hiroshiba mentioned this pull request Sep 25, 2024
Merged
@Hiroshiba
Copy link
Member Author

エンジンがインストールされているか判定するために、アプリを起動する前にエンジン情報を2回取得する必要が出てきました。
今はキャッシュ作っていて2回取得するのがややこしかったので、リファクタクリング用のプルリクエストを出しました。

@Hiroshiba Hiroshiba mentioned this pull request Oct 6, 2024
Draft
@Hiroshiba
Copy link
Member Author

デフォルトエンジンが複数ある件の考慮

  • .envに書くデフォルトエンジンの情報ごとに、アップデート URL を持てばよさそう
    • だとすると現状voicevox_blog側にあるのとかは全部defaultEngineInfosと複数形になっているのは間違い
    • blog側も変えとかないといけない

@Hiroshiba
Copy link
Member Author

とりあえずVVPPをダウンロードしてインストールしてから起動できるようにしてみました！

一旦main.tsに書いた一連の機構はEngineControllerでやった方が良さそう。
そこを移したり、あと差分を減らしたりしてプルリクにしたいと思います！

@Hiroshiba Hiroshiba force-pushed the 指定されているVVPPがなかったら確認後にインストールし、使えるようにするようにする branch from 97d03df to f5f705d Compare October 7, 2024 17:18
@Hiroshiba
Copy link
Member Author

main.tsに書いた一連の機構をEngineControllerに移しました！

あとは環境変数からエンジンの情報を読み込むのをloadEnvEngineInfos関数にまとめてリファクタリングした部分があるので、それだけ切り出してプルリクエストにしようと思います。

@Hiroshiba Hiroshiba mentioned this pull request Oct 29, 2024
Merged
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Nov 6, 2024 •
edited
Loading

EngineInfoManagerを、「利用可能になってるエンジンの情報のマネージャー」とするとすっきりすることがわかりました！

↓こうだったものを

envEngineInfo: デフォルトエンジンを作る環境変数、ダウンロードタイプもある

VVPP ENGINE: VVPPディレクトリにある全部

default ENGINE: デフォルトエンジン、インストール済みのものだけ
  VVPP ENGINEとenvEngineInfoが必要

registered ENGINE: 登録ディレクトリにあるエンジン

additional ENGINE: VVPP ENGINE + registered ENGINE - VVPPデフォルトエンジン

all ENGINE: additional ENGINE + デフォルトエンジン

↓こうしてみました

envEngineInfo: デフォルトエンジンを作る環境変数、ダウンロードタイプもある

env ENGINE: envに書かれてる、ダウンロードタイプ以外のエンジン
  envEngineInfoが必要

VVPP ENGINE: VVPPディレクトリにある全部

registered ENGINE: 登録ディレクトリにあるエンジン

all ENGINE: env ENGINE + VVPP ENGINE + registered ENGINE

結構いい感じになったと思うので、リファクタリングプルリクエストを2つほど出してから完成になりそうです！
（パッケージ回りのリファクタリングと、EngineInfoManager周りのリファクタリング）

@Hiroshiba Hiroshiba mentioned this pull request Nov 6, 2024
Merged
@Hiroshiba Hiroshiba mentioned this pull request Nov 6, 2024
Merged
@Hiroshiba
戻したり移動したり
7c8c4f8
@Hiroshiba
移動はしない方が良さそう
f7b17d6
@Hiroshiba
FIXME要らなくなってた
32a2f2e
@Hiroshiba
ちょっとわかりやすく
bacf688
@Hiroshiba
エンジン情報の取得に関するコメントを修正
c0219f2
@Hiroshiba
エンジン情報の取得に関するコメントを修正
d8013ae
@Hiroshiba
Merge branch 'main' into EngineInfoManagerのリファクタリング
e903075
@Hiroshiba
Merge remote-tracking branch 'upstream/main' into 指定されているVVPPがなかったら確認…
b2434d6 
…後にインストールし、使えるようにするようにする
@Hiroshiba
Merge remote-tracking branch 'origin/EngineInfoManagerのリファクタリング' into…
02e758a 
… 指定されているVVPPがなかったら確認後にインストールし、使えるようにするようにする
@Hiroshiba
逆だった
687212c
@Hiroshiba
Merge branch 'main' into 指定されているVVPPがなかったら確認後にインストールし、使えるようにするようにする
40e180a
@Hiroshiba
Merge remote-tracking branch 'upstream/main' into 指定されているVVPPがなかったら確認…
973a5a5 
…後にインストールし、使えるようにするようにする
@Hiroshiba
refactor: エンジンとパッケージ情報の取得メソッドを改善し、型定義を更新
cee971a
@Hiroshiba
refactor: ダウンロード処理のエラーハンドリングを改善し、失敗時のファイル削除を追加
c054061
@Hiroshiba
refactor: getSuitableVariantをgetSuitablePackageInfoに名称変更し、関連する変数名を更新
b5756d2
@Hiroshiba
docs: エンジン周りに関するドキュメントを削除
091df5f
@Hiroshiba
refactor: デフォルトエンジンのタイプチェックを追加し、パス以外の場合にエラーをスロー
fa29982
@Hiroshiba Hiroshiba marked this pull request as ready for review November 10, 2024 18:49
@Hiroshiba
Copy link
Member Author

できたのでドラフトあけます！

@Hiroshiba Hiroshiba changed the title [WIP] .envで指定されているVVPPがなかったら確認後にインストールし、使えるようにするようにする VVPPをデフォルトエンジンに指定可能にし、未インストール時にインストールするか聞くようにする Nov 10, 2024
Hiroshiba
Hiroshiba commented Nov 18, 2024
View reviewed changes
Copy link
Member Author

@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.

@sabonerune さん
（もしよければ @sevenc-nanashi さんも）

いつも頼ってばかりですみません！！ 🙇
もしよければ眺めたり実行したりしてみて、疑問に思ったこととか設計上の不安とかあればコメントいただけると･･･ 🙇

どうにも設計に自信がなく、このまま進んで行って大丈夫なのか結構不安で 😇
.envのVITE_DEFAULT_ENGINE_INFOSを、"type": "downloadVvpp",にしてlatestUrlを追加してexecutionFilePathを消せば動くはずです。
もし試す場合はVOICEVOXのVVPPが残ってしまうので、vvpp-enginesから手動で消す必要があります。

タスクとしては↓に一覧があって、このPRでは3番目と4番目を解決してます。
#1194 (comment)

開発者が.envを変えたときにVVPPをダウンロードできるようにもなってるだけ、という感じです。
後ほど開発者向けにVVPPダウンロード機能を提供する予定です。
（READMEに書くだけの予定。.env.developを作ってそっちにお手本いろいろ書いても良いかも）
ダウンロード中画面などを追加して UX をもっと綺麗にしたら一般提供したい予定です。

sabonerune
sabonerune reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@sabonerune sabonerune left a comment

Choose a reason for hiding this comment

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

とりあえず一通り動かして変更したコードを眺めたコメントです。

設計に関しては今のところ思い浮かびませんね…

src/backend/electron/engineAndVvppController.ts
const { url, name, size } = p;

log.info(`Download ${name} from ${url}, size: ${size}`);
const res = await fetch(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

globalのfetchを使っていますがelectronのfetch(net.fetch)を使うべきだと思います。

Copy link
Member

Choose a reason for hiding this comment

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

みた感じそこまで変わらなさそう？（ https://www.electronjs.org/ja/docs/latest/tutorial/progress-bar が適用されるというわけでもなさそうだし）

src/backend/electron/engineAndVvppController.ts
Comment on lines +202 to +206
const buffer = await res.arrayBuffer();
if (failed) return; // 他のダウンロードが失敗していたら中断

const downloadPath = path.join(downloadDir, name);
await fs.promises.writeFile(downloadPath, Buffer.from(buffer));
Copy link
Contributor

Choose a reason for hiding this comment

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

1GBを超えるファイルをメモリに載せるのは流石に良くない気がします。
Streamを使うべき？

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sabonerune sabonerune sabonerune left review comments

@sevenc-nanashi sevenc-nanashi sevenc-nanashi left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hiroshiba @sevenc-nanashi @sabonerune