search

VOICEVOX / voicevox_core

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

クレートのversionはworkspace設定に、featureはpackage設定に #688

Closed qryxip closed 11 months ago

qryxip commented 11 months ago

内容

Cargo.tomlのdependenciesについて、

  1. versionはworkspace.dependenciesに
  2. featuresはpackageのdependencies系に

書くようにします。

1.についてはrust-lang/cargo自体がやっています。

2.については、次のコマンドが通るようにfeaturesの補足も行いました。

❯ find ./crates/ -name Cargo.toml -exec cargo clippy --manifest-path {} ';'
❯ find ./crates/ -name Cargo.toml -exec cargo clippy --manifest-path {} --all-targets ';'

関連 Issue

その他

Hiroshiba commented 11 months ago

たぶん問題ないのでマージしようかなと思ったのですが、レビュワーに @sevenc-nanashi さんが指名されていました。 こういう時って @sevenc-nanashi さんのレビューを待った方がいいでしょうか？ 👀

（@sevenc-nanashi さん、@qryxip さんどちらの意見も聞くのが良さそう） （ヒホさんがマージで良さそうと思ったらマージ、とかでも全然良いと思います 🙏 ）

sevenc-nanashi commented 11 months ago

自分は別に大丈夫です。

qryxip commented 11 months ago

このPRの場合気をつけるべきはhttps://github.com/VOICEVOX/voicevox_core/pull/688/files#r1399498252のようなものかと思っています。

これはどういうことなのかというと、featureって通常APIの存在をon/offするために使われ、「デフォルトの挙動」の変更はすべきではないとされているのですが、tracing/logはその例外になります。tracing/logをオフにしてしまうとtracingのログがlogのログへの自動変換がされなくなり、pyo3-logやandroid_loggerに何も流れなくなってしまいます。そして現状そういうことが起きてもそれに対するテストはまだ無いはずです。 (対してAPIが消える分にはコンパイルが通らなくなるだけなので問題無い)

そういう観点のレビューを頂けるなら待った方がよいのかなと思うのですが、このあたりRustのエコシステムの土地勘が必要でもあると思うので、そこを要求するのはという気もしています。なので方針自体の合意ができればそれでいいのかなと思っています。

Hiroshiba commented 11 months ago

なるほどです！！！ 完全に理解したわけではないのですが、気をつけないといけないということはなんとなく把握しました。 issue作っても良いかも？

Hiroshiba commented 11 months ago

問題ないと思うのでマージします！！