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 19 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
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.
@Pushkarm029 @kirillt I removed those steps for now, because the behavior of the
Format
was not being 100% predictable. Since I started this PR, around a month ago, theFormat
step changed its results multiple times, even though the Rust code remained exactly the sameAs example, you can check that from time to time some old and recent jobs didn't fully complete because formatting errors were randomly being thrown.
Most recent occurrence:
I judge as a bad practice using a
nightly
or any other "dynamic" version to format a project, since it can introduce errors from newly added formatting features 🤔Curious to know what you guys think about 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.
Interesting point. Actually, I prefer setting up
cargo fmt
everywhere, because it helps to avoid inconsistent code style (and standard Rust code style looks pretty fine). But I've never encounteredcargo fmt check
rejecting a bit outdated codebase.My guess is that the code style used by
cargo fmt
develops including new decisions made by Rust community. And out code is modified too slowly so we don't include these new updates. If this is true, then we could keepcargo fmt check
and actually reformat the code every time we encounter such failures on CI, but only manually.@Pushkarm029 what do you think?
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.
We need to use nightly for formatting because some of the rules we define in
rustfmt.toml
are only supported in the nightly version. Rust nightly is updated frequently, that’s not a problem — it’s the pre-release version. We have two options:• We can abandon the rules we’ve defined (I would like to keep them)
• We can check for formatting in a separate job that runs nightly, within the same workflow.
For example:
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.
We actually care about formatting only during merging, so we probably can configure GitHub rules and prevent merge if
cargo fmt
job fails specifically. This would allow see that the build itself works.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.
@oluiscabral I think we don't have
cargo clippy --workspace --bins -- -D warnings
anymore..Not sure if it's really any valuable though, does anyone looks into CI warnings? 🤔
cc @tareknaser @Pushkarm029
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.
i think
cargo clippy --workspace --bins -- -D warnings
is necessary, It helps us catch common mistakes, anti-patterns, and potential improvements that the compiler doesn't check for.So, I suggest we should add it between the
format
andbuild-and-test
job.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.
@Pushkarm029 feel free to review 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.
Not sure if we really need a separate workflow just for the cache (
.github/workflows/cache.yml
). I see that it only runs on push/PR to the main branch. Can we modify the save condition in the main workflow to:save-if: ${{ github.ref == 'refs/heads/main' }}
and remove the redundant workflow?
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.
cc @Pushkarm029
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.
Whether we keep this workflow or not, I don’t think it’s worth running it on macOS ARM (
macos-13-xlarge
) with every push to main. We only use the macOS ARM workflow in the weekly build since it’s a paid runner.Or, we could increase the retention period for this specific GitHub Actions artifact.
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.
Agreed
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?