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

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

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Nov 20, 2023

内容

Cargo.tomlのdependenciesについて、

  1. versionworkspace.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

その他

Comment on lines -602 to -606
"js-sys",
"num-traits",
"serde",
"time 0.1.45",
"wasm-bindgen",
Copy link
Member Author

Choose a reason for hiding this comment

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

chronoはC APIとJava APIで使っているが、Java APIだけ無駄にdefault featuresをオンにしていたためそれを削った結果。

@@ -4399,6 +4385,7 @@ dependencies = [
"once_cell",
"serde_json",
"tokio",
"tracing",
Copy link
Member Author

Choose a reason for hiding this comment

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

Python APIはpyo3-logのため、Java APIはandroid_loggerのためにtracing/logを有効化する必要があるため、2.の方針によりtracingを明示的に依存に入れるようにした。

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

workspaceに書いてる依存が、package側のビルドのタイミングで全部入ってくる、というわけじゃないんですね!
(poetryとかはrootの設定を.devなどが勝手に参照するはず)

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 21, 2023

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

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

@sevenc-nanashi
Copy link
Member

自分は別に大丈夫です。

@qryxip
Copy link
Member Author

qryxip commented Nov 21, 2023

この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
Copy link
Member

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

@Hiroshiba
Copy link
Member

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

@Hiroshiba Hiroshiba merged commit fb8fc4b into VOICEVOX:main Nov 21, 2023
44 checks 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.

3 participants