Build fs-storage and publish Java package
#94
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for 1e22079Click to view benchmark
|
Benchmark for ca7196fClick to view benchmark
|
Benchmark for ef870fdClick to view benchmark
|
Benchmark for d3bd137Click to view benchmark
|
Benchmark for c2f4c49Click to view benchmark
|
Closed
Benchmark for 3da4d0cClick to view benchmark
|
Pushkarm029
requested changes
Nov 3, 2024
kirillt
reviewed
Nov 9, 2024
kirillt
reviewed
Nov 9, 2024
oluiscabral
commented
Nov 9, 2024
oluiscabral
commented
Nov 9, 2024
Benchmark for 113f173Click to view benchmark
|
Benchmark for e0121b1Click to view benchmark
|
Benchmark for ba0c3daClick to view benchmark
|
Pushkarm029
approved these changes
Nov 15, 2024
Benchmark for 7c743f2Click to view benchmark
|
Benchmark for 7290f2dClick to view benchmark
|
Benchmark for c862088Click to view benchmark
|
|
@oluiscabral Let me know when you are ready for the final review. |
Benchmark for e8fad36Click to view benchmark
|
Benchmark for 47a5554Click to view benchmark
|
Benchmark for 8f9ea0fClick to view benchmark
|
Benchmark for 3b7ea9dClick to view benchmark
|
Benchmark for 905472dClick to view benchmark
|
Benchmark for 37c6660Click to view benchmark
|
Benchmark for 97659e5Click to view benchmark
|
Alright, @Pushkarm029 and @kirillt, now The published package (AAR) is currently being referenced at ARK-Builders/ark-android#110 |
oluiscabral
commented
Dec 12, 2024
.github/workflows/build.yml
Outdated
Comment on lines
24
to
37
| with: | ||
| toolchain: nightly # nightly is required for fmt | ||
| components: rustfmt, clippy | ||
|
|
||
| - name: Check | ||
| run: cargo check | ||
|
|
||
| - name: Format | ||
| run: | | ||
| cargo fmt --all -- --check | ||
| cargo clippy --workspace --bins -- -D warnings | ||
|
|
||
| - name: Build Debug | ||
| run: cargo build --verbose |
There was a problem hiding this comment.
@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, the Format step changed its results multiple times, even though the Rust code remained exactly the same
As 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:
- https://github.com/ARK-Builders/ark-core/actions/runs/12120246957/job/33788525665 ran just fine
- https://github.com/ARK-Builders/ark-core/actions/runs/12287131386/job/34288596396 a week later started raising a formatting error
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.
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 encountered cargo 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 keep cargo 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.
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:
jobs:
format:
name: Code Formatting Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Install Nightly Rust with rustfmt
uses: dtolnay/rust-toolchain@stable
with:
toolchain: nightly
components: rustfmt
- name: Run rustfmt
run: cargo fmt --all -- --check
build_and_test:
name: Build and Test
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Install Stable Rust
uses: dtolnay/rust-toolchain@stable
with:
toolchain: stable
...There was a problem hiding this comment.
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.
@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? 🤔
There was a problem hiding this comment.
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 and build-and-test job.
Benchmark for 6cb015bClick to view benchmark
|
tareknaser
reviewed
Dec 14, 2024
There was a problem hiding this comment.
getting this warning locally, and I can also see it in the CI:
> Task :lib:compileReleaseJavaWithJavac
Java compiler version 22 has deprecated support for compiling with source/target version 8.
warning: [options] source value 8 is obsolete and will be removed in a future release
Try one of the following options:
1. [Recommended] Use Java toolchain with a lower language version
2. Set a higher source/target version
3. Use a lower version of the JDK running the build (if you're not using Java toolchain)
For more details on how to configure these settings, see https://developer.android.com/build/jdks.
To suppress this warning, set android.javaCompile.suppressSourceTargetDeprecationWarning=true in gradle.properties.
warning: [options] target value 8 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
3 warnings
warning: [options] source value 8 is obsolete and will be removed in a future releaseThere was a problem hiding this comment.
Hmm I see, I'll investigate it
There was a problem hiding this comment.
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.
Yes. Maybe we could even move the JNI Rust related code to ark-android then, since it is specifically related to Java as well
There was a problem hiding this comment.
@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?
Comment on lines
+20
to
+21
| targets: aarch64-linux-android armv7-linux-androideabi i686-linux-android x86_64-linux-android | ||
|
|
There was a problem hiding this comment.
Could you add these targets to the instructions in java/README.md, specifically the rustup command?
There was a problem hiding this comment.
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.
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:
rustup target add \
aarch64-linux-android \
armv7-linux-androideabi \
...There was a problem hiding this comment.
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 version 28.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.
@tareknaser this one seems to be resolved, right?
.github/workflows/cache.yml
Outdated
There was a problem hiding this comment.
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.
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.
and remove the redundant workflow?
Agreed
.github/workflows/build.yml
Outdated
Comment on lines
24
to
37
| with: | ||
| toolchain: nightly # nightly is required for fmt | ||
| components: rustfmt, clippy | ||
|
|
||
| - name: Check | ||
| run: cargo check | ||
|
|
||
| - name: Format | ||
| run: | | ||
| cargo fmt --all -- --check | ||
| cargo clippy --workspace --bins -- -D warnings | ||
|
|
||
| - name: Build Debug | ||
| run: cargo build --verbose |
There was a problem hiding this comment.
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:
jobs:
format:
name: Code Formatting Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Install Nightly Rust with rustfmt
uses: dtolnay/rust-toolchain@stable
with:
toolchain: nightly
components: rustfmt
- name: Run rustfmt
run: cargo fmt --all -- --check
build_and_test:
name: Build and Test
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Install Stable Rust
uses: dtolnay/rust-toolchain@stable
with:
toolchain: stable
...
Benchmark for 0bdc6d5Click to view benchmark
|
Benchmark for 89e2920Click to view benchmark
|
Benchmark for 165583eClick to view benchmark
|
Benchmark for 315e023Click to view benchmark
|
Benchmark for 7255d80Click to view benchmark
|
Benchmark for d37222cClick to view benchmark
|
Merged
* refactor(CI): move Java bindings into a separate job * chore(CI): remove old benchmark workflow and integrate into build process * refactor(CI): consolidate OS jobs into a single build-and-test job --- Signed-off-by: Tarek <[email protected]>
Benchmark for aa5a35cClick to view benchmark
|
Pushkarm029
approved these changes
Jan 11, 2025
There was a problem hiding this comment.
Great work
@oluiscabral, Let's resolve the minor changes now and save the major ones for the next PR or create an issue for them. This PR is already quite large and has been open for months.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.