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

PR時に走るworkflowの数を減らす #382

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Jan 15, 2023

内容

題の通り
paths条件など、いろいろ絞ってみましたが、間違ってそうであればご指摘お願いします。

関連 Issue

@y-chan
Copy link
Member Author

y-chan commented Jan 15, 2023

かなり減ったと思うけど、それでも20個弱走っていて、test.ymlの影響範囲が結構大きいのが問題な気がしているので、これを目的に合わせて細分化したさがあります。
どうですかね...?

@Hiroshiba
Copy link
Member

細分化、良いと思います!!

Copy link
Member

@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.

paths指定はここまで入れた方がよいと思います。

正直cargo-denyとかに留めておいた方がよい気もします。

.github/workflows/build_and_deploy.yml Outdated Show resolved Hide resolved
.github/workflows/cargo-deny.yml Outdated Show resolved Hide resolved
.github/workflows/generate_document.yml Outdated Show resolved Hide resolved
@qryxip
Copy link
Member

qryxip commented Jan 15, 2023

あーでも細分化する前提ならpathsを細かくした方がいいですね。

@y-chan
Copy link
Member Author

y-chan commented Jan 15, 2023

一旦Suggestionを取り入れて、細分化していきたいと思います...!

y-chan and others added 3 commits January 16, 2023 00:00
@y-chan
Copy link
Member Author

y-chan commented Jan 15, 2023

条件に合わせてactionsを分割したのと、結合してもよさそうなものを結合してみました。
私の主観で結合したので、まずそうだったら教えてもらえるとありがたいです...!

.github/workflows/build_cpp_example.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 40 to 47
- name: Set up Rust (test only)
if: matrix.os != 'windows-2022'
uses: ./.github/actions/rust-toolchain-from-file
- uses: Swatinem/rust-cache@v2
with:
# cargoのキャッシュが原因でテストが失敗する場合はバージョン部分をカウントアップすること
key: "v2-cargo-test-cache-${{ matrix.features }}-${{ matrix.os }}"
- name: Run cargo test
shell: bash
run: cargo test -vv --features ,${{ matrix.features }}

xtask-generate-c-header:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Rust
- name: Set up Rust (test and lint)
if: matrix.os == 'windows-2022'
uses: ./.github/actions/rust-toolchain-from-file
with:
components: clippy,rustfmt
Copy link
Member

Choose a reason for hiding this comment

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

matrixに"components"を含む形でもよいのでは?
判定するときは${{ contains(matrix.components, 'clippy') }}とかにして。

Copy link
Member Author

Choose a reason for hiding this comment

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

確かに、とりあえず移植していたので思いつきませんでした...!
ちょっとやってみますね...!

Comment on lines 48 to 49
- name: Validate Cargo.lock
run: cargo metadata --locked --format-version 1 > /dev/null
Copy link
Member

Choose a reason for hiding this comment

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

別workflowに分離してもよさそうな気がします。cargo-fmtも。

Copy link
Member Author

Choose a reason for hiding this comment

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

ジョブの数が多いと並行ジョブ数上限に引っかかって、かえってCIにかかる時間が増えそうだったので、まとめられそうなものをまとめてみましたが、分ける方がメリットが大きそうでしょうか...?
いろんな視点からの意見が知りたいです:pray:

Copy link
Member

Choose a reason for hiding this comment

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

ちゃんと追えてないのですが、Actions起動のオーバーヘッドもあるので、対象のpath範囲が同じならまとめちゃった方が早いのかなと思いました!

y-chan and others added 2 commits January 16, 2023 13:27
Co-authored-by: Ryo Yamashita <qryxip@gmail.com>
Co-authored-by: Ryo Yamashita <qryxip@gmail.com>
@Hiroshiba
Copy link
Member

こちら、そろそろマージできると嬉しいかもです!!

@qryxip
Copy link
Member

qryxip commented Mar 3, 2023

そういえばpathsですが、download_testのようにworkflow file自体を含めないといけない気がします。

@Hiroshiba
Copy link
Member

Hiroshiba commented May 13, 2023

@y-chan すみません、こちらどうでしょう・・・!
コアのテストに相当な時間がかかってしまっており。。。

@y-chan
Copy link
Member Author

y-chan commented May 18, 2023

ぱぱっとやっちゃいたいのですが、ちょっとActionsの書き方を調べるとかで時間がかかりそうなので、 @qryxip さんか @Hiroshiba さん、巻き取っていただけると大変助かります...!いかがでしょうか...?
(Allow edits and access to secrets by maintainersにチェックを入れているので、このPRをそのままいじっていただくのもいいですし、一旦closeして作り直してもらうでも大丈夫です)

巻取りが難しい感じでしたら、私の方で頑張ってみます...!

@Hiroshiba
Copy link
Member

なるほどです! 興味ありますが、ちょっといろんなタスクがあるので着手がかなり遅れる気がします・・・ 🙇‍♂️

@PickledChair
Copy link
Member

@y-chan @Hiroshiba こちら、もし良かったら作業引き継ぎますか?(作業を引き継いだことがないので、もしかしたらやり方を質問するかもしれませんが……)

@Hiroshiba
Copy link
Member

@PickledChair ぜひぜひ!!

@PickledChair
Copy link
Member

PickledChair commented May 20, 2023

@PickledChair ぜひぜひ!!

@Hiroshiba わかりました! とりあえず conflict を解消しました。もう少し内容を確認して、どんな作業をすべきか整理したいと思います。

Copy link
Member

@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.

とりあえずいくつか気になった点を:

branches:
- "*"
- "**/*"
branches:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
branches:
branches:

branches:
- "*"
- "**/*"
branches:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
branches:
branches:

Copy link
Member

Choose a reason for hiding this comment

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

build_cpp_example.ymlに分離したbuild-unix-cpp-exampleがまだ残っています。

Copy link
Member

Choose a reason for hiding this comment

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

指摘ありがとうございます!

昨日の時点で気づいてはいて、ローカルでは直しています。あとで他の変更と一緒にまとめてpushするつもりでした

Comment on lines +16 to +17
paths:
- build_util/**
Copy link
Member

Choose a reason for hiding this comment

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

参考: #345

Suggested change
paths:
- build_util/**
paths:
- .github/workflows/build_and_deploy.yml
- build_util/**

Comment on lines +7 to +8
paths:
- crates/voicevox_core**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paths:
- crates/voicevox_core**
paths:
- .github/workflows/build_cpp_example.yml
- crates/voicevox_core**

Comment on lines +7 to +9
paths:
- '**/Cargo.*'
- deny.toml
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paths:
- '**/Cargo.*'
- deny.toml
paths:
- '**/Cargo.*'
- .github/workflows/cargo-deny.yml
- deny.toml

Comment on lines +8 to +9
- 'Cargo.*'
- crates/voicevox_core**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 'Cargo.*'
- crates/voicevox_core**
- 'Cargo.*'
- .github/workflows/generate_document.yml
- crates/voicevox_core**

@PickledChair
Copy link
Member

@qryxip いくつかのコメントありがとうございます! 参考にさせていただきます。
もう少し全体をよく見てから変更・修正したいと思うので少しお待ちください……!

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.

プルリクやコミット時に回るテストの数を減らしておきたい
4 participants