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

chore: build_optsを整理する #48

Merged
merged 4 commits into from
Aug 18, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Aug 15, 2024

内容

build_optsの整理をします。

  1. 全ビルドに共通し、かつ共通し続けるであろうオプションをまとめる
  2. YAMLの|-を使った複数行の記述方式にし、#でコメントも加えられるようにする
    • 実際にいくつかコメントを加える
  3. 統一感が出るようにオプションの順番も整える

モチベーションとしては、このリポジトリの秘伝のタレ化を少しでも防ぐためです。

関連 Issue

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

その他

@qryxip
Copy link
Member Author

qryxip commented Aug 15, 2024

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

別に想定通りも動きそうであれば問題ない部分に関してコメントしました。
まあもしよければ!

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@qryxip qryxip force-pushed the chore-refine-build-opts branch from cb2f54f to 3be316e Compare August 16, 2024 10:34
@qryxip
Copy link
Member Author

qryxip commented Aug 16, 2024

[skip ci]を使いました。今の設定だと、マージメッセージのbody部分を消すことをしないとスカッシュマージコミットに[skip ci]が伝播してしまうはずなので、ご注意願います。

@qryxip qryxip requested a review from Hiroshiba August 16, 2024 10:58
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!!

いくつかコメント書いていますが、おそらく問題ないと思うのでマージしていただければ!

skip ciに関してはissue建てます。

.github/workflows/build.yml Outdated Show resolved Hide resolved
python ./tools/ci_build/build.py --build_dir ./build ${{ matrix.build_opts }} ${{ matrix.build_opts_workaround_protoc }}

build_opts=(
${{ matrix.build_opts }}
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

matrix.build_optsはbashの配列構文?の中に入るもの、というのが名前から分からないのをどうにかできないかとちょっとだけ思いました!
でもいいのが思いつかなかった。。build_ops_bash_arrayとか・・・?
思いつかないのでこのままでも良さそう!

Copy link
Member Author

@qryxip qryxip Aug 17, 2024

Choose a reason for hiding this comment

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

正直ここのmatrix自体、名前-値のペアだけでその機能を推測するのが大体困難なので後で全部まとめてコメントを書くというのはどうでしょうか?こんな風に:
https://github.com/VOICEVOX/voicevox_core/blob/d6765462fd246d22306a7badd438d34745345231/.github/workflows/build_and_deploy.yml#L44-L51

Copy link
Member

Choose a reason for hiding this comment

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

たしかに!
1行だけで説明するのは難しいのも結構ありそうな気がしますしねぇ・・・。

とりあえず今の形でも問題ないとは思うので、一旦マージしますか!!
コメント追加はたぶんしたほうが良いと思うので、もしよければ追加PRいただければということで・・・! 🙏

.github/workflows/build.yml Show resolved Hide resolved
@Hiroshiba
Copy link
Member

マージします!!

@Hiroshiba Hiroshiba merged commit 03b3195 into VOICEVOX:main Aug 18, 2024
1 check passed
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