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

anki: 2.1.15 -> 2.1.60 #221229

Merged
merged 3 commits into from
Apr 11, 2023
Merged

anki: 2.1.15 -> 2.1.60 #221229

merged 3 commits into from
Apr 11, 2023

Conversation

euank
Copy link
Member

@euank euank commented Mar 14, 2023

Description of changes

This updates the native packaging of anki for their new ninja-based build-system.

It's somewhat fragile feeling since their build-system isn't exactly designed for nix, but it does appear to work!
The high-level approach is that we do stuff like create node_modules, download stuff, create a python environment with nix, then we mock out a couple of things they run with our own stub bash scripts.
The upstream build system is responsible for building rust code and python wheels, and the nix derivation is responsible for getting node_modules in place, vendoring cargo deps, and all the other usual stuff like setting buildinputs.

After getting the wheel, we do normal python application packaging stuff.

I'm also happy to report that ibus based IME works on this one, unlike anki-bin, which was my main motivation for tackling this in the first place, so that's awesome!

I haven't tested this on macOS at all since I don't have macOS hardware, so someone else will have to see what's broken there!

Fixes #78449, Fixes #112921

Helps with #147989

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@martinetd
Copy link
Member

Syncing doesn't work, returning a network error ("⁨error sending request for url (): error trying to connect: invalid URL, scheme is not http⁩", like it's using empty string for the sync server), and I'm really not sure why.

I'm using my own anki server and setting the SYNC_ENDPOINT="https://xyz.tld/sync/" env var manually when running the client, you can perhaps give that a spin. The default seems to be https://sync.ankiweb.net/sync/

with that said -- awesome work. I agree not using pyoxidizer makes more sense for nixos, and I'll try to get my sync server working with this (been using the pip version as the ankisyncd in nixpkgs is too old and updating it requires a new anki...) when I find the time to try :)

pkgs/games/anki/default.nix Outdated Show resolved Hide resolved
@euank
Copy link
Member Author

euank commented Mar 15, 2023

I figured out what was wrong with syncing - I forgot to add buildFeatures = [ "rusttls" ] to the rust bridge, which is where http requests happen... so it just couldn't do tls and gave a kinda bad error.

Easy enough to fix, and syncing seems happy now 😀

@euank euank marked this pull request as draft March 15, 2023 13:41
@euank
Copy link
Member Author

euank commented Mar 15, 2023

Back to draft. While it now launches and syncs, the review and stats pages are blank, and it produces some javascript errors.

Something's still wrong with the packaging I guess.

@euank euank force-pushed the anki-native branch 3 times, most recently from bd4a249 to f5d0635 Compare March 16, 2023 10:47
@euank
Copy link
Member Author

euank commented Mar 16, 2023

the review and stats pages are blank

Found it! I was missing applying this patch: https://github.com/ankitects/anki/blob/2.1.60/ts/patches/protobufjs%2B7.2.1.patch

I missed that they had a postinstall in their package.json which handles that, and I had --ignore-scripts on our invocation.

After fixing that, it all seems to work!

@euank euank marked this pull request as ready for review March 16, 2023 10:48
@euank
Copy link
Member Author

euank commented Mar 16, 2023

I've updated the top-level comment with a bit more detail. I think this is ready for review now!

I'd also appreciate if someone with a mac could see if it works there

, CoreAudio
# This little flag adds a huge number of dependencies, but we assume that
# everyone wants Anki to draw plots with statistics by default.
, plotsSupport ? true
Copy link
Member Author

Choose a reason for hiding this comment

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

this used to pull in matplotlib, but grepping around in the repo, it looks like they don't use that anymore, so we can drop this.

pkgs/games/anki/default.nix Outdated Show resolved Hide resolved
@GabrielDougherty
Copy link
Member

Result of nixpkgs-review pr 221229 run on x86_64-linux 1

1 package failed to build:
  • ankisyncd
2 packages built:
  • anki
  • mnemosyne

@nessdoor
Copy link
Contributor

@euank sorry for getting back to you so late. Yes, the Qt6 modifications worked, and I'm now able to launch Anki without any issues.

If anything else comes up, I'll try to let you know as soon as possible.

@martinetd
Copy link
Member

