Updated anki-sync-server to work with the latest version of Anki #20

kalehmann · 2020-08-28T10:48:24Z

This PR tackles the following issues: #13, #14, #16 and #19.

The embedded Anki submodule was removed, anki-sync-server now supports the current version of Anki installed by pip install anki.
To support newer versions of Anki, a Nginx-Proxy to "unchunk" requests is required before the anki-sync-server
An example addon for Anki versions >= 2.1.28 is provided in the Readme
The web media test was removed, because it depended on the MediaSyncer class, which no longer exists

I tested these changes with the latest master of AnkiDroid and Anki 2.1.28. I have found no issues while syncing between these two.

Required steps before merging

Patching all tests to work with the new code
Write Anki dependency to lock file

This commit applies the fix from https://github.com/tsudoko/anki-sync-server/pull/60/files However it using a shorter version by utilizing the params attribute of the webob request. The params attribute combines the get and post params

This method was removed in a2b7a3084131f747fb476cc8a24f96a00c654859

AlexBocken · 2020-08-31T10:57:48Z

To support newer versions of Anki, a Nginx-Proxy to "unchunk" requests is required before the anki-sync-server

If I try to run the example unchunker as given in the Readme using nginx, I'm not able to run the ankisyncd server at the same time via python -m ankisyncd as this Error: OSError: [Errno 98] Address already in use get's raised.

Which seems reasonable to me as an error. Of course this could be a result of my incompetence with nginx, I'm rather new to it. Does ankisyncd.conf need to modified to use a different port? But then the proxy_pass setting in the given example nginx setup does not make sense to me anymore.

Would it be possible to update the documentation to further clarify this area for an easier implementation? (I'm already quite grateful for the example nginx config at all). My guess is that other's who will try to to implement this workaround will have similar issues if they're not too familiar with nginx.

I apologize if this is the wrong place for a comment like this, but I didn't feel like it's appropriate to raise an issue on this project and the issues feature seems to be disabled on your personal fork.

kalehmann · 2020-08-31T14:58:10Z

HI @AlexBocken, thank you for your input.

I apologize if this is the wrong place for a comment like this, but I didn't feel like it's appropriate to raise an issue on this project and the issues feature seems to be disabled on your personal fork.

Your comment is suited perfectly fine here as it describes a problem introduced by this pull request 😄

During development I have used docker and both services (ankisyncd and Nginx) ran on different containers. Therefore they could both listen at same port (inside their respective containers). As you have already found out, this is not possible when both services run on your local machine.

I have updated the README file and further clarified how Nginx should be configured when both services run on the same machine.

AlexBocken · 2020-08-31T17:23:48Z

Thanks for the clarifications!

VikashKothary

Mate this is great work. It's all looks good to me. Thanks for all your hard work. A few quick changes and this PR should be ready to be merged.

.gitignore

README.md

src/ankisyncd/sync_app.py

README.md

AntonOfTheWoods

There is some code that looks like it is no longer needed. AFAICT, for example, there is no longer any particular use for the runHooks. The sync method is also, unless I'm mistaken, client-side code. As we now have to maintain this, it is very important not to have dead code that will just confuse future contributors. Can you give the file a clean?

AntonOfTheWoods

Are you deleting code you added in 59603b8 ? If so, can you combine the commits instead? If not, I'm lost, could you explain? :-)

AntonOfTheWoods · 2020-09-01T22:21:04Z

Thanks again for the great work @kalehmann ! Apart from a couple of minor issues it looks great. I know it's sometimes hard and a little annoying but maybe next time maybe try and have several smaller PRs to avoid changing the same parts of the code in several commits of the PR.

src/ankisyncd/full_sync.py

kalehmann · 2020-09-02T16:42:07Z

AFAICT, for example, there is no longer any particular use for the runHooks. The sync method is also, unless I'm mistaken, client-side code.

You were right, the runHook calls and the sync method served no purpose for us anymore.

As we now have to maintain this, it is very important not to have dead code that will just confuse future contributors. Can you give the file a clean?

I have removed any code related to these two points. But I still believe, that there is more unused code in this project. However I think that finding and removing these lines would better suite another PR.

This prevents the missing collation unicase error on the client

The dirty field does not exist in the media table anymore.

That way we make sure, our ServerMediaManager is used instead of the MediaManager

These hooks were only used on the client.

kalehmann · 2020-09-02T16:55:40Z

Are you deleting code you added in 59603b8 ? If so, can you combine the commits instead?

Yes, it seems like Anki desktop only accepts the downgraded database, therefore I reverted these changes. Anyways, I squashed these two commits

kalehmann · 2020-09-02T16:58:49Z

I know it's sometimes hard and a little annoying but maybe next time maybe try and have several smaller PRs to avoid changing the same parts of the code in several commits of the PR.

Yeah I know 🤔 The problem is just, that when I started working on this PR I didn't even know how big it would get. I believe splitting it into smaller changes requires a profound evaluation of all necessary changes beforehand - which I was unable to do because I knew nothing about this project.

AntonOfTheWoods · 2020-09-03T03:12:21Z

Yes, it seems like Anki desktop only accepts the downgraded database, therefore I reverted these changes. Anyways, I squashed these two commits

That's interesting, I didn't follow that up for djankiserv, it "Just Worked", and I didn't really know why :-). I definitely think that they will fully migrate to the new structure soon though, so that is definitely a TODO to test properly.

AntonOfTheWoods · 2020-09-03T03:19:19Z

I have removed any code related to these two points. But I still believe, that there is more unused code in this project. However I think that finding and removing these lines would better suite another PR.

Oh, how many times I would have liked to have got away with that at my last company... :-). The way I am managing this for importing upstream code to djankiserv is to bring in a module then write tests to get the code to do useful stuff. Then run coverage and anything that hasn't been run gets the chop. It is pretty violent and misses cases but we can always go back and get code if it really was necessary. Let's definitely plan for something like this!

