Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Build
fs-storage
and publish Java package #94base: main
Are you sure you want to change the base?
Build
fs-storage
and publish Java package #94Changes from all commits
6af1614
571aab6
560fca7
8ccb90e
0a76e96
475a94e
f5f4420
85a00ca
57a28c1
06db31b
6730418
68f478f
c80086b
2e77dfe
3c572a6
5cec424
b6c4d2c
5b1ea61
229b48f
78d058d
7e2bd8b
54d9665
736a206
ceef135
9cd65f3
0c1fa09
61cd3f5
14b2fff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
getting this warning locally, and I can also see it in the CI:
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.
Hmm I see, I'll investigate it
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
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.
Aaahh, that's why would be great to move all the Java code to
ark-android
.. especially after we switched from JAR to AAR, so we don't do "plain Java" anymore and really only use Java for Android..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.
Yes. Maybe we could even move the JNI Rust related code to
ark-android
then, since it is specifically related to Java as wellThere 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.
@oluiscabral exactly! do you think it's reasonable amount of work for the current pair of PRs, or should we merge these first, and proceed with refactoring in new PRs?
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.
Could you add these targets to the instructions in
java/README.md
, specifically therustup
command?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.
Good idea, do you mean to add a step-by-step on how to locally cross-compile the library?
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.
When I tried running this locally, it failed because I didn’t have these targets set up, and I had to debug it myself. It would be helpful to include a command for adding these targets as a perquisite, something like:
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.
Also, let’s include a note about the supported
ndkVersion
, because running this with a different version also causes it to completely fail locally for me (I had version28.0.12674087
. This is why I suggested #94 (comment))with an annoying error as well (I think it said something like
ndk
not installed)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.
@tareknaser this one seems to be resolved, right?