Skip to content
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

Add FSRS version to BuildConfig #427

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Add FSRS version to BuildConfig #427

merged 2 commits into from
Oct 14, 2024

Conversation

voczi
Copy link
Contributor

@voczi voczi commented Oct 13, 2024

Excerpt from the Discord (slightly edited):
"i just used the FSRS crate version. actually pulling the version from the JS file (in the "normal" format, i.e. "FSRS 5.x.x") would require us to checkout the fsrs4anki repo with the branch==fsrs crate version, then pull the version from the js file with some regex.. very messy.
using the fsrs crate version would be that we can track any change to the FSRS crate, not just to the algorithm.
pulling the hash of the fsrs git commit can also be done, but is messy because anki is fed the fsrs dep through crates.io and not through github. so, best avoided.
image"

Related issue: ankidroid/Anki-Android#17236

david-allison
david-allison previously approved these changes Oct 13, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, cheers!

@voczi
Copy link
Contributor Author

voczi commented Oct 13, 2024

I'm just wrestling a bit with Gradle right now. Seems like JavaExec has different behavior for double quotes on different platforms..

@david-allison david-allison dismissed their stale review October 13, 2024 10:35

This is why I didn't review first thing in the morning. CI failure

@voczi
Copy link
Contributor Author

voczi commented Oct 13, 2024

I'll see if I can avoid cargo downloading a bunch of crates to parse the version.. Otherwise, we will just use the "non-clean" route of grabbing it directly from Cargo.toml.

@voczi voczi force-pushed the main branch 2 times, most recently from cdb1b77 to 9b3b4d3 Compare October 13, 2024 11:01
@voczi
Copy link
Contributor Author

voczi commented Oct 13, 2024

Never mind. I don't think some extra crates being downloaded matters that much..
Maybe @mikehardy has an opinion here, but I think I prefer using the cargo command (+jaq to parse the JSON) over some hard-coded method.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, I'm going to dump the Cargo.lock change (consider in another PR that repairs whatever weirdness we have) though or at least separate it, then repush - so it's cherry-pickable

Cargo.lock Outdated
@@ -1485,9 +1485,9 @@ dependencies = [

[[package]]
name = "fsrs"
version = "1.2.2"
version = "1.3.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why, on main, is Cargo.lock set with fsrs of 1.2.2 ? That pin should have moved with the 24.10beta2+ and beta3 submodule sha updates? 🤔

Either way I need lock updates in separate commits because we have a release-2.19 branch here for me to release stable backend versions (that do not contain 24.10betas...) paired with the Anki-Android repo 2.19 release coming up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, peeled this to #428 - it was my mistake originally, and you weren't wrong to have it in your local main but it was a whole process failure on my part. That PR should fix it, I'm purging it from this PR

build_rust/src/main.rs Show resolved Hide resolved
@mikehardy mikehardy added the pending-merge Waiting on CI or question responses to merge, but otherwise ready label Oct 13, 2024
@mikehardy
Copy link
Member

Pending CI, I expect to see an FSRS version 1.2.1 in there indicating this is running well
It worked great locally

@mikehardy
Copy link
Member

post-merge:
1- cherry-pick -x to release-2.19 branch
2- bump the gradle.properties local backend version number there
3- run release workflow with shipit input param on the release-2.19 branch
4- integrate this in to Anki-Android via a PR there

optional:
5- bump the gradle.properties local backend version on main
6- run release workflow with shipit input param on main for use in other PRs

...or could wait until next anki beta with FSRS v3/v4 cross-compat updates upstream

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much!

@mikehardy leaving this one to you, as there are follow-up tasks

@@ -26,7 +26,7 @@ def getAnkiCommitHash = { ->
def getFsrsVersion = { ->
def pkgStdout = new ByteArrayOutputStream()
exec {
commandLine "cargo", "metadata", "--format-version=1", "--manifest-path=" + new File("${project.rootDir}", "anki/Cargo.toml")
commandLine "cargo", "metadata", "--locked", "--format-version=1", "--manifest-path=" + new File("${project.rootDir}", "anki/Cargo.toml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat trick, wasn't aware of this one

@mikehardy mikehardy merged commit 15b4385 into ankidroid:main Oct 14, 2024
6 checks passed
@mikehardy mikehardy removed the pending-merge Waiting on CI or question responses to merge, but otherwise ready label Oct 14, 2024
@mikehardy
Copy link
Member

I have bumped local version to 0.1.43 on main and release-2.19 branches and triggered releases for both, for use in Anki-Android
They should be up and available for integration in that repo in approximately an hour

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants