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

[Rust]C++ unix向けのexampleを修正する #197

Merged
merged 13 commits into from
Jul 24, 2022

Conversation

qwerty2501
Copy link
Contributor

内容

example/cpp/unix/ を修正してRustで生成したバイナリが動作できるようにする

関連 Issue

refs #128

その他

@Hiroshiba Hiroshiba deleted the branch VOICEVOX:rust July 23, 2022 05:03
@Hiroshiba Hiroshiba closed this Jul 23, 2022
@Hiroshiba
Copy link
Member

間違えてrustブランチを消してしまったのでcloseになってしまいました。。

@Hiroshiba Hiroshiba reopened this Jul 23, 2022
@qwerty2501 qwerty2501 force-pushed the feature/example-unix-for-rust branch from 6ba87d7 to 8b43c0a Compare July 23, 2022 08:10
@qwerty2501
Copy link
Contributor Author

サンプル動かすように修正してて気づいたのですが、 現状 onnxruntime は libonnxruntime.soとバージョンがついてるlibonnxruntime.so.1.11.1があり、デプロイはバージョンなしのほうのみを配置しているのですが、
サンプルを動かそうとしてみた限りでは現状voicevox_coreはバージョン付きの方が実行時にないとshared library読み込みに失敗するようです。
そのためdeployのfileコピー処理をバージョンつきのほうをコピーするようにする必要がりそうです。(macosもかも?)

@qwerty2501 qwerty2501 marked this pull request as ready for review July 23, 2022 08:17
@PickledChair
Copy link
Member

(macosもかも?)

macOS もです!(今ちょうどそのあたりを見ていました)

- macOS の場合:`voicevox_core-osx-universal2-cpu-{バージョン}.zip` 内の `libcore.dylib`
- ONNX Runtime v1.10.0 の共有ライブラリ(配布ページ: https://github.com/microsoft/onnxruntime/releases/tag/v1.10.0 )
- Linux の場合:`onnxruntime-linux-{お使いのCPUアーキテクチャ}-1.10.0.tgz` 内の `lib/libonnxruntime.so.1.10.0`
- macOS の場合:`onnxruntime-osx-universal2-1.10.0.tgz` 内の `lib/libonnxruntime.1.10.0.dylib`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onnxruntimeの配置はcmakeで行うようにしてあるので削除

Copy link
Member

Choose a reason for hiding this comment

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

このexampleはビルド済みcoreをダウンロードして使う想定(coreのビルド環境がなくても良い想定)でした。
なのでこのダウンロード工程と、あとcmakeの変更がいりそうです。
(気づくのが遅れてしまって申し訳ないです。。)

Copy link
Member

Choose a reason for hiding this comment

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

coreのビルド環境がなくても良い想定

すっかり失念していました……。確かに元々は C/C++ の開発環境さえあれば試せる感じに作っていました。onnxruntime の自動配置は便利だと思いつつ見ていましたが、ビルド済みcore や onnxruntime をユーザーが取ってくるようにする方が良いかもですね(親切に設計するとしても、プロジェクトのルートにある configure.py を再整備して使ってもらうようにする方が良いかもしれません)。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ダウンロードする必要がある旨記載しました

Copy link
Member

Choose a reason for hiding this comment

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

configure.pyの再整備は賛成です!rust化完了後にissue作りたいですね

qwerty2501 added a commit to qwerty2501/voicevox_core that referenced this pull request Jul 23, 2022
サンプル実装より、現在配置してるshared
libraryはバージョンなしのため実行に失敗するためバージョンつきのものに変更する必要がある
refs VOICEVOX#128 VOICEVOX#197 (comment)
Hiroshiba pushed a commit that referenced this pull request Jul 23, 2022
linux,macosのshared libraryをバージョンつきのものを配置するように変更

サンプル実装より、現在配置してるshared
libraryはバージョンなしのため実行に失敗するためバージョンつきのものに変更する必要がある
refs #128 #197 (comment)
@qwerty2501
Copy link
Contributor Author

もともとのCIにあったexampleのビルド確認を元にexampleのビルド確認を追加しました

@qwerty2501 qwerty2501 force-pushed the feature/example-unix-for-rust branch from 21e44d4 to 3f0f0d0 Compare July 23, 2022 12:38
@qwerty2501 qwerty2501 force-pushed the feature/example-unix-for-rust branch from c1c34d2 to dfdf9a9 Compare July 23, 2022 13:05
@qwerty2501
Copy link
Contributor Author

なぜかmacだと失敗するな・・・

@qwerty2501
Copy link
Contributor Author

そもそもCIの修正はこのPRの範囲ではない気もしてきましたがどうしましょうかね

@Hiroshiba
Copy link
Member

いったんmacはなぜか落ちるということでTODOにしてコメントアウトにしちゃって、いったんマージするという手もあると思います・・・!

@qwerty2501
Copy link
Contributor Author

@Hiroshiba 一旦macコメントアウトしました

@PickledChair
Copy link
Member

なぜかmacだと失敗するな・・・

手元の環境でうまくいくかどうかちょっと試してみます。もしうまくいったら、CI の方と条件がどう異なっているのかみてみます

@PickledChair
Copy link
Member

手元の macOS 環境ではビルド可能でした!(ただし実行可能にするにはもう少し作業する必要があります)。

少し調べた程度では、CI 環境でビルドが落ちる理由は謎でした。C標準ライブラリのヘッダのいくつかが見つからないというようなエラーメッセージに見えますね……。時間がかかりそうなので、macOS はコメントアウトにしたまま TODO にするというというのに私も賛成です……。

@Hiroshiba
Copy link
Member

調査ありがとうございます!!

# ./simple_tts <読み上げさせたい文章>
./simple_tts これはテストです
cp build/simple_tts ./
LD_LIBRARY_PATH=./:$LD_LIBRARY_PATH simple_tts これはテストです
Copy link
Contributor Author

Choose a reason for hiding this comment

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

実行するのに LD_LIBRARY_PATHの設定が必要になってしまったのがちょっと引っかかってます・・・

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#203 の設定を行えば LD_LIBRARY_PATHの設定は不要そう

@qwerty2501
Copy link
Contributor Author

macos含めてCI通るようになりました!やはりcmakeが原因だったようです

qwerty2501 added a commit to qwerty2501/voicevox_core that referenced this pull request Jul 23, 2022
linux,macosのshared libraryをバージョンつきのものを配置するように変更

サンプル実装より、現在配置してるshared
libraryはバージョンなしのため実行に失敗するためバージョンつきのものに変更する必要がある
refs VOICEVOX#128 VOICEVOX#197 (comment)
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!!!

すごくシンプルに仕上がった…!!

@PickledChair
Copy link
Member

PickledChair commented Jul 24, 2022

問題なさそうでした!(特に CMake 周りはかなりシンプルになって助かります……! 手元の macOS 環境でも手順の通りにサンプルを実行できることを確認しました)

一部の表記揺れを修正後、マージしたいと思います。

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.

マージします!

@PickledChair PickledChair merged commit 55de65f into VOICEVOX:rust Jul 24, 2022
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