Skip to content

Rust版ダウンローダーを追加する #375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Jan 28, 2023
Merged

Rust版ダウンローダーを追加する #375

merged 28 commits into from
Jan 28, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 10, 2023 •
edited
Loading

内容

Rust版のダウンローダーを追加します。

関連 Issue

ref #367
Closes #367.

その他

qryxip
qryxip commented Jan 10, 2023
View reviewed changes
qryxip
qryxip commented Jan 13, 2023
View reviewed changes
qryxip
qryxip commented Jan 13, 2023
View reviewed changes
@qwerty2501
Copy link
Contributor

@Hiroshiba このPRがmergeされた場合スクリプト版のダウンローダーの扱いはどうしますか？

qryxip
qryxip commented Jan 14, 2023
View reviewed changes
crates/download/Cargo.toml Outdated
Comment on lines 18 to 22
octocrab = { git = "https://github.com/qryxip/octocrab.git", rev = "ad15d0b51ea3fac2f683be879fd640a4407d8a2e", default-features = false, features = ["rustls-tls", "stream"] }
once_cell.workspace = true
platforms = "3.0.2"
rayon = "1.6.1"
reqwest = { version = "0.11.13", default-features = false, features = ["rustls-tls", "stream"] }
Copy link
Member Author

@qryxip qryxip Jan 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

クロスコンパイル時のOpenSSLの用意が面倒なため、Rustlsを使ってしまいます。

@qryxip
Copy link
Member Author

qryxip commented Jan 14, 2023

reqwestを入れたおかげでCargo.lockのdiffが凄いことになってますね... (+806-55)
ダウンローダーのビルドだけプライベートの方じゃなくてこのリポジトリでやった方がいいかもしれません。

@qryxip qryxip mentioned this pull request Jan 14, 2023
Merged
@qryxip
Copy link
Member Author

qryxip commented Jan 14, 2023 •
edited
Loading

image

@qryxip qryxip marked this pull request as ready for review January 14, 2023 14:50
@qryxip
Copy link
Member Author

qryxip commented Jan 15, 2023

record.mp4

qwerty2501
qwerty2501 reviewed Jan 15, 2023
View reviewed changes
crates/download/src/main.rs Outdated
}

fn read_bytes(mut rdr: impl Read) -> io::Result<Vec<u8>> {
let mut buf = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

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

ここも各entryからfile size取れるはずなのでwith_capacityしたほうが良いですね

Copy link
Member

Choose a reason for hiding this comment

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

ちょっと文脈わかってないのですが、ダウンロード先が言ってるファイルサイズが間違って小さめに言ってきたりしたらまずいかもとちょっと思いました！
（あんま強い意見じゃないです）

@Hiroshiba
Copy link
Member

こちら、もし間に合うなら0.14に含められると面白いだろうなと思いました！！
久々の機能追加アプデで注目度も高そうなので、コアを触ってみる人が増えると嬉しいなぁと！！

@qryxip
Copy link
Member Author

qryxip commented Jan 18, 2023

できそうな気はするので時間ができたら仕上げます。

@qryxip
Copy link
Member Author

qryxip commented Jan 18, 2023 •
edited
Loading

  • 2段階目(解凍)と3段階目(ファイル展開)のbar/tickを用意する
  • これがあるため、GitHubのREST APIから取れるsizeを使ってVecのcapacityをセットする
  • プログレス部分の表示も含め動くかどうか確かめる
  • Fix stream_asset XAMPPRocky/octocrab#296 の対応を見る
    • マージされたが、まだリリースはされていない

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2023

これでどうでしょうか?

downloading.mp4

@qryxip qryxip requested a review from qwerty2501 January 27, 2023 18:09
Hiroshiba
Hiroshiba reviewed Jan 27, 2023
View reviewed changes
Hiroshiba
Hiroshiba reviewed Jan 27, 2023
View reviewed changes
crates/download/src/main.rs

info!("対象OS: {os}");
info!("対象CPUアーキテクチャ: {cpu_arch}");
info!("ダウンロードアーティファクトタイプ: {accelerator}");
Copy link
Member

@Hiroshiba Hiroshiba Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

（めちゃめちゃ細かいですが）DirectMLとかCUDAのことだったら「デバイス」かもです！

（CPU/GPUはaccelaretionMode）

Copy link
Member Author

Choose a reason for hiding this comment

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

Bash/PowerShell版由来の--acceleratorも--deviceにした方がいいでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

あ、ぜひお願いします…！！
（APIがsupported deviceなので、統一しておきたいなと！）

Hiroshiba
Hiroshiba reviewed Jan 27, 2023
View reviewed changes
Hiroshiba
Hiroshiba reviewed Jan 27, 2023
View reviewed changes
@Hiroshiba
Copy link
Member

@qwerty2501 すみませんこちら答えられてませんでした 🙇‍♂️

@Hiroshiba このPRがmergeされた場合スクリプト版のダウンローダーの扱いはどうしますか？

２つともメンテするのは大変そうなので、実行バイナリがしっかり動きそうであれば実行バイナリ版に統合するのが良いかなと思います！
なので実行バイナリ版の検証が十分できたらスクリプト版は役割を全うした形になるかなと思います。

ちなみに0.14リリースが近く、その検証が間に合わないかもと感じています。
その場合はどちらもリリースしちゃって、検証後にアップデートできればと思っています！
（メンテナ判断のお気持ち共有です @y-chan ）

Hiroshiba
Hiroshiba approved these changes Jan 27, 2023
View reviewed changes
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！！

aarch64とarm64→arm64だけお願いできればという感じです！！
作成ありがとうございました！！

@qwerty2501 qwerty2501 merged commit dc53546 into VOICEVOX:main Jan 28, 2023
This was referenced Jan 28, 2023
Merged
Merged
@PickledChair PickledChair mentioned this pull request Mar 8, 2023
Closed
@qryxip qryxip mentioned this pull request Mar 26, 2023
Merged
@Hiroshiba Hiroshiba mentioned this pull request Sep 9, 2023
Merged
@qryxip qryxip mentioned this pull request Aug 6, 2025
Merged
qryxip added a commit that referenced this pull request Aug 6, 2025
@qryxip
fix(downloader): --devicesのhelpの"(cudaはlinuxのみ)"を削除 (#1124)
e999442 
`--devices <DEVICES>...`のhelpにはRust版ダウンローダー誕生 (#375)の時点
から、PowerShell版から引き継ぐ形で「(cudaはlinuxのみ)」と書かれていた。
しかしこれはRust版、というよりPowerShell版の誕生 (#249)の時点で正しくな
かった。
@Copilot Copilot AI mentioned this pull request Aug 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

@qwerty2501 qwerty2501 qwerty2501 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qryxip @qwerty2501 @Hiroshiba