search

VOICEVOX / voicevox_core

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXのコア
https://voicevox.hiroshiba.jp/
MIT License
864 stars 117 forks source link

cbindgenの設定をcbindgen.tomlに移行し、CLI版のcbindgenを使う #282

Closed qryxip closed 2 years ago

qryxip commented 2 years ago

内容

cbindgenにはCLI版があり、cargo install cbindgenでインストールできます。build.rsからこのCLI版cbindgenに移行することを提案します。

現在voicevox_core.hはcargo build -p voicevox_core_c_api --features generate-c-headerという形で生成されています。 (#219) これはもともとcargo buildするたびに毎回生成していたのに制限を加えた形ですが、結果として見た目的に「build.rsの中にあるcbindgenを起動するためだけにわざわざクレートごとcargo buildしている」という状態になっています。

Pros 良くなる点

Cons 悪くなる点

実現方法

OSの種類/ディストリ/バージョン

GitHub Actionsで走らせているプラットフォーム全部

その他

CI/CD時間が増えるか減るかはわかりません。feature違いによる依存の再ビルドは減るでしょうし、cargo installの代わりにcargo-binstallを使えばバイナリを持ってこれるため、予想としては減るだろうと思っています。

qwerty2501 commented 2 years ago

build.rsの中にあるcbindgenを起動するためだけにわざわざクレートごとcargo buildしている

CI上ではC ヘッダーを出力するときはビルドあるいはテストが必要な場合なのでそこまで問題にはならないと考えてます。 また提案だとローカルでCヘッダー出力を確認したい場合にCLI版cbindgenをインストールを明示的に行う必要があるため開発参加障壁がやや上がるのがデメリットになるのではないかと考えてます。現状だとbuild-dependenciesとして自動で持ってこれるため。

qryxip commented 2 years ago

また提案だとローカルでCヘッダー出力を確認したい場合にCLI版cbindgenをインストールを明示的に行う必要があるため開発参加障壁がやや上がるのがデメリットになるのではないかと考えてます。現状だとdev-dependenciesとして自動で持ってこれるため。

その観点だったらcargo-xtask的な仕組みでcargo xtask generate-c-headerという手もあるかなと思いました。こっちなら実装的にも今のbuild.rsの内容を移すだけでよいです。

現在ヘッダの生成についてのドキュメントは無く、書くとしたら「cargo check -p voicevox_core_c_api --features generate-c-headerしたらtarget/voicevox_core.hができるのでそれをコピーしてください」となると思うのですが、それなら「cargo xtask generate-c-headerしてください」(あるいは「cbindgen -c voicevox_core_c_apiしてください」)の方が自然かなと思ったのが動機です。

取り回し的にも

❯ cargo check -p voicevox_core_c_api --features generated-c-header && less ./target/voicevox_core.h
❯ cargo check -p voicevox_core_c_api --features generated-c-header && cp ./target/voicevox_core.h ./example/cpp/unix/

よりも

❯ cargo xtask generate-c-header | less
❯ cargo xtask generate-c-header -o ./example/cpp/unix/voicevox_core.h

の方がシンプルかなと。

qryxip commented 2 years ago

--features generate-c-headerをやめる話とは別に、cbindgenのドキュメントではどうもcbindgen.tomlを使うのが推奨されているので、その点で移行した方がよいんじゃないかとも思ってます。

qwerty2501 commented 2 years ago

現在のコマンドが煩雑であるのはたしかにそうですね。しかし cbindgen.tomlを推奨されているというのはどのあたりをみてそう感じられたのでしょうか？

qryxip commented 2 years ago

cbindgen.tomlを使わずに設定を書いているサンプルコードがcbindgenのリポジトリに見当たらないことからそう思いました。

qryxip commented 2 years ago

一応cbindgen.tomlを使わない方法も示されているのですが、その方法でもConfig { .. }の形ではなくBuilderを使うもので、かつBuilderにはメソッドのdocstringが欠けているような状態です。 https://github.com/eqrion/cbindgen/blob/master/docs.md#buildrs

qwerty2501 commented 2 years ago

なるほどそういうことならcbindgen.tomlで管理しても良いかもですね

qryxip commented 2 years ago

ではまずcbindgen.tomlへの移行だけでPRを出そうと思います。