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

リリースファイルとアーティファクトにプロダクト名とバージョンを追加 #537

Merged
merged 14 commits into from
Jan 10, 2023

Conversation

sarisia
Copy link
Contributor

@sarisia sarisia commented Dec 25, 2022

内容

.github/workflows/build.yml から自動でリリースされるファイルとアーティファクトにプロダクト名 (voicevox_engine) とバージョン (e.g. 0.14.0) を追加します.

PR 後のファイル名:

voicevox_engine-linux-cpu-0.14.0.7z.001
voicevox_engine-linux-cpu-0.14.0.7z.txt
voicevox_engine-windows-directml-0.14.0-preview.5.vvpp
voicevox_engine-windows-directml-0.14.0-preview.5.vvpp.txt

PR 後のアーティファクト名 (リリースファイル名 + github.sha)

voicevox_engine-linux-cpu-latest-e540de8a30f969518d3f07acaf065b512890ec68
voicevox_engine-windows-nvidia-0.14.0-sarisia.1-e540de8a30f969518d3f07acaf065b512890ec68

関連 Issue

fixes #533

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

その他

  • naming convention は VOICEVOX (Editor) に合わせています

  • ワークフロー内部では, アーティファクト名 (matrix.artifact_name) は参照されている箇所が多いので変更せず, リリースファイル, 及びアーティファクトのアップロード時にリネームする方針で実装しています

  • アーティファクト名を変更していないので, GitHub Actions のログ画面からアーティファクトをダウンロードする場合のファイル名は変更していません. FIXME が付いているので, よく利用されるのであれば別 Issue で検討する必要があるかと思います: この PR で対応しました.

    # FIXME: versioned name may be useful; but
    # actions/download-artifact and dawidd6/download-artifact do not support
    # wildcard / forward-matching yet.
    # Currently, It is good to use static artifact name for future binary test workflow.
    # https://github.com/actions/toolkit/blob/ea81280a4d48fb0308d40f8f12ae00d117f8acb9/packages/artifact/src/internal/artifact-client.ts#L147
    # https://github.com/dawidd6/action-download-artifact/blob/af92a8455a59214b7b932932f2662fdefbd78126/main.js#L113
    - name: Upload artifact
    uses: actions/upload-artifact@v3
    # env:
    # VERSIONED_ARTIFACT_NAME: |
    # ${{ format('{0}-{1}', matrix.artifact_name, (env.VOICEVOX_ENGINE_VERSION != 'latest' && env.VOICEVOX_ENGINE_VERSION) || github.sha) }}
    with:
    name: ${{ matrix.artifact_name }}
    path: dist/run/

  • release-test.yml にて, windows-directml のテストが漏れていましたので, 追加しています.

  • フォークでテストした際のリリースファイル名, アーティファクト名はこちらで確認いただけます:

    https://github.com/sarisia/voicevox_engine/releases/tag/0.14.0-sarisia.1

    https://github.com/sarisia/voicevox_engine/actions/runs/3783184456

@github-actions
Copy link

github-actions bot commented Dec 25, 2022

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/model.py 154 7 coverage-95%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetLoader.py 34 1 coverage-97%
voicevox_engine/preset/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 206 166 coverage-19%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 57 49 coverage-14%
voicevox_engine/synthesis_engine/synthesis_engine.py 133 12 coverage-91%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 129 6 coverage-95%
voicevox_engine/utility/init.py 3 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
TOTAL 1237 262 coverage-79%

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

エディタ側に対応が必要そうなので、issueを立ててみました!
VOICEVOX/voicevox#1066

また、このエンジンをダウンロードしているサードパーティアプリなどに案内が必要です。
こちらは0.14アプデ時にアナウンスしつつ、release noteでも案内したいと思います。

@Hiroshiba
Copy link
Member

@sevenc-nanashi さん、vvpp周りの名称問題なさそうでしょうか 👀
(問題ないかなと思っています)

@aoirint さん、FIXMEコメントのとこの意図って覚えていらっしゃったりしますか・・・?
もしかしたら以前のときと変わってwildcard / forward-matchingが不要になっているのかもと感じました。
であればよい機会なのでFIXMEコメントを取っても良いかもと思った次第です!

@sarisia sarisia marked this pull request as draft December 25, 2022 20:25
@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

すみません、修正が必要な箇所が見つかりましたので一度ドラフトに戻させていただきました、お手数おかけします…

@sevenc-nanashi
Copy link
Member

名称は問題ないと思います!

