-
Notifications
You must be signed in to change notification settings - Fork 26
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
2.1.66 and the removal of legacy schema support #294
Conversation
47e91f9
to
6811cb7
Compare
So, unfortunately I hit a roadblock - Linux/Windows builds appear to be ok, but an x86_64 library built on a Mac fails to load in the emulator. I've rolled back the NDK and Rust version for now, which seems to have resolved the issue. I couldn't see any clear solution; hopefully we can revisit this in the future and the issue will be resolved or a clear workaround will be available. |
Bleh, another failure when cross compiling the Linux archives. Rolling back zstd or the bzip crate may help, but I've run out of time for now. |
Signed-off-by: gradle-update-robot <[email protected]>
Bumps com.android.tools.build:gradle from 4.2.2 to 7.4.2. --- updated-dependencies: - dependency-name: com.android.tools.build:gradle dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
If set locally, rust-gradle uses stale artifacts. Guess who just wasted a bunch of time investigating a regression that actually wasn't? :-)
- Update the backend protobuf generation to match the refactoring done on the desktop code recently. This involved switching from the Python-based script to a new one in Rust. Comments from the proto files are now included in the generated backend bindings. - Switch from protobuf to protobuf-lite, which is what Google recommend for use on Android. - Recent desktop changes introduce a protobuf construct that protobuf-lite doesn't support, so the offending file needed to be removed as part of the build. This was easier to do by directly invoking protobuf, instead of relying on the Gradle protobuf plugin. As a bonus, this should also solve the protobuf-sources-are-missing-from-bundle issue. (file removal is no longer required due to a refactor). - Update Rust deps. is_terminal was failing to build on Windows, and is no longer required after updating to the latest android_logger - Rumo contributed changes to the upstream Chrono crate which make it work properly on Android, and these changes have been merged in, so AnkiDroid no longer needs to pin an older version. - Update to latest NDK, as latest Rust requires it (closes ankidroid#180). - Update to latest Rust - Add a script to make it easier to reformat Rust code when it fails checks.
Allows us to drop the per-platform handling and extra build step
c3469aa
to
ff5c374
Compare
- Cargo NDK is a Rust tool that builds the JNI libs for us, which we then just need to add to the Gradle search path. This allows the building to be done without running Gradle (except for the final step where they're bundled into the .aar/.jar). - The separate build-robo and build-web-assets have been folded into the single build.sh script. - Build.sh will automatically invoke gradle and vice versa, so you can build either in the UI or on the command line equally easily. - Made debug builds the default
Well that was a saga, but I think I'm getting there. The latest failure looks like it might be solved by bumping the minimum API to 23, which I'm trying now. I've been meaning to bring up the supported versions actually - while it's noble to support older devices, given that AnkiDroid barely has enough developer resources as it is, I think it may be worth considering being a bit stricter about the version skew you're willing to support, especially as the tail end comprise only a tiny portion of the userbase. The low API target presumably also causes headaches with dependency updates on the non-backend side, and it's not like those users would suddenly be unable to use the app - it would just mean they'd lose access to updates until they're able to upgrade to a newer device. Thoughts? |
21 + 22 is ~0,26% of our userbase, a pretty small and safe to cut number. If it is something important that we can't workaround (e.g. dependencies) or that demands too much work to do, I'm all in favor of cutting older APIs (21 was released almost 10 years ago) For the current release cycle, I think that we will be able to release 2.16 soon before having to make the API cut, so I'm quite confortable to remove those older APIs. Pinging @mikehardy, as he may have some more thoughts about this |
2.16 should go before making minsdkversion changes or merging this, which is just agreeing, if I understand correctly Fast follows with moving a 2.17 that bumps minsdkver and this change and all sorts of other stuff I hope |
Finally green 😩 |
This will allow the code to be shared between Unix and Windows builds.
It contains a change to the build process that should fix the CI error
That's intended - we want the output artifacts to be built in release mode, but not build_rust, as building it in release mode would be slower than any minor runtime savings it might have. |
Something went wrong with the post-run upload and now the build is broken
I'm not sure what's suddenly broken the build. Will investigate when I have a chance. |
I have no idea what happened there, but the upgrade seems to have fixed it. 🤷♂️ |
Testing note: I just ran through the buid on the M1 machine in the house, along with the corresponding Anki-Android build (using the coordinated PR there...) and ran So I think macOS apple silicon is ✅ now I'll check intel mac and windows to make sure they're still good then approve... |
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.
worked for me on macos M1 + intel, as well as windows. I assume linux will work (it's tested in CI plus I think you use it @dae ?). I use it frequently as well, happy to help fuzzbust if I'm wrong about it working
Co-authored-by: Mike Hardy <[email protected]>
Yep, Linux is my main dev box again these days. Thank you for the review! |
Hey @dae - I don't think bisectability is such a concern there and I like just about every commit in the chain. Leaning towards rebase, but could squash. You have a preference? After that I'll publish likely tomorrow (nearly bedtime here now...) |
I'll try to get a release out today, then I/we can modify the Anki-Android PR to point at the new artifact |
What a huge improvement, wow! Thank you |
@dae (minor, don't spend time on it if it's not an instant answer) Do you recall why It compiled with the following, but don't know whether it was removed due to a problem downstream: Subject: [PATCH] Bump version to 0.1.30-anki23.10.1
---
Index: rsdroid/build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/rsdroid/build.gradle b/rsdroid/build.gradle
--- a/rsdroid/build.gradle (revision 3dc1379468fc7f6322b7ddc79e080a516486b490)
+++ b/rsdroid/build.gradle (date 1702053780640)
@@ -17,11 +17,22 @@
return commit
}
+def getAnkiDesktopVersion() {
+ Properties properties = new Properties()
+ properties.load(project.rootProject.file('gradle.properties').newDataInputStream())
+ def versionName = properties.getProperty('VERSION_NAME')
+ return versionName.substring(versionName.indexOf("anki") + "anki".length())
+}
+
android {
namespace 'net.ankiweb.rsdroid'
compileSdk rootProject.ext.compileSdk
ndkVersion "26.1.10909125" // Used by GitHub actions - avoids an install step on some machines
+ buildFeatures {
+ buildConfig true // expose 'ANKI_DESKTOP_VERSION'
+ }
+
compileOptions {
sourceCompatibility JavaVersion.VERSION_11
targetCompatibility JavaVersion.VERSION_11
@@ -34,6 +45,7 @@
versionName VERSION_NAME
consumerProguardFiles "consumer-rules.pro"
+ buildConfigField "String", "ANKI_DESKTOP_VERSION", "\"" + getAnkiDesktopVersion() + "\""
}
buildTypes { |
I believe it broke when Gradle was updated. If your changes or the passage of time got it working again, and you plan to use it, feel free to add it back. |
ANKI_COMMIT_HASH restored from c495fd0 Removed (likely0 due to a bad gradle upgrade: ankidroid#294 (comment)
ANKI_COMMIT_HASH restored from c495fd0 Removed (likely0 due to a bad gradle upgrade: #294 (comment)
2.1.66 is not due out soon, but I thought I'd do this now while the proto gen changes are fresh in memory.
on the desktop code recently. This involved switching from the Python-based
script to a new one in Rust. Comments from the proto files are now
included in the generated backend bindings.
Also:
it work properly on Android, and these changes have been merged in,
so AnkiDroid no longer needs to pin an older version.
Needs testing to make sure building/running on macOS still works.
for use on Android.
checks.