Move to libzim7 #72

kelvinhammond · 2021-12-22T16:17:39Z

Fixes #69
Fixes #79

This is a work in progress. The test script is what I'm using to test things as I develop, I'll add proper tests later. Once I figure out the threading issues I'll remove / cleanup the code.

Let me know if you want the API changed. I went with accessor methods for some of the C++ class methods because that is the current javascript style. This code could be dried out with all the try catch stuff but then it ends up looking a bit unwieldy so its not dry for readability and flexibility in this way.

kelson42 · 2021-12-22T17:42:52Z

@kelvinhammond Thank you for your PR. Should I try to build it and see how we could use it with mwoffliner?

kelvinhammond · 2021-12-22T17:52:40Z

@kelvinhammond Thank you for your PR. Should I try to build it and see how we could use it with mwoffliner?

@kelson42 Its not quite ready yet, the creator doesn't work because there are some issues with accessing the javascript objects via a thread. I'm working on figuring that out. Single threaded might work but its missing some functions such as add illustration etc.

kelson42 · 2022-01-05T08:57:58Z

@kelvinhammond FYI We have also made a new maintenance release of libzim, the version 7.1.0 https://download.openzim.org/release/libzim/. Nothing really important for you, but please use it if possible. Here is the changelog https://github.com/openzim/libzim/blob/master/ChangeLog

kelson42 · 2022-01-28T05:41:48Z

@kelvinhammond Any news on this PR? Is there anything which can be done to help you to make it ready for review?

kelvinhammond · 2022-01-29T02:15:33Z

@kelson42 Sorry about the delay, I've been busy with life. I'll get started on this gain this weekend and hopefully finish this up.

kelvinhammond · 2022-04-13T04:17:04Z

Rebase from master and force pushed with lease to this branch.

.editorconfig

CPPLINT.cfg

kelvinhammond · 2022-04-13T04:18:39Z

package.json

@@ -1,13 +1,16 @@
 {
  "name": "@openzim/libzim",
+  "main": "dist/index.js",
+  "types": "dist/index.d.js",
  "version": "2.4.4",
  "description": "Libzim bindings for NodeJS",
  "scripts": {
    "clean": "rm -rf dist build/native/build",
    "tsc": "tsc",


We could probably remove this, no longer required, just use src/index.js directly. Typescript applications can automatically source the src/index.d.ts that is provided in the types config here. Tests are in typescript.

No opinion on this...

package.json

tsconfig.json

kelvinhammond · 2022-04-13T04:23:18Z

@kelson42 Can someone review this? I've basically rewritten the library again so that it supports the new libzim7 architecture. I'll updated mwoffliner after this has been merged and released.

codecov · 2022-04-13T04:44:14Z

Codecov Report

Merging #72 (6c945ae) into master (b44a29f) will not change coverage.
The diff coverage is n/a.

❗ Current head 6c945ae differs from pull request most recent head b4db7ca. Consider uploading reports for the commit b4db7ca to get more accurate results

@@            Coverage Diff            @@
##            master       #72   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kelvinhammond · 2022-04-13T04:48:58Z

Not sure how to fix the CI builds. Some of the builds use node 12 which is end of life this month. The ones that use node 14x+ pass. Can someone update those?

Codefactor is currently outdated and does not currently align with the CPPLint.cfg in the project for some reason.

kelson42 · 2022-04-13T05:21:10Z

@kelvinhammond ~~If Nodejs12 makes difficulties we can just remove it, see .github/workflows/ci.yml~~ I will, you will just need to rebase.

kelson42 · 2022-04-13T05:33:24Z

@ke

@kelvinhammond If Nodejs12 makes difficulties we can just remove it, see .github/workflows/ci.yml I will, you will just need to rebase.

Done, and all the CI pass, so if something does not anymore with this PR. This is probably a regression.

BTW, we have meanwhile a libzim7.2.0 released at https://download.openzim.org/release/libzim/ and you can also anytime test with the cutting edge nightly at https://download.openzim.org/nightly/ (a few improvements have been done after your reported a problem and might be useful for you). If necessary, we could consider a patch release "just for you" getting the latest codebase for node-libzim 2.5.0

kelvinhammond · 2022-04-13T05:46:53Z

I'll make that push / commit soon then

kelvinhammond · 2022-04-16T04:48:12Z

Its ready

kelson42

@kelvinhammond So great to read we near us to the end of this PR! But before diving in details, my first points are:

What about the macos CI, why does it fail?
We should also look to the Codefactor fail (but probably far easier to fix).
The branch should be rebased on origin/master

kelvinhammond · 2022-04-17T18:01:30Z

@kelson42 It says node 14x but then I see this gyp info using [email protected] so I'm not sure what's wrong with the build

kelson42 · 2022-05-26T19:00:06Z

I also updated the CPPLINT.cfg to allow for #pragma once which should be acceptable as a header guard.

It seems to be a weakness in cpplint! cpplint/cpplint#65

kelvinhammond · 2022-06-08T00:31:27Z

Still waiting on the node-addon-api team for a fix, not sure when they'll have something. Any ideas on what to do next?

kelson42 · 2022-06-08T04:43:42Z

@kelvinhammond What is exactly the impact of the nodejs "bug" in mwoffliner? Is that aporadic, systematic?

kelvinhammond · 2022-06-08T04:48:58Z

@kelson42 The issue happens on pretty much every run, it is reproducible.

kelson42 · 2022-06-08T09:00:20Z

@kelvinhammond Would that be wise/workable to start a PR on mwoffliner to adapt the code to new libzim7?

kelvinhammond · 2022-06-09T02:20:17Z

I've already started looking at mwoffliner

kelson42 · 2022-06-09T04:26:34Z

@kelvinhammond What about merging this PR and opening a dedicated ticket for the crash scenario? This would make it easier to move on I believe.

kelvinhammond · 2022-06-09T04:36:12Z

The test-mem-leak test does not pass because of this and also will not work with larger zim files because of this bug.

kelvinhammond · 2022-06-09T05:22:51Z

@kelson42 @mgautierfr Does this seem normal for speed on an ssd laptop?

|----------| 6438/1000000 0% [elapsed: 02:03, left: 34327777:26:04, 52.10 iters/s]
Default settings, clustersize 2048, indexing on with "en";

kelson42 · 2022-06-30T05:20:05Z

@kelvinhammond I'm not sure to understand the question. Where this log comes from. What has been done to get it?

kelvinhammond · 2022-06-30T06:37:49Z

@kelvinhammond I'm not sure to understand the question. Where this log comes from. What has been done to get it?

I was testing libzim calls and it was doing about 52 iterations per second which seemed kinda slow. I wanted to know if this was normal or if there was a bottleneck somewhere.

kelson42 · 2022-06-30T11:19:32Z

@kelvinhammond Unfortunately I have no opinion on this

mgautierfr · 2022-07-11T12:18:12Z

Iterations on what ?
Reading entries ? You read only 52 entries per second ?
Or writing ?

In any case, it seems a bit slow yes.

kelvinhammond · 2022-07-19T03:22:06Z

I seem to be stuck with the nodejs bug at the moment, not sure what I should do from here.

nodejs/node-addon-api#1174

kelson42 · 2022-07-19T03:51:56Z

@kelvinhammond Yes, not easy. I would recommend to push on the mwoffliner front in the meantime, so we will be ready there too when this bug will be fixed.

kelson42 · 2022-08-12T12:40:29Z

@kelvinhammond libzim8.0.0 has been published.

- getContentProvider has replaced contentProvider object - getIndexData has replaced indexData similar to getContentProvider

kelvinhammond · 2022-08-13T05:03:05Z

@kelson42

Rebased with origin/master
Updated dependencies
Updated to libzim 8.0.0
Removed lzma compression.
I've updated the code so that file and string items work, the custom item however does not work with the makeLargeZim.js . We can fix that later (or nodejs can fix it). The custom item does work but it crashes after about 10k or so with lots of threads.

I think this PR is ready to go, what do we need to do next?

kelson42 · 2022-08-13T09:14:42Z

@kelvinhammond Agree with you, but before merging, can you please open a ticket with a clear description of the problem/regression please?

kelvinhammond · 2022-08-16T06:12:08Z

@kelson42 issue here #80

kelson42

@kelvinhammond Thank you so much for the work on this big and crucial PR

kelvinhammond requested review from rgaudin and kelson42 December 22, 2021 16:17

kelson42 requested a review from mgautierfr December 22, 2021 16:23

kelson42 mentioned this pull request Jan 28, 2022

Re-architecture the node binding #36

Open

mgautierfr mentioned this pull request Apr 6, 2022

Correctly detect the number of article for zim version <= 6 kiwix/libkiwix#743

Merged

kelvinhammond changed the title ~~Initial commit of work on move to libzim7~~ Move to libzim7 Apr 13, 2022

kelvinhammond marked this pull request as ready for review April 13, 2022 03:56

kelvinhammond force-pushed the move_to_libzim7 branch from 6683281 to 5a8c7c2 Compare April 13, 2022 04:16

kelvinhammond commented Apr 13, 2022

View reviewed changes

.editorconfig Show resolved Hide resolved

kelvinhammond commented Apr 13, 2022

View reviewed changes

CPPLINT.cfg Show resolved Hide resolved

kelvinhammond commented Apr 13, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

kelvinhammond commented Apr 13, 2022

View reviewed changes

tsconfig.json Show resolved Hide resolved

kelvinhammond force-pushed the move_to_libzim7 branch from 26e3412 to 229dbf8 Compare April 13, 2022 04:38

kelson42 requested changes Apr 17, 2022

View reviewed changes

Move to libzim7

6486cfd

Bump node version to 7.2.2

e41d69d

Updated dependencies

555bf3e

Fixes #79: Remove lzma compression option

b66d6cb

kelvinhammond added 2 commits July 19, 2022 01:13

Fix tests for compression options count

2d6cd8c

Update dependencies

abbb0f3

kelvinhammond added 2 commits August 12, 2022 22:12

Updated to libzim8 and change getContentProvider

36f7f63

- getContentProvider has replaced contentProvider object - getIndexData has replaced indexData similar to getContentProvider

Don't wrap FileItem and StringItem

b4db7ca

kelvinhammond mentioned this pull request Aug 16, 2022

Segmentation Fault when using a custom item #80

Open

kelson42 approved these changes Aug 17, 2022

View reviewed changes

kelson42 merged commit 82b9225 into master Aug 17, 2022

kelson42 deleted the move_to_libzim7 branch August 17, 2022 07:38

Move to libzim7 #72

Move to libzim7 #72

Conversation

kelvinhammond commented Dec 22, 2021 • edited by kelson42 Loading

kelson42 commented Dec 22, 2021

kelvinhammond commented Dec 22, 2021

kelson42 commented Jan 5, 2022

kelson42 commented Jan 28, 2022

kelvinhammond commented Jan 29, 2022

kelvinhammond commented Apr 13, 2022

kelvinhammond Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

kelson42 Apr 16, 2022

Choose a reason for hiding this comment

kelvinhammond commented Apr 13, 2022

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

kelvinhammond commented Apr 13, 2022

kelson42 commented Apr 13, 2022 • edited Loading

kelson42 commented Apr 13, 2022

kelvinhammond commented Apr 13, 2022 • edited Loading

kelvinhammond commented Apr 16, 2022

kelson42 left a comment • edited Loading

Choose a reason for hiding this comment

kelvinhammond commented Apr 17, 2022

kelson42 commented May 26, 2022

kelvinhammond commented Jun 8, 2022

kelson42 commented Jun 8, 2022

kelvinhammond commented Jun 8, 2022

kelson42 commented Jun 8, 2022

kelvinhammond commented Jun 9, 2022

kelson42 commented Jun 9, 2022

kelvinhammond commented Jun 9, 2022

kelvinhammond commented Jun 9, 2022

kelson42 commented Jun 30, 2022

kelvinhammond commented Jun 30, 2022

kelson42 commented Jun 30, 2022

mgautierfr commented Jul 11, 2022

kelvinhammond commented Jul 19, 2022

kelson42 commented Jul 19, 2022

kelson42 commented Aug 12, 2022

kelvinhammond commented Aug 13, 2022

kelson42 commented Aug 13, 2022

kelvinhammond commented Aug 16, 2022

kelson42 left a comment

Choose a reason for hiding this comment

kelvinhammond commented Dec 22, 2021 •

edited by kelson42

Loading

kelvinhammond Apr 13, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

kelson42 commented Apr 13, 2022 •

edited

Loading

kelvinhammond commented Apr 13, 2022 •

edited

Loading

kelson42 left a comment •

edited

Loading