@aoirint
Copy link
Member

aoirint commented Dec 26, 2022

@aoirint さん、FIXMEコメントのとこの意図って覚えていらっしゃったりしますか・・・?
もしかしたら以前のときと変わってwildcard / forward-matchingが不要になっているのかもと感じました。
であればよい機会なのでFIXMEコメントを取っても良いかもと思った次第です!

for future binary test workflowについては、Artifactに対してバイナリテストをする別のWorkflow YAMLを導入するケースを想定していて、build workflowからArtifactの名前を渡せず、Artifactを参照できないのではないか、という意図だったと思いますが、現在のrelease testの呼び出しのようにworkflow_callやworkflow dispatchの引数を使えばよさそうということがわかったので、解決していそうです。

他には、GitHub Actionsの仕様上、job levelのenvmatrixからtop levelのenvを参照できず、step levelでenv.VOICEVOX_ENGINE_VERSIONから可変Artifact名を作る処理を何か所も書く必要があって、メンテナンス性が悪くなることが問題だったと思いますが、ビルドの一本化 #504 によって軽減されていそうです。

また、job levelのoutputやstep levelのoutput($GITHUB_OUTPUT)を使うと、記述箇所を減らせそうです。

Artifactをversioned nameにするかどうか、は可能ならしてもいいかなと思います。
(動作テストにはコミット単位のArtifactではなくPreviewビルドが使われていると思うので、「よく利用される」わけではなさそう)
この場合、バージョン部分にはlatestが入るでしょうか...? (非リリースビルドのバージョンがlatestになるのは #288 で導入されていそうです)

@sarisia
Copy link
Contributor Author

sarisia commented Dec 26, 2022

#537 (comment)
また、job levelのoutputやstep levelのoutput($GITHUB_OUTPUT)を使うと、記述箇所を減らせそうです。

ありがとうございます! この方法, ステップが増える点と, 参照がどうしても長くなる点 (他 job からだと needs.<job_id>.steps.<step_id>.outputs.variable_name)(docs にそのような記載がなかったので気のせいかもしれません, すいません) が微妙だなあと思っていたのですが, 今後のメンテナンス性を考えると一箇所にまとめたほうが良さそうですね. 入れる方向で対応させていただきたいと思います.

#537 (comment)
Artifactをversioned nameにするかどうか、は可能ならしてもいいかなと思います。
(動作テストにはコミット単位のArtifactではなくPreviewビルドが使われていると思うので、「よく利用される」わけではなさそう)
この場合、バージョン部分にはlatestが入るでしょうか...? (非リリースビルドのバージョンがlatestになるのは #288 で導入されていそうです)

latest だけだとローカルにダウンロードしたときに結局ファイル名が重複しそうだと感じたので, (雑かもしれませんが) github.sha を使い, 以下のようにするのはどうでしょう?

voicevox_engine-windows-cpu-fa30e5940586af810042322cb201bd2883a876b8

もしくは, 一応バージョン名も入れる:

voicevox_engine-linux-cpu-0.14.0-fa30e5940586af810042322cb201bd2883a876b8
voicevox_engine-windows-directml-latest-fa30e5940586af810042322cb201bd2883a876b8

@sarisia sarisia changed the title リリースファイルにプロダクト名とバージョンを追加 リリースファイルとアーティファクトにプロダクト名とバージョンを追加 Dec 26, 2022
@sarisia sarisia requested review from Hiroshiba and removed request for aoirint and sevenc-nanashi December 26, 2022 20:44
@sarisia sarisia marked this pull request as ready for review December 26, 2022 20:44
@sarisia
Copy link
Contributor Author

sarisia commented Dec 26, 2022

修正をしましたので, お手数をおかけしますが再度レビュー頂けますでしょうか?

(GitHub の再レビュー依頼ボタンを押したらレビュワーの方2人が自動削除されてしまいました, すみません…)

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 Actions、ここまで複雑なことしないと"変数"を使えないんですね・・・。

.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!!!
修正ありがとうございました、とてもすっきりしたと思います!!

@aoirint さんもまたレビュー頂けると心強いです!!

Copy link
Member

@aoirint aoirint left a comment

Choose a reason for hiding this comment

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

LGTM!

変数名artifact_nameの変更先のtargetという名前が、Dockerのマルチステージビルドのステージを指定するtargetと被っているのが気になりますが、短くするには仕方なさそうな気がしました...

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.

リリースするvvppや7zをVOICEVOX ENGINEだとわかるようにする
4 participants