opened #224366 for ankisyncd rust package update (addition) and service update
(I wouldn't hold this back for the server side; won't be using the client on nixos so no real opinion but this LGTM at this point)

@euank
Copy link
Member Author

euank commented Apr 3, 2023

Thanks @martinetd!

Since I personally think this PR's good to merge at this point (and it has a couple approvals), I'll ping the PRs already reviewed thread in a couple days if no one here objects or merges it before then.
Thanks again for the help testing this!

@martinetd
Copy link
Member

@euank I tried rebuilding this after cherry-picking on master and I can no longer build

cargo-deps-vendor.tar.gz> source root is source
cargo-deps-vendor.tar.gz> patching sources
cargo-deps-vendor.tar.gz> configuring
cargo-deps-vendor.tar.gz> no configure script, doing nothing
cargo-deps-vendor.tar.gz> building
cargo-deps-vendor.tar.gz> source = "git+https://github.com/ankitects/rust-csv.git?rev=1c9d3aab6f79a7d815c69f925a46a4590c115f90#1c9d3aab6f79a7d815c69f925a46a4590c115f90"
cargo-deps-vendor.tar.gz> source = "git+https://github.com/ankitects/rust-csv.git?rev=1c9d3aab6f79a7d815c69f925a46a4590c115f90#1c9d3aab6f79a7d815c69f925a46a4590c115f90"
cargo-deps-vendor.tar.gz> source = "git+https://github.com/ankitects/linkcheck.git?rev=2f20798ce521cc594d510d4e417e76d5eac04d4b#2f20798ce521cc594d510d4e417e76d5eac04d4b"
cargo-deps-vendor.tar.gz> ERROR: The Cargo.lock contains git dependencies
cargo-deps-vendor.tar.gz> This is currently not supported in the fixed-output derivation fetcher.
cargo-deps-vendor.tar.gz> Use cargoLock.lockFile / importCargoLock instead.

It looks like you'll need to switch to lockFile with a Cargo.lock in nixpkgs like I did for anki-sync-server-rs in the other PR (I assume they turned on fixed-output derivation style building recently? I'm pretty sure I was able to build on this system on your branch before rebasing... will recheck and update if not.

@euank
Copy link
Member Author

euank commented Apr 5, 2023

Yup, indeed, it looks like since #221716 landed on master, I need to switch over as it says.

I'll give that a go now

Updating anki in the next commit will break it otherwise.
@euank
Copy link
Member Author

euank commented Apr 5, 2023

$ maintainers/scripts/convert-to-import-cargo-lock.sh anki
converting anki (1/1)
thread 'main' panicked at 'package should contain cargoHash/cargoSha256', src/main.rs:176:52

The automated tool couldn't get all of it, but it was easy enough to do the rest by hand.

Updated, verified it builds and launches after converting it from fetchCargoTarball to importCargoLock

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/893

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

Builds and works on plasma/wayland (through Xwayland).

@@ -11,7 +10,8 @@ python3.pkgs.buildPythonApplication rec {
owner = "ankicommunity";
repo = "anki-sync-server";
rev = version;
sha256 = "196xhd6vzp1ncr3ahz0bv0gp1ap2s37j8v48dwmvaywzayakqdab";
hash = "sha256-RXrdJGJ+HMSpDGQBuVPPqsh3+uwAgE6f7ZJ0yFRMI8I=";
fetchSubmodules = true;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the review!

anki-sync-server 2.4.0 still requires anki from pypi, so it's not going to be a straightforward change (and the update isn't as simple as bumping the version number)

The current discussion seems to favor just getting rid of this flavor of ankisyncd in favor of the rust one (being discussed in #224366 ) so I'd say to just not touch ankisyncd here more than we have to for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, as discussed in #224366 and previously in this pr, I don't think it's worth updating it (nor is it easy to do so).
I'm just trying to leave it functioning at all since this PR would otherwise break it, and then we can deal with it separately

This redoes all the packaging for their new build-system.

It feels a bit fragile, but in practice it works.

Basically, we build most of it in nix, write some wrapper scripts to
mock out stuff we just did in nix, and then call thier build system to
make a wheel
Thanks to Lucio for pointing this one out, appreciated
@mightyiam
Copy link
Member

Thank you!

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

Successfully merging this pull request may close these issues.

anki: upgrade to latest release