ankisyncd-rs: add package for anki-sync-server-rs #224366
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
8.has: package (new)
|
This should probably have a release note. |
|
@martinetd I haven't been following anki development very much. Is there a discussion you can link to where this is discussed by the maintainers of anki or ankisyncd? |
Yes, see TODO. probably should have marked this as draft, sorry doing it now.
Haven't discussed anything, the python and rust ankisyncd are reasonably compatible if up to date so it doesn't matter which we update -- but now we're updating anki itself, we definitely should update ankisyncd, and it looked like less effort to use the rust version. (I've been using it for a few days without problems as a datapoint, the users and data was conserved coming from an up to date python ankisyncd, so not the one in nixos. I haven't tested coming from the old python version as it doesn't work with any of my anki clients... And that is a known issue afaik) If you'd prefer updating the python version instead I'll be happy to close this :) |
The most relevant issue upstream I think is this: ankicommunity/ankicommunity-sync-server#158 I think that's already a quite reasonable argument for using this rust variant, despite it being a breaking change. The other quite compelling reason for using this is that anki-sync-server is a pain for nixpkgs to package. It's many versions behind, I don't expect anyone to actually update it, and I think switching to the rust one is pragmatic in terms of our own packaging time and effort. |
|
@martinetd @euank I'm probably not the best person to weigh in on this, being a bit out of touch with the anki community. My main thought with linking to a discussion was just whether this was an "official" solution or at least something which has acceptance from a large portion of the community. If I've understood your previous comments and a bit of light research, there is now an official sync server provided by Anki but it has worse backwards compatibility and is difficult to package for nix. I think it's great to package ankisyncd-rs but I'm not so sure this should become the default implementation used in Nixpkgs given that there is an official version. The anki project has hundreds of contributors and is 10 years old. This project is just a few years old and has 3 contributors (only 1 large contributor). I would weigh those arguments against each other. Maybe we can also find some other Nix community members that use these packages to provide their opinion? |
Fortunately, we actually do end up with that ( On the rest, I agree with you. The official one is better if that's good enough for users of the module. |
|
My understanding is that this is just a wrapper above the sync server code in anki itself -- the build here takes the anki sources and applies the patch in the anki-sync-server-rs which basically just makes functions accessible from outside, and then use it with a few bells and whistles. Even without these bells and whistles, anki itself requires qt, plenty of node stuff, lame/mpv... while the sync server is mostly standalone with close to 0 dep. My server running the sync server isn't my workstation where I run the client, it doesn't make sense to have the same bundle with everything. Honestly I don't see the need for both the python and the rust versions of the standalone server, so I don't think we need to package both, but I think we ought to package either for the reasons given above. Given the database itself are compatible between the two so we can always switch (I've just switched from a recent python ankisyncd installed from pip to this rust package myself last week) as long as we switch the config as well, so we're not locking ourselves in anything either and we can just say ok let's go back to python later if the rust port dies; but at this point the rust server seems easier to maintain for us so I also don't see a reason not to use it given our python ankisyncd is obsolete (doesn't currently work with ankidroid nor my semi-oldish 2.1.54 on this laptop, so there's been another breakage before that 2.1.57+ link) |
|
You using this module and finding this rust server better than I, personally, would be happy to approve and see this PR merged once we have the release note indicating this change is happening, for whatever my opinion is worth. And yeah, I think the community python version should probably be dropped since I don't really expect anyone to take the time to update it, especially since you could use this version instead of spending hours of time fighting with build-systems. |
Sorry for double push notification I fumbled a bit with a rebase on the cargolock change. |
| @@ -288,6 +288,8 @@ In addition to numerous new and upgraded packages, this release has the followin | |||
| - `services.dhcpcd` service now don't solicit or accept IPv6 Router Advertisements on interfaces that use static IPv6 addresses. | |||
| If network uses both IPv6 Unique local addresses (ULA) and global IPv6 address auto-configuration with SLAAC, must add the parameter `networking.dhcpcd.IPv6rs = true;`. | |||
|
|
|||
| - The module [services.ankisyncd](#opt-services.ankisyncd.package) has been switched to [anki-sync-server-rs](https://github.com/ankicommunity/anki-sync-server-rs) from the old python version, which was difficult to update and not updated in a while, not supporting recent clients. The config file is different, but users' database is compatible so at most a rsync from clients might be needed. | |||
There was a problem hiding this comment.
Couple wording nits:
- "rsync" -> "resync"
- "which was difficult to update and not updated in a while, not supporting recent clients" -> "which was difficult to update, had not been updated in a while, and did not support recent versions of anki." (the last bit is a sentence fragment otherwise I think).
|
Running nixpkgs-review output(thanks for carrying this forward though! Things look good to me, other than the tiny wording nits and nixpkgs-review not being happy) |
|
A naive |
|
gah, forgot to git add the file, added it now. Sorry/thanks! Also fixed the wording while I was at it, that helped. Bedtime here, will address any further issue tomorrow if there's more. |
|
Result of 1 package blacklisted:
1 package built:
I also built it and made sure it ran, looks good! |
|
@ofborg build ankisyncd (not sure what that internal error is about, but let's try to have it build again...) EDIT: looks ok |
| configFile = toml.generate "ankisyncd.conf" { | ||
| listen = { | ||
| host = cfg.host; | ||
| port = cfg.port; | ||
| data_root = stateDir; | ||
| auth_db_path = authDbPath; | ||
| session_db_path = sessionDbPath; | ||
|
|
||
| base_url = "/sync/"; | ||
| base_media_url = "/msync/"; | ||
| }; | ||
| }); | ||
| paths.root_dir = stateDir; | ||
| # encryption.ssl_enable / cert_file / key_file | ||
| }; |
There was a problem hiding this comment.
Perhaps be nice to have an RFC42-like settings attr for additional settings.
There was a problem hiding this comment.
I focused on migrating equivalent functionality but I guess that might be useful for someone at some point, yes... And it's trivial now we use toml.generate. I guess I can add that tonight.
At this rate though I'd say I want some nixos/test/ankisync.nix first and I'm not sure that'll end well, but I'll leave that for a much later future me; I have no idea how to start the anki client interactively to test sync and don't want to get started with GUI testing :P
There was a problem hiding this comment.
Some basic smoke tests would already be pretty good; whether it runs at all or can answer some basic requests.
There was a problem hiding this comment.
I had a quick look at the anki protocol and it's a real monster (some binary data sent as post to get a key, then some more binary data including the key to get stats like last modfcations etc.
Can probably just replay something straight out of tcpdump through curl but I have no idea how fragile that'll be, I'll just slack for now.
Also didn't do free form yet -- I didn't remember how to integrate existing felds in the free form nicely and ran out of time for today; might do it later but imo it's orthogonal enough that we don't need to do it now (perfect is the enemy of good etc); I'd rather do in a later PR to keep things separate.
There was a problem hiding this comment.
Either mkMerge (mkDefault the defaults) or use the built-in update operator (//). First would probably be cleaner.
Not a hard requirement; I won't block on this.
|
module aside I think I've fixed all your remarks -- thanks for reminding me of runCommand, it's quite cleaner than stdenvNoCC! |
I've built it but not tested yet (system rebuilding with it); will confirm tomorrow but not expecting any problem. release notes says it allows modifying users dynamically so I assume it was slurping the sqlite db on service start previously, and now queries it when appropriate... If you don't hear from me in ~8 hours please assume the service works (that or a bus ran into my bedroom) |
the current ankisyncd server is obsolete and does not work with modern anki clients, the new version of the ankisyncd python server might work but is hard to build
Unfortunately the config is not compatible; data itself looks like it was compatible from an up to date python ankisyncd but I wouldn't assume anything with the older service (which didn't work for me with either ankidroid or recent desktop version)
- remove old ankisyncd python package - rename new rust package so it's a drop in replacement
|
(rebased again to fix release note merge conflict again; and I can confirm the minor update works without problem by now...) |
There was a problem hiding this comment.
This won't get any better by just sitting here, this is ready for unstable and I trust @martinetd to fix issues that crop up.
Could you make an announcement in https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/?
|
Thank you!
Done |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/27 |
|
A heads up that the developer of this appears to have abandoned it, and it will fail to work with the next Anki release due to minor protocol changes. |
Thanks for the heads up! That's very nice of you. Would you be able to confirm a bit more precisely what protocol change this is about? A quick search on anki commits only turned up ankitects/anki#2329 but I believe the anki-sync-server-rs is already on top of this, and I didn't see any other commit with 'protocol' in the message either merged recently or in recent PRs. afaiu this (anki-sync-server-rs) appears to be using anki itself to do a bulk of the work, so if just rebasing ought to suffice then I'll be happy to keep rebasing the code for the forseeable future, but it'd help to understand what changed exactly -- I'm sure you've taken compatibility with older clients in consideration but this has been a problem in the past so I'd like to check that too. |
|
Typically, a newer server will work with older clients, but newer clients will not work with an older server. The change I was referring to is ankitects/anki#2647. I'm afraid I'm not keen on adding a dependency on SQLite. But one of the issues raised above has been addressed - since 2.1.66, the built-in sync server can be built without Python. |
|
Thanks for the links and confirmation! My incomplete understanding makes me think a quick rebase should work and I've been able to build just fine, so I'll test this over the next weekend and open a PR to anki-sync-server-rs to update / update this package here after confirming it works.
This is great -- if this had been around back in May I might have given up on compatibility with the old modules; I'd be much less reluctant to use this server than the full bundle. I'm honestly split 50/50 between how much pain it'll be to update in the long run and wanting to keep compatibility with the original anki-sync-server module sqlite db... As long as the anki-sync-server-rs stays this simple to update I guess I can take up ownership for a while, and we can break compat when that ship sails? (BTW I'll be interested in further notices when the protocol gets updated again, but it'd be selfish to ask you to ping me everytime -- if you have a preference for this (issues on the anki-sync-server-rs project?) I'll be happy to add a watch somewhere) |
It wasn't particularly complicated. :-) I'm afraid I can't guarantee I'll remember to update you every time there's a breaking change, so if you wish to keep using anki-sync-server-rs, I'd recommend you check it's still functional after each Anki release. There's at least one more minor protocol change that's likely to land in the next few releases, related to config handling. |
|
@martinetd I was pushing for the switch to the official version before because I was concerned about the longevity of the anki-sync-server-rs project. Now that it is abandoned I feel it's one more argument in favour of switching to using the official version. Otherwise it seems like you are taking on responsibility for maintaining that project going forward which seems like it might be more work long-term than making the switch. From @euank 's comment in this thread:
And @dae 's comment in this thread:
I wonder if there have been sufficient simplifications to building and using the built-in sync server that you might also agree that it's a better option at this point. Looking to hear your opinion on that as I'm kind of far removed from using sync functionality at this point. |
|
Yes, I agree the simplifications on anki side are great -- building a standalone anki-sync-server package from anki sources ought to be very simple and would get a working server without all the anki dependencies I was complaining about earlier this year. Now we're here though it's a bit of a sunk time fallacy problem: we have something working that keeps module compatibility through Basically it boils down to a bit of my time vs. compatibility with existing setups, which we have no way of knowing how many users would be impacted (just because the old python sync server wouldn't be working with new client doesn't mean people weren't using it with older clients... I was one of them) But we really need to make sure the data itself (cards and media) is compatible otherwise it's better to do this in two steps (1/ keep data on server (nixos module config unchanged, anki-sync-server-python > anki-sync-server-rs), upgrade clients; 2/ keep clients, switch server to anki main server and resync from client to preserve data). Since I haven't verified that I cannot say anything really, but I'm probably am too involved in this to make a fair decision anyway. |
|
I haven't taken the time to build anki's master branch, but at least my rebased ankisyncd still works with current anki releases so I've opened ankicommunity/anki-sync-server-rs#76 to prepare this side. Unfortunately I can't get it to build in nixpkgs (tested building manually)... And anki will have the same problem whenever someone tries to update it, because our cargo vendoring script makes a link for each crate we depend on as $crate-$version and the latest anki depends on two different csv-core v0.1.10: So we'll either need to patch anki to depend on another version of csv (had a look at the anki patch and we can't drop it, but we can just have it change the csv-core version so it doesn't collide), or teach buildRustPackages' cargo-vendor-dir to deal with duplicate crates name/version pairs if one is a git repo... So, yeah, this is more work than expected but work we'll have to do for anki at some point anyway so my opinion hasn't really changed; I'm done for this weekend but I'll eventually have another look if no-one beats me to it. |
Description of changes
the current ankisyncd server is obsolete and does not work with modern anki clients,
the new version of the ankisyncd python server might work but is hard to build
Things done
./result/bin/)TODO:
cc @euank @Atemu (worked on anki recently) @matt-snider (old ankisyncd maintainer)
interestingly the service has no maintainer...