-
Notifications
You must be signed in to change notification settings - Fork 14
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
add Windows support #12
Conversation
0f7653e
to
f2a9944
Compare
f2a9944
to
baed72c
Compare
bae9673
to
8bb9e45
Compare
b26ba20
to
d876733
Compare
d876733
to
b935771
Compare
bbe4a82
to
98ed409
Compare
be0e788
to
21c2d6a
Compare
And, as an aside, this PR could probably be cleaned up by someone who knows Maven better than me. 😄 But this works and isn't too invasive. |
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.
Thank you @erip for your great work!
One question, can we remove the file windows-jni/include/jni.h
?
I can try, but I suspect there may be issues. Specifically on Windows, jint is an alias to a native long, which causes a lot of issues. The workaround is often to use a custom (in this case, Android-based) JNI header. |
f94a639
to
a5cf8b6
Compare
@levyfan, as I suspected this won't work: https://github.com/erip/sentencepiece-jni/runs/1668614105?check_suite_focus=true#step:4:3561 Because a |
a5cf8b6
to
96cd231
Compare
This PR seeks to add Windows support to this repo to close #11. As a part of this, it will transition to GitHub Actions for ease of testing.