-
Notifications
You must be signed in to change notification settings - Fork 209
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
macOS向け自動ビルドの際に.dylibファイルの問題を検出・修正するスクリプトを追加 #191
macOS向け自動ビルドの際に.dylibファイルの問題を検出・修正するスクリプトを追加 #191
Conversation
フォーク先でビルドしてみた時のアーティファクトは以下のリンク先で確認できます: ダウンロードして https://github.com/Hiroshiba/voicevox_engine/issues/177#issuecomment-967229739 で用いた検証スクリプトで調べた限りでは、問題は解消されています。また、自分の環境(Intel Mac, macOS Monterey version 12.0.1)で動作確認も取れました。 (アーティファクトは zip ファイルでダウンロードできますが、OS の標準解凍ツールを用いるだけだと https://github.com/Hiroshiba/voicevox_engine/issues/163 で見られたようなセキュリティ警告で簡単には実行できません。この Issue の末尾で触れた 7z コマンドで再圧縮・再解凍すると素直に実行可能になります。) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
全体的に良い実装だと感じました!!
たぶん変更してもしなくても良いコメントしか書いていないので、変更せずにそのままrereviewして頂いても問題ないはずです。
|
||
def get_rpaths(shared_lib_path: Path) -> List[Path]: | ||
"""引数で指定された共有ライブラリのrpathのリストを返す""" | ||
proc = subprocess.run(["otool", "-L", str(shared_lib_path)], stdout=subprocess.PIPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capture_output=Trueでもできるかもです。
(修正はしてもしなくてもいいと思います)
[ | ||
"install_name_tool", | ||
"-change", | ||
str(old_rpath), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subprocess.runはPathをそのまま受け取れるからstrはなくてもよかったりします。
@@ -0,0 +1,46 @@ | |||
import argparse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここにあるコードはおそらくずっと変わらないと思います。
将来みたい人がどういう目的かぱっとわかるように、このファイルが行うことを一言だけ一番上にdocstringを書いて頂けると!
for rpath in non_distributable_rpaths: | ||
if not (rpath.name in internal_dylib_names): | ||
external_dylib_paths.append(rpath) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここから上の処理は結構似ているので、1つのファイルにしても良いかもと思いました。
(が、まああまりいじられないコードだとも思うので、このままでも良いと思います)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
これでmacOSのエディタ側の自動ビルドが完成まで進められそうですね・・・!
試しにreleasesを作ってみます。 |
* run build.yml on every push (for test-build) * macOSビルドの.dylibファイル関連の修正を行うスクリプトを追加 * macOS向け自動ビルド時に.dylibファイル関連の修正スクリプトを実行 * fix typo * run build.yml on master push * strオブジェクトへの不要な変換を削除 * add docstring
内容
macOS向け自動ビルドの産物において、
ことについて、自動ビルド時に https://github.com/Hiroshiba/voicevox_engine/issues/177#issuecomment-967229739 で検討した方法で修正します。
関連 Issue
close #177
ref https://github.com/Hiroshiba/voicevox/issues/348
ref https://github.com/Hiroshiba/voicevox/issues/399
スクリーンショット・動画など
なし
その他
https://github.com/Hiroshiba/voicevox_engine/issues/177#issuecomment-967285885 で、スクリプトの置き場所として
との指示を受けましたが、macOS向け自動ビルド時に https://github.com/Hiroshiba/voicevox_engine/blob/d652f48a9092cb9debe9ff6122b1808fd30b14c2/.github/workflows/build.yml#L37-L38 で Nuitka のビルド結果を格納する一時ディレクトリの名前として使用済みだったので、代わりに build_util ディレクトリを作りました。もしもっと良い名前があればご提案いただければと思います。