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]RPATHの設定をした #203

Merged
merged 2 commits into from
Jul 24, 2022

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented Jul 23, 2022

内容

#197 (comment) によりRPATHがされていないとdynamiclink時にLD_LIBRARY_PATHの設定を行わないとlink errorになってしまうため

関連 Issue

refs #128

rustflags = ["-C", "link-arg=-Wl,-rpath,$ORIGIN"]

[target.'cfg(target_os="macos")']
rustflags = ["-C", "link-arg=-Wl,-rpath,@rpath"]
Copy link
Contributor Author

@qwerty2501 qwerty2501 Jul 23, 2022

Choose a reason for hiding this comment

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

@PickledChair この設定でmacosでのidentification nameの設定が行われないか確認してもらえますか?

Copy link
Member

Choose a reason for hiding this comment

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

この設定ではうまくいかなかったのですが、

[target.'cfg(target_os="macos")']
rustflags = ["-C", "link-arg=-Wl,-rpath,@rpath"]

の代わりに

[target.'cfg(target_os="macos")']
rustflags = ["-C", "link-arg=-Wl,-install_name,@rpath/libcore.dylib"]

と記述すると、identification name の設定が行われました!

しかしこれで懸念されるのは、この .cargo/config.toml がプロジェクトルートにあることから、crates ディレクトリに他の crate が増えた時に、その crate にもこのカスタムフラグが影響を及ぼす可能性があることです。

そこで、 #200 (comment) において crates/voicevox_core.cargo/config.toml を置いてもうまくいかなかった件についてもう少し実験してみたところ、cd crates/voicevox_core してから cargo build すると identification name が意図通りに設定されることがわかりました。

上記の実験結果から読み取れるのは、「cargo はコマンドが実行されたカレントディレクトリにある .cargo/config.toml を読みに行く」ということです。

もし crates/voicevox_core crate のみにカスタムのビルドフラグを追加したい場合は、crates/voicevox_core/.cargo/config.toml を書いて、cd crates/voicevox_core してから cargo build する、ということになるのでしょうか……(この辺り、うまく上のディレクトリに設定が伝播してくれる仕組みがあれば便利なのですが、あるかどうかはまだ調べられていません……)。

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
Contributor Author

Choose a reason for hiding this comment

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

@PickledChair ちなみに、macosでrpath自体の設定ってやったほうがいいですか?それともあるとかえって良くないですか?

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.

@qwerty2501

ちなみに、macosでrpath自体の設定ってやったほうがいいですか?それともあるとかえって良くないですか?

今まで見た感じでは、特に設定しなくても rpath として @rpath/libonnxruntime.1.11.1.dylib が自動で追加されていたりしていたので、設定なしで問題ないと思っています。

うーん、ただプロジェクトルートでビルドできなくなってしまうのはちょっと避けたいかも・・・

これ過ぎますね……。

https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure に書いてある仕様を今までも見ていたのですが、今回の実験でようやく理解できました。先ほど書いた

「cargo はコマンドが実行されたカレントディレクトリにある .cargo/config.toml を読みに行く」

というのはちょっと不正確で、正しくは子ディレクトリから順番に親を辿って、見つかった .cargo/config.toml を読んでいく、という感じのようです。また、

At present, when being invoked from a workspace, Cargo does not read config files from crates within the workspace. i.e. if a workspace has two crates in it, named /projects/foo/bar/baz/mylib and /projects/foo/bar/baz/mybin, and there are Cargo configs at /projects/foo/bar/baz/mylib/.cargo/config.toml and /projects/foo/bar/baz/mybin/.cargo/config.toml, Cargo does not read those configuration files if it is invoked from the workspace root (/projects/foo/bar/baz/).

現在、ワークスペースから起動した場合、Cargo はワークスペース内のクレートから設定ファイルを読みません。例えば、ワークスペースに /projects/foo/bar/baz/mylib/projects/foo/bar/baz/mybin という二つのクレートがあって、 Cargo 設定ファイルが /project/foo/baz/mylib/.cargo/config.toml/projects/foo/bar/baz/mybin/.cargo/config.tomlにあるとすると、ワークスペースのルート (/projects/foo/bar/baz/) から起動した場合、Cargo はこれらの設定ファイルを読み込みません。

と記載されていました。

Copy link
Member

Choose a reason for hiding this comment

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

今までの調査を踏まえると、cd crates/voicevox_core してから cargo build という手順は避けられなさそうな気がします……

Copy link
Member

Choose a reason for hiding this comment

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

あるいは、workspace の使用をやめると話がシンプルになるのかもしれません。今の所 open_jtalk-rsonnxruntime-rs は別リポジトリで管理していて crates 以下には入らなさそうですし、今後も crates に他のクレートを追加する予定がなければ workspace を使わないという選択肢はありそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PickledChair build.rsつかえばcrate個別にlink-flag設定できるのでこの問題は解決しそうだったのでやりました!
またmacosについても install_name の設定を行ってます

Copy link
Member

Choose a reason for hiding this comment

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

@qwerty2501 ありがとうございます、macOS でうまく行っていることを確認しました!

@qwerty2501 qwerty2501 changed the title RPATHの設定をした [Rust]RPATHの設定をした Jul 23, 2022
Comment on lines -93 to -96
- name: Set libcore.dylib's identification name (macOS)
if: startsWith(matrix.os, 'macos')
shell: bash
run: install_name_tool -id "@rpath/libcore.dylib" "artifact/${{ env.ASSET_NAME }}/libcore.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.

build.rsで設定できるようになったので消しました

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

crates/voicevox_core/build.rs Show resolved Hide resolved
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
Copy link
Member

マージします!

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