Skip to content

Conversation

@qryxip
Copy link
Member

@qryxip qryxip commented May 2, 2025

内容

関連 Issue

Resolves #1668

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

その他

@qryxip qryxip force-pushed the pr/build-use-pyinstaller-package-for-docker branch from 7dfc341 to dac6673 Compare May 2, 2025 10:34
@qryxip qryxip force-pushed the pr/build-use-pyinstaller-package-for-docker branch from dac6673 to f777833 Compare May 2, 2025 15:03
Copy link
Member Author

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

f777833 (#1669): download-engine-envを仕上げました。

Dockerfile Outdated
ARG VOICEVOX_ENGINE_VERSION=latest
ARG USE_GPU=false

RUN --mount=type=secret,id=gh-token,env=GH_TOKEN <<EOF
Copy link
Member Author

@qryxip qryxip May 2, 2025

Choose a reason for hiding this comment

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

📝 シェルスクリプトの書き方については、

  • インデントは4
  • (実質的に)定数となる変数は大文字

というところだけ合わせておく。

@qryxip
Copy link
Member Author

qryxip commented May 2, 2025

8ae5a76 (#1669): docker run -p 127.0.0.1:50021:50021までできるようにしました。

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.

まだ完成じゃないかもですが気になるポイントをコメントしてみました!
かなり良い感じな気がしました!!!

判断メモ:
Github Runner側でパッケージをダウンロードして、dockerfileには COPY するだけという手もありそう。
でも今までは結構dockerfileの中でダウンロードしたりしていたので、そっちに合わせる手もありそう。
どちらが良いか今のところわからないので、個人的には一旦どっちでも良さそう!!

@Hiroshiba Hiroshiba requested a review from aoirint May 2, 2025 16:17
@Hiroshiba
Copy link
Member

現状draftですが、もしよかったら @aoirint さんからも気になる点があったらコメントいただけると・・・!! 🙏
(「こういう設計の方がいいんじゃないか」みたいなのって人それぞれで、いろんな人の視点を入れた方が良いものが出来上がる)

@qryxip
Copy link
Member Author

qryxip commented May 3, 2025

fd5ad27 (#1669): Docker内のTARGETPLATFORMはどうやらlinux/arm64/v8ではなくlinux/arm64になるっぽいので訂正。
74196e1 (#1669): GHA側をやりました。

これにより、

  1. Ubuntu 20.04を爆破する (Dockerのベースイメージにubuntu:20.04が使われている箇所への対応 #1559)
  2. VOICEVOX_ENGINE_VERSIONとしてlatestを受け入れるのをやめる

のどちらかをやればdraftを外せる状態になると思います。


ちなみにUbuntu 22.04のdocker buildまでなら動くことを確認しました。
https://github.com/qryxip/voicevox_engine/actions/runs/14806840981

@Hiroshiba
Copy link
Member

@aoirint さんのキャッシュに関してのコメントを見て、20.04を爆破するのも良さそうですが、その場合でもlatestは辞めたほうが良さそうだなと思いました!!
とりあえず20.04をどうするかは置いといて、2のlatestを辞める方向で進めるというのはどうでしょうか?

@qryxip
Copy link
Member Author

qryxip commented May 3, 2025

cce8419 (#1669)
10a6205 (#1669): latestの対応をやめました。

@qryxip qryxip force-pushed the pr/build-use-pyinstaller-package-for-docker branch from cce8419 to 10a6205 Compare May 3, 2025 10:20
@qryxip qryxip marked this pull request as ready for review May 3, 2025 10:34
@qryxip qryxip requested a review from a team as a code owner May 3, 2025 10:34
@qryxip qryxip requested review from Hiroshiba and removed request for a team May 3, 2025 10:34
Copy link
Member Author

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

6a042f9 (#1669): README.mdとCONTRIBUTING.mdを更新

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です!!

@aoirint さん的に想像と違ってるとことかあればぜひコメントいただければという感じです!!
(あと #1662 の兼ね合いとかでもコメントあれば!!)


env:
IMAGE_NAME: ${{ vars.DOCKERHUB_USERNAME }}/voicevox_engine
VOICEVOX_RESOURCE_VERSION: "0.23.0"
Copy link
Member

Choose a reason for hiding this comment

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

(ただのメモコメントです)

あ、VOICEVOX_RESOURCE_VERSIONは本当はなくせそうな気がしますね!!
packageのなかにあるはずなので・・・・・・と思ったら入ってなさそう!!!
じゃあ今の形が良さそう!!

@qryxip qryxip requested a review from Hiroshiba May 18, 2025 16:01
@qryxip qryxip requested review from Hiroshiba and removed request for Hiroshiba May 21, 2025 01:50
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!!!

実装ありがとうございました!!!
レビューしましたが、正直全く問題ないのかどうか分かっていません・・・!
あとは実際にビルドして動かしてみて問題があるか確認かなと思います!

@aoirint さんも、見てくださってありがとうございました!
このプルリクエストはマージさせていただきますが、また気づいたことがあったら是非コメントいただければ・・・!!

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.

パッケージ用にビルドしたものをdockerコンテナ内で動かすビルドフローにしたい

4 participants