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

C API版のライブラリとヘッダファイル名をcore -> voicevox_coreに変更 #256

Merged
merged 5 commits into from
Sep 13, 2022

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented Aug 28, 2022

内容

rust 版を作成する際にライブラリファイルとヘッダファイルの名称を既存のものと合わせて作っていたが、それから事情も変わって今なら変えれそうなタイミングかなと判断したのでやってみました

変えれそうかなと判断したのは
engineが既存のC APIのものからpython API版を使う流れになりそうになったので、C API版のファイル名称を変更しても影響は少なそうだと判断したためです

Pros

  • file名に voicevox を prefixにいれることによりvoicevox関連のファイルであることを表明できる
  • core のみだと一般的な名称であるためユーザー側に同名ファイルがあって取り違える可能性もゼロではないのでprefixをつけることによりそれを避けることができる

Cons

  • 既存 core ライブラリユーザーが参照するファイル名を変更する手間が発生する(既存ユーザーは恐らく少ない上、次のリリースでどのみち大幅な変更がはいるのでやるなら今な気がします。むしろここでやらないとcore名称に依存したライブラリ参照したユーザーが増え続けてcoreから名称を変えることができない自体にずるずると陥っていきそうです)

@Hiroshiba
Copy link
Member

ファイル名変更、良いと思います!!

@qwerty2501 qwerty2501 marked this pull request as ready for review September 12, 2022 13:24
@qwerty2501
Copy link
Contributor Author

review可能になりました

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.

LGTM

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

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

以下の行の libcore.dyliblibvoicevox_core.dylib への変更をお願いしたいです……!

println!("cargo:rustc-link-arg=-Wl,-install_name,@rpath/libcore.dylib");

@qwerty2501
Copy link
Contributor Author

@PickledChair 忘れてました。修正しました。

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

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

LGTM! 大丈夫だと思うのでマージします

@PickledChair PickledChair merged commit d865b5a into VOICEVOX:main Sep 13, 2022
@qwerty2501 qwerty2501 deleted the feature/rename-library-name branch September 13, 2022 17:58
@qwerty2501
Copy link
Contributor Author

@Hiroshiba @PickledChair engine側でfile名が変わったことによる対応が必要になると思います。
互換性保つ目的なら voicevox_core.{dll,so,dylib} を core.{dll,so,dylib} にrenameする必要がありそうです

@PickledChair
Copy link
Member

@qwerty2501

@Hiroshiba @PickledChair engine側でfile名が変わったことによる対応が必要になると思います。

互換性保つ目的なら voicevox_core.{dll,so,dylib} を core.{dll,so,dylib} にrenameする必要がありそうです

ご指摘ありがとうございます。rename するかどうかは少し迷いますね……。

エンジン側では新しいコアの名前に対応すること自体はそれほど大変ではないと感じました(例えばまず voicevox_core.{dll,so,dylib} を探し、あればそれを読む。なければ core.{dll,so,dylib} を読む、という仕様にしてしまうなど)。なので rename するかどうかは互換性を(コアの名前まで含めて)気にするかどうかのみに依ると思います。

rename する場合は、そのタイミングをコアの配布前にするか、エンジンのビルド時にするか、といった選択肢があると思いました。

@qwerty2501
Copy link
Contributor Author

engineで古いバージョンのものを読み込む場合名前が coreであるためそのあたり問題になりそうかなと思ったんですが、大丈夫そうならそのままでも問題ないと思います。

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