AntonOfTheWoods

LGTM. There is a lot of cleanup to be done but there was before so let's get this merged and move forward from there.

kalehmann added 7 commits August 26, 2020 16:22

Removed embedded anki submodule

f7ae69f

Removed unused import

26d16b6

Ported Python Syncer from Anki source code

d0f7d05

Added missing fields to meta endpoint

bc88995

Read hostkey from GET or POST

3857f15

This commit applies the fix from https://github.com/tsudoko/anki-sync-server/pull/60/files However it using a shorter version by utilizing the params attribute of the webob request. The params attribute combines the get and post params

Removed call to load method of anki.collection.Collection

b566e32

This method was removed in a2b7a3084131f747fb476cc8a24f96a00c654859

Hide the media managers db

8358b09

kalehmann mentioned this pull request Aug 28, 2020

Your client is using unsupported sync protocol (10, supported version: 9) with latest version of Anki #13

Closed

kalehmann force-pushed the patch-issue-13 branch 2 times, most recently from c5075a9 to d3456a2 Compare August 31, 2020 14:55

VikashKothary reviewed Sep 1, 2020

View reviewed changes

VikashKothary requested a review from AntonOfTheWoods September 1, 2020 19:52

AntonOfTheWoods suggested changes Sep 1, 2020

View reviewed changes

AntonOfTheWoods reviewed Sep 1, 2020

View reviewed changes

AntonOfTheWoods reviewed Sep 2, 2020

View reviewed changes

src/ankisyncd/full_sync.py Outdated Show resolved Hide resolved

kalehmann added 10 commits September 2, 2020 18:49

Made sure to use ConfigManager in Syncer

c97a096

fix chunk in sync

93d37d6

Log sanity check errors

e18e86e

Always downgrade the database before sync

7eff381

This prevents the missing collation unicase error on the client

fix sanity check

4c09c1e

updated readme to reflect latest changes

537bbe8

Removed haveDirty check from sync code

471e3ae

The dirty field does not exist in the media table anymore.

Removed unused class MediaSyncer

c6f82e2

Marked test to fail because of missing _logChanges Method

9d67943

Updated media test to work with latest changes

94da88a

kalehmann added 20 commits September 2, 2020 18:53

Fix parent initialization of SyncCollectionHandler

75c9267

Removed test because of deprecated MediaSyncer

2c1e593

Adapted sync app test to latest changes

c5bce62

server_utils test helper works with non-expose media db

5f17eb7

Removed references to unused methods

e2e756d

mock_servers test helper works with new syncer code

0ef99f3

file_utils test helper works with new syncer code

60f12cf

Close whole collection instead of just the database

9831159

Use wrapped collection in tests

b0d57d3

That way we make sure, our ServerMediaManager is used instead of the MediaManager

Added emacs temporary files to gitignore

75c1ea0

Added anki to dependencies

9da0eb7

Further clarified configuration for Nginx proxy

70bfaa1

replaced relative with absolute import

6f29fce

Removed artifact from README

3ec37ec

Updated docs about installation of requirements

65e9bbf

Removed whole section about ankisyncd without pyaudio from the README

7deda95

Removed unused post- and prehooks

614f209

Removed runHook calls from sync code

0d8f3c6

These hooks were only used on the client.

Removed unused sync method

e9e0645

Removed explicit object parent from FullSyncManager

f510050

kalehmann force-pushed the patch-issue-13 branch from 5de429f to f510050 Compare September 2, 2020 16:54

kalehmann requested a review from AntonOfTheWoods September 2, 2020 16:58

AntonOfTheWoods approved these changes Sep 3, 2020

View reviewed changes

AntonOfTheWoods merged commit 049bb04 into ankicommunity:develop Sep 3, 2020

This was referenced Dec 27, 2020

Update dependencies #23

Closed

Remove anki submodule dependency #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated anki-sync-server to work with the latest version of Anki #20

Updated anki-sync-server to work with the latest version of Anki #20

kalehmann commented Aug 28, 2020 •

edited

Loading

AlexBocken commented Aug 31, 2020

kalehmann commented Aug 31, 2020 •

edited

Loading

AlexBocken commented Aug 31, 2020

VikashKothary left a comment

AntonOfTheWoods left a comment

AntonOfTheWoods left a comment

AntonOfTheWoods commented Sep 1, 2020

kalehmann commented Sep 2, 2020

kalehmann commented Sep 2, 2020

kalehmann commented Sep 2, 2020

AntonOfTheWoods commented Sep 3, 2020

AntonOfTheWoods commented Sep 3, 2020

AntonOfTheWoods left a comment

Updated anki-sync-server to work with the latest version of Anki #20

Updated anki-sync-server to work with the latest version of Anki #20

Conversation

kalehmann commented Aug 28, 2020 • edited Loading

Required steps before merging

AlexBocken commented Aug 31, 2020

kalehmann commented Aug 31, 2020 • edited Loading

AlexBocken commented Aug 31, 2020

VikashKothary left a comment

Choose a reason for hiding this comment

AntonOfTheWoods left a comment

Choose a reason for hiding this comment

AntonOfTheWoods left a comment

Choose a reason for hiding this comment

AntonOfTheWoods commented Sep 1, 2020

kalehmann commented Sep 2, 2020

kalehmann commented Sep 2, 2020

kalehmann commented Sep 2, 2020

AntonOfTheWoods commented Sep 3, 2020

AntonOfTheWoods commented Sep 3, 2020

AntonOfTheWoods left a comment

Choose a reason for hiding this comment

kalehmann commented Aug 28, 2020 •

edited

Loading

kalehmann commented Aug 31, 2020 •

edited

Loading