-
Notifications
You must be signed in to change notification settings - Fork 6
Build fs-storage and publish Java package
#94
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
Changes from 20 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
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: > 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 release
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I see, I'll investigate it
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Maybe we could even move the JNI Rust related code to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,20 +21,11 @@ jobs: | |
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| 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 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
I judge as a bad practice using a Curious to know what you guys think about it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting point. Actually, I prefer setting up My guess is that the code style used by @pushkarm029 what do you think?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oluiscabral I think we don't have Not sure if it's really any valuable though, does anyone looks into CI warnings? 🤔
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think So, I suggest we should add it between the |
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: false | ||
oluiscabral marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - name: Run tests | ||
| run: cargo test --verbose | ||
|
|
@@ -54,6 +45,15 @@ jobs: | |
| - name: Set up Gradle | ||
| uses: gradle/actions/setup-gradle@v3 | ||
|
|
||
| - name: Set up Android SDK | ||
| uses: android-actions/setup-android@v3 | ||
|
|
||
| - name: Set up Android NDK | ||
| uses: nttld/setup-ndk@v1 | ||
| with: | ||
| link-to-sdk: true | ||
| ndk-version: r28-beta1 | ||
|
|
||
| - name: Java tests | ||
| run: gradle test | ||
| working-directory: ./java | ||
|
|
@@ -68,12 +68,17 @@ jobs: | |
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Build Release | ||
| run: cargo build --verbose --release | ||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: false | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose | ||
|
|
||
| - name: Build Release | ||
| run: cargo build --verbose --release | ||
|
|
||
| - name: Run `ark-cli watch` test | ||
| run: ./integration/ark-cli-watch.sh | ||
|
|
||
|
|
@@ -86,6 +91,15 @@ jobs: | |
| - name: Set up Gradle | ||
| uses: gradle/actions/setup-gradle@v3 | ||
|
|
||
| - name: Set up Android SDK | ||
| uses: android-actions/setup-android@v3 | ||
|
|
||
| - name: Set up Android NDK | ||
| uses: nttld/setup-ndk@v1 | ||
| with: | ||
| link-to-sdk: true | ||
| ndk-version: r28-beta1 | ||
|
|
||
| - name: Java tests | ||
| run: gradle test | ||
| working-directory: ./java | ||
|
|
@@ -100,12 +114,17 @@ jobs: | |
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Build Release | ||
| run: cargo build --verbose --release | ||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: false | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose | ||
|
|
||
| - name: Build Release | ||
| run: cargo build --verbose --release | ||
|
|
||
| - name: Run `ark-cli watch` test | ||
| run: ./integration/ark-cli-watch.sh | ||
|
|
||
|
|
@@ -118,6 +137,15 @@ jobs: | |
| - name: Set up Gradle | ||
| uses: gradle/actions/setup-gradle@v3 | ||
|
|
||
| - name: Set up Android SDK | ||
| uses: android-actions/setup-android@v3 | ||
|
|
||
| - name: Set up Android NDK | ||
| uses: nttld/setup-ndk@v1 | ||
| with: | ||
| link-to-sdk: true | ||
| ndk-version: r28-beta1 | ||
|
|
||
| - name: Java tests | ||
| run: gradle test | ||
| working-directory: ./java | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pushkarm029 feel free to review it 😀
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( save-if: ${{ github.ref == 'refs/heads/main' }}and remove the redundant workflow?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @pushkarm029
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| name: Cache build | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| branches: | ||
| - main | ||
|
|
||
| jobs: | ||
| linux: | ||
| name: Linux | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: true | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose --release | ||
|
|
||
| windows: | ||
| name: Windows | ||
| runs-on: windows-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: true | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose --release | ||
|
|
||
| mac-intel: | ||
| name: MacOS Intel | ||
| runs-on: macos-14 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: true | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose --release | ||
|
|
||
| mac-arm: | ||
| name: MacOS ARM | ||
| runs-on: macos-13-xlarge | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: true | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose --release |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| name: Release | ||
|
|
||
| on: | ||
| push: | ||
| tags: | ||
| - "*" | ||
kirillt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| jobs: | ||
| release: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| packages: write | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| targets: aarch64-linux-android armv7-linux-androideabi i686-linux-android x86_64-linux-android | ||
|
|
||
|
Comment on lines
+20
to
+21
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add these targets to the instructions in
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: rustup target add \
aarch64-linux-android \
armv7-linux-androideabi \
...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, let’s include a note about the supported with an annoying error as well (I think it said something like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tareknaser this one seems to be resolved, right? |
||
| - name: Set up Cargo Cache | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| save-if: false | ||
|
|
||
| - name: Run tests | ||
| run: cargo test --workspace --verbose | ||
|
|
||
| - name: Install JDK | ||
| uses: actions/[email protected] | ||
| with: | ||
| distribution: "temurin" | ||
| java-version: "22" | ||
|
|
||
| - name: Set up Gradle | ||
| uses: gradle/actions/setup-gradle@v3 | ||
|
|
||
| - name: Set up Android SDK | ||
| uses: android-actions/setup-android@v3 | ||
|
|
||
| - name: Set up Android NDK | ||
| uses: nttld/setup-ndk@v1 | ||
| with: | ||
| link-to-sdk: true | ||
| ndk-version: r28-beta1 | ||
|
|
||
| - name: Java tests | ||
| run: gradle test | ||
| working-directory: ./java | ||
|
|
||
| - name: Publish Java release | ||
| run: gradle publish | ||
| working-directory: ./java | ||
| env: | ||
| RELEASE_VERSION: ${{ github.ref_name }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| # Ignore Gradle project-specific cache directory | ||
| .gradle | ||
| .local.properties | ||
|
|
||
| # Ignore Gradle build output directory | ||
| build | ||
| *.log | ||
| *.log |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| buildscript { | ||
| repositories { | ||
| google() | ||
| gradlePluginPortal() | ||
| maven { url "https://plugins.gradle.org/m2/" } | ||
| } | ||
| dependencies { | ||
| classpath libs.gradle | ||
| classpath libs.rust.android | ||
| } | ||
| } | ||
|
|
||
| allprojects { | ||
| repositories { | ||
| google() | ||
| gradlePluginPortal() | ||
| maven { url "https://jitpack.io" } | ||
| maven { url "https://plugins.gradle.org/m2/" } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.