-
Notifications
You must be signed in to change notification settings - Fork 120
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
cmakeとsetup.pyを切り離す、テストの追加 #51
Conversation
ref #15 |
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!!
良さそうに感じました!
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じゃなかった、すみません。
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.
すでに挙げられている点以外には特に問題ないと思います
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.
API部と取得部が分割されていてとてもいいと思います
一点だけ確認をお願いします
testが落ちてますね... |
おそらく無いです・・・。 |
あれーテストが通らない |
Ubuntu と Mac に関しては、Python3 のコードを Python 2.7 で走らせていて、SyntaxError で落ちているようです。engine の方のテストやビルドは Python のバージョンを 3.8 に指定して走らせているので、参考になるかもしれません。 https://github.com/VOICEVOX/voicevox_engine/blob/a451258c937113e4d244a643cbaea9d86b1fdcc1/.github/workflows/test.yml |
x86のpythonモジュールのビルドが上手くいきませんね |
テスト結果はこちらで見れそうでしょうか なるほど・・・ちょっとわかりませんね・・・ |
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.
build.ymlの294行目付近を
# Build
- if: startsWith(matrix.os, 'windows')
uses: ilammy/msvc-dev-cmd@v1
with:
arch: ${{ matrix.python_architecture }}
とすると通りました
全部通りました!ありがとうございます 🙇 |
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!!
とても良いと思います!!!
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!
お疲れさまでした
内容
関連 Issue
close #50
ref #16 : cudaをサポートしているかどうかを確認する
supports_cuda
という関数を実装したので、良さそうならこれをAPIに追加しようと思います