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

追加: 起動前にengine_manifest.jsonをチェックする #1526

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nanae772
Copy link
Contributor

内容

check_release_build.py でエンジン起動前にengine_manifest.json

  1. 存在チェック
  2. jsonとして読み込めるかチェック
  3. 適当なキー(manifest_versionとした)の存在チェック

を行うようにしました。

fork先でbuildのCIが通ることを確認しました。
https://github.com/nanae772/voicevox_engine/actions/runs/13263787224

関連 Issue

ref #1300

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

その他

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 19, 2025

プルリクエストありがとうございます!

skip_run_processはその名の通り「runの実行をスキップする」という引数なので、その中でマニフェストのチェックをするのは役割が混じってるかもです!

ということで別で引数を用意してあげるのはどうでしょうか。
例えばskip_check_manifestを用意し、falseのときだけチェックするとか!

マニフェストの存在を確認するチェックコードは現状で問題ないと思います!

@nanae772
Copy link
Contributor Author

レビューいただきありがとうございます、skip_check_manifestフラグを持たせる方針で修正いたしました。

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.

すみません1点だけ!

たぶんdockerで動かすtest-engine-containerテストの方ではマニフェストファイルがないと思うので、--skip_check_manifestを指定しないとエラーになるかもです!
たぶんここ?

run: python tools/check_release_build.py --skip_run_process --dist_dir dist/

間違ったことを言ってそうだったらご指摘いただけると 🙇

@nanae772
Copy link
Contributor Author

ご指摘ありがとうございます、全く把握しておりませんでした。
fork先で試してみようと思ったのですが、自前のDockerHubアカウントを使って動かす必要があるみたいですね。
そのあたりも分かってなかったので、もう一度確認しておきます。

@Hiroshiba
Copy link
Member

検証の提案ありがとうございます!!
Dockerアカウントの作成が難しそうとか、チェックが難しそうであれば言ってください!
マージした後正しく動かなければ修正する、とかもできると思うので・・・!

…tを追加

dockerコンテナで動かしているのでmanifestは存在しないため
@nanae772
Copy link
Contributor Author

ご親切にありがとうございます。
DockerHubの利用まではいけたのですが、build-dockerのCICDでDocker ImageのBuildに失敗してしまうようでちょっと検証するのが難しい感じです。
もしかしたら、#1525 と関連しているかもしれないので、ログなどはそちらにアップロードさせていただきます。

ただ仰るようにdockerコンテナを利用するためmanifestファイルが無いというのは、その通りだと思ったので
そちら、5b70f21 にて修正いたしました。

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.

2 participants