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

Linux arm64へのビルドを追加 #13

Merged
merged 5 commits into from
Dec 24, 2022

Conversation

haru3me
Copy link

@haru3me haru3me commented Dec 22, 2022

arm64向けのビルドを追加します.

aarch64ビルドの場合ポインタの扱いが異なる場合があるようで,場合分けのコードを追加しています.

@qryxip
Copy link
Member

qryxip commented Dec 22, 2022

aarch64ビルドの場合ポインタの扱いが異なる場合があるようで,場合分けのコードを追加しています.

こちらについてですが、Cのcharにあたる型(c_char)のことでしょうか?
そうであるなら関数単位でコードを重複させなくても、i8c_charに置き換えるだけで解決しそうな気がします。
(onnxruntime-rsのオリジナルの作者であるnbigaouette氏がc_charを使わずi8と直接書いている理由が不明ではあるんですが)

参考: https://github.com/rust-lang/rust/blob/1.66.0/library/core/src/ffi/mod.rs#L104-L157

@qryxip
Copy link
Member

qryxip commented Dec 22, 2022

onnxruntime-rsのオリジナルの作者であるnbigaouette氏がc_charを使わずi8と直接書いている理由が不明ではあるんですが

フォーク元の方にぶら下がっている nbigaouette#79 でも普通にc_charに置き換えてますし、特に理由があるわけでもないみたいですね。

@haru3me
Copy link
Author

haru3me commented Dec 22, 2022

ありがとうございます。

c_charにする事も検討したのですが、影響範囲が大きすぎるのでひとまず場合分けで対処していました。

提示していただいたフォーク元のコミットをchery-pickで持ってこれそうに感じているので、試してみようと思います。ダメでした

参考にしながら修正しようと思います.

@qryxip
Copy link
Member

qryxip commented Dec 22, 2022

唯一変えては駄目な場所はここくらいで、(onnxruntimeクレートにある)それ以外のi8は全部置換しちゃっていいと思います。

impl_type_trait!(i8, Int8);

@qryxip
Copy link
Member

qryxip commented Dec 22, 2022

提示していただいたフォーク元のコミットをchery-pickで持ってこれそうに感じているので、試してみようと思います。ダメでした

参考にしながら修正しようと思います.

必要のないアドバイスかもしれませんが、nbigaouette#79 のうちnbigaouette/onnxruntime-rs@c235ccb (#79) とかは除外し、残りのコミットを見て考えた方がいいかもしれません。
PR全体のdiffを見たとき、例えばcustom_loggerからextern_system_fn!が剥がされてますがこれは必要なはずなので。

@haru3me
Copy link
Author

haru3me commented Dec 23, 2022

ありがとうございます。

必要最小限の変更に留めようと考えていますので、その辺りの変更は取り入れないでおこうと思います。

masterベースで新しく作成したのですが、如何でしょうか
大丈夫そうならPR出そうと思うのですが

https://github.com/haru3me/onnxruntime-rs/tree/c_char-aarch64-linux

取り込んだvoicevox_coreは正常にビルドできていますのでc_charに変更した影響は大きくないのかなと考えています
よく考えたら他archのbindingsも更新しないと正しく評価出来ないかもしれませんね.
再度評価してみます

通りました.問題無さそうです
https://github.com/haru3me/voicevox_core/actions/runs/3762753493/jobs/6395738066

@qryxip
Copy link
Member

qryxip commented Dec 23, 2022

よいと思います。
ただ新しくPRを作るのもいいのですが、次のどちらかでもいいと思いました。

  1. このPRをc_char-aarch64-linux (haru3me@c74a02b) のものに置き換えてforce pushする
  2. このPRにある4コミットを全部git revert -m 1し、その上にc_char-aarch64-linuxの4コミットをcherry-pickする (このorganizationではPRは基本squash and mergeする方針なので、PR上のコミットはそれほど綺麗である必要が無いため)

まあただブランチの根本から作り直す形なので、PRを新しく作るかどうかは@haru3meさんに任せたいと思います。

@haru3me
Copy link
Author

haru3me commented Dec 23, 2022

ありがとうございます.
squashされるとの事なので,1の方法でファイルのみ置き換えてcommit&pushしました.
内容が矛盾するような変更は無いのでforceはつけていません(理解が間違っていたらごめんなさい)

よろしくお願いします.

@qryxip
Copy link
Member

qryxip commented Dec 23, 2022

ファイルのみ置き換えて

なるほど。その方法でもいいと思います。

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

@qwerty2501 qwerty2501 left a comment

Choose a reason for hiding this comment

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

LGTM

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