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

Bump sdcv to 0.5.4 + a couple of PRs to fix it #1516

Merged
merged 1 commit into from
Sep 14, 2022
Merged

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Sep 14, 2022

I'm not sure if it was actually broken for me before, but at least this one works on both Kobo & Android ;).

@pazos's hunch was nearly perfectly spot-on (my working theory is that it's the sizeof comparisons that were broken, quite likely because off64_t vs. ulong, and not the fstat call itself).

Split our existing patch in two, and submitted the sanest part of it upstream, because apparently I'd forgotten about it, and it happened to have been reported upstream.


This change is Reviewable

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 14, 2022

OnePlus6:/data/user/0/org.koreader.launcher.debug/files $ LD_LIBRARY_PATH=./libs ./sdcv -v                                                                                                                                                                                                                                                                              
Console version of Stardict, version 0.5.4

(Actual in-app lookups work, that was just to double-check the version ;p).

@NiLuJe NiLuJe merged commit b8a77ba into koreader:master Sep 14, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 15, 2022
NiLuJe added a commit to koreader/koreader that referenced this pull request Sep 15, 2022
Since I never actually needed to look into that data ever until today, let's just get rid of the weird debug-specific behavior.

Instead, just add a dedicated "Developer options" entry that will dump it on demand (and it'll be sorted to boot, which makes it 500% more usable).

Plus, since yesterday's change, the cache format switch between debug or not miiiight actually be crashy, so re-trigger the migration ;p.

* Includes a couple of noteworthy base bumps:

koreader/koreader-base#1516 (update to sdcv 0.5.4 + fixes pending upstream)
koreader/koreader-base#1517 (fix ffiUtil.orderedPairs with keys of mixed types)
@poire-z
Copy link
Contributor

poire-z commented Oct 8, 2022

Possibly posted in not the right sdcv PR, but I just noticing this today.
When I lookup some word, on the emulator or my Kobo, I see this logged (so, even when not in debug mode):

Unknown dictionary: Dictionnaire de lâAcadémie Française, 8ème édition (1935).
Unknown dictionary: Petit Robert 2007
Unknown dictionary: XMLittre
Unknown dictionary: Wiktionary FR-FR
Unknown dictionary: Arramooz
Unknown dictionary: English-Arabic dictionary
[...]

It's some bit you get added upstream in Dushistov/sdcv@6e36e77
and may be it is logged because of our option to specify which dicts to use or ignore.

Everything seems to work, it's just noise in the logs, but still :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 8, 2022

@poire-z: What's the commandline look like in this instance?

EDIT: i.e., do we pass an allow list or a deny list of dictionaries? (I seem to remember that we pass an allow list, but if we passed a deny list, I could just skip the warning for explicitly denied dicts in sdcv ;)).

Also, I seem to remember that we didn't/couldn't actually do anything with stderr because io.popen, and this is going to stderr, so I'm a bit puzzled as to how it actually ends up in the logs to begin with?

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 8, 2022

Okay, yeah, it's an allow list, so we're stuck handling this on our side...

I'd be inclined to leave it as-is, because logging stderr makes sense, but if not, we'd "just" need to throw stderr to /dev/null in the shell command ;).

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 8, 2022

Or... we could just patch that out in sdcv, because, while the warnings might make sense upstream, they don't really for us ;).

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.

None yet

3 participants