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

残りのターゲットをビルドする #26

Merged
merged 19 commits into from
Jan 9, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 6, 2024

内容

現時点で最新であるv1.16.3で、VOICEVOX COREに必要なすべてのターゲットをビルドします。

関連 Issue

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

その他

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です!!!
とても楽しみです!!!!!!

結構ややこしくなってきちゃったのでbashスクリプトに切り出すのもありなのかなとちょっと思いました。
まあ、ご興味あれば。。。

@@ -113,8 +164,18 @@ jobs:
path: build/
key: ${{ matrix.artifact_name }}-v${{ env.ONNXRUNTIME_VERSION }}-cache-v1-${{ hashFiles('matrix.json') }}

- name: Install tree
if: steps.cache-build-result.outputs.cache-hit != 'true' && runner.os == 'Windows'
Copy link
Member

Choose a reason for hiding this comment

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

cache-build-result、このジョブのアップロード以外の全てに入ってて、あまりキャッシュの意味がない気がちょっとしました!
(まあこちらのプルリクエストには関係ないのですが)

Copy link
Member Author

Choose a reason for hiding this comment

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

このjobにおけるcacheですが、意味合いとしては「アップロード以外の全て」で正しいという理解を私はしています。というのもキャッシュ対象は「ビルドが成功した状態のbuild/全体」なので。

むしろ #24 のもこっち側に移すのがよいのではと思っています。

Copy link
Member

@Hiroshiba Hiroshiba Jan 9, 2024

Choose a reason for hiding this comment

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

なるほどです!

おっしゃることは結構わかるのですが、結局キャッシュがあった場合にできるのは「完成物を再アップロードする」だけで、それが有効なのは間違えてreleaseを削除してしまった時くらいなんですよね。
デバッグなどで不要なのをビルドしたくない時は、不要などコメントアウトした状態でチェックすれば良いので・・・。

他にメリットといえば、build-xcframeworkの後続タスクに便利だとは思います。

そのメリットと、ifを書き忘れて実行してしまったせいで変なキャッシュが出来上がって気づかずにリリースに支障をきたしてしまうのとで、リスクの大きさと天秤にかける必要があるなあと。
例えばarm linux版のビルドだけ変なファイルが入っちゃって、それに気づかずリリース+コアビルド+エンジンビルドまでしてしまって、エンジンのリリースをした後にarm版が動かないissueが経って、ここまで戻ってきてエンジン再リリース、みたいな・・・。

色々考えましたが、リスクは承知で、まあ慎重にコードを書いて行く感じで良いかなと思いました!
ミスったら笑いましょう!

それとは別に、どちらの利点も得る方法もあると思います。
例えばシェルスクリプト全部一つにまとめてしまって、ステップを1つにしてキャッシュ有無のifを1回だけにするとか。
そこを目指すissueを立てるとかが良いのかなと思いました。

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml 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.

LGTM!!!

キャッシュ周りはまあ細かいことなので良いのかなと思いました!
マージしてしまっても大丈夫そうでしょうか 👀

@qryxip
Copy link
Member Author

qryxip commented Jan 9, 2024

多分大丈夫だと思います。

@Hiroshiba Hiroshiba merged commit 3f85fdd into VOICEVOX:main Jan 9, 2024
@Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba mentioned this pull request Jan 9, 2024
3 tasks
Comment on lines +87 to +88
- artifact_name: onnxruntime-osx-arm64
os: macos-12
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Android版onnxruntimeのビルドをする?
2 participants