プリセットファイルの位置をユーザーディレクトリに変更 #1493
Conversation
takana-v
requested review from
Hiroshiba and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no suggestions.
There was a problem hiding this comment.
ほぼLGTMです！！！
presets.yamlの場所が変わるという破壊的変更になりますが、 @takana-v さんのおっしゃる通り再インストール時に消える場合が多そうな気がしています。
･･･････が、例えばサードパーティソフトウェアが利用していて、アップデートの際に気をつけている場合などは問題になる気がしますね。。。。
ちょっともしよかったらなのですが、Pythonでマイグレーションがどれぐらい心地よく書けるかのチェックも兼ねて、マイグレーションするのはどうでしょうか。
後でマイグレーションが必要になった時に自信を持ってできそうだなーと。
あまり興味なければこちらで書こうと思います！
多分やり方は簡単で、PresetManager内の処理をちょっと書き足せば終わり？
ユーザーディレクトリのプリセットがなく、エンジンディレクトリのプリセットファイルがあればデータをコピーすればよさそう。
エンジンディレクトリのプリセットファイルは消して良さそうだけど、これもエラーになっちゃいそうなので放置しても良さそう。
まあでも悪さしそうだし、tryで囲みつつrmしてみるのもよさそう。
プルリクいただいててお願いしちゃって申し訳ないです 🙇
微妙だったら結構遠慮なくおっしゃっていただければ･･･！！！
Co-authored-by: Hiroshiba <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (2)
voicevox_engine/preset/preset_manager.py:47
- The error message 'Failed to migrate preset file.' is unclear. Consider providing more context, such as 'Failed to migrate preset file from engine directory to user directory.'
warnings.warn("Failed to migrate preset file.", stacklevel=1)
voicevox_engine/preset/preset_manager.py:41
- The new migration logic for moving 'presets.yaml' from the engine directory to the user directory is not covered by tests. Ensure there is a test case that verifies this behavior.
if not self.preset_path.exists():
| @@ -350,3 +351,19 @@ def test_delete_preset_write_failure(tmp_path: Path) -> None: | |||
| preset_manager.delete_preset(1) | |||
| assert len(preset_manager.presets) == 2 | |||
| remove(preset_path) | |||
|
|
|||
|
|
|||
| def test_migrate_preset(tmp_path: Path) -> None: | |||
There was a problem hiding this comment.
テストのために（一時的ですが）エンジン直下にファイルを作るのが少し気になります。
There was a problem hiding this comment.
たしかに！！
CI環境でのみ実行でもいいかもですね！
確かActionsはenv.CIがtrueになってるはず。
条件によって実行するかどうか制御できる機構がきっとあるので、そこに渡す感じで実装できるかもです！
| old_preset_path = engine_root() / "presets.yaml" | ||
| if old_preset_path.exists(): | ||
| try: | ||
| shutil.move(old_preset_path, self.preset_path) |
There was a problem hiding this comment.
shutil.moveについて以前に言及があったので、リンクを貼っておきます。
#630 (comment)
There was a problem hiding this comment.
Copilot reviewed 6 out of 7 changed files in this pull request and generated no suggestions.
Files not reviewed (1)
- README.md: Evaluated as low risk
| @@ -350,3 +351,19 @@ def test_delete_preset_write_failure(tmp_path: Path) -> None: | |||
| preset_manager.delete_preset(1) | |||
| assert len(preset_manager.presets) == 2 | |||
| remove(preset_path) | |||
|
|
|||
|
|
|||
| def test_migrate_preset(tmp_path: Path) -> None: | |||
There was a problem hiding this comment.
他にもマイグレーションが発生するかもなので、何をテストしたいかの意図をちょっとだけ含めるとさらに良いかも。
こんな感じでしょうか。
| def test_migrate_preset(tmp_path: Path) -> None: | |
| def test_migrate_default_preset_path(tmp_path: Path) -> None: |
内容
プリセットファイル（
presets.yaml）の位置を、エンジン直下からユーザーディレクトリに変更します。これにより、全ユーザー向けにインストールした時など、エンジン直下へのファイル書き込み権限が無い時にエラーが発生することを防ぎます。
関連 Issue
close #1479
ref #1491
ref #1478
ref #1323 (comment)
スクリーンショット・動画など
その他
ユーザーが古いバージョンのVOICEVOXをアンインストールする時に
presets.yamlが削除される気がしたので、マイグレーション処理は入れていません。必要であれば追加したいと思います。