JNI interface to opening ZIM archives (including embedded ones) by fd #429

veloman-yunkan · 2020-11-29T19:00:37Z

Will fix #423

This PR exposes in the libkiwix java wrapper the functionality introduced by openzim/libzim#449.

The changes were tested in native_mixed build mode via the src/wrapper/java/org/kiwix/testing/compile_and_run_test.sh script. Achieving successful compilation in that mode required some tweaks. In particular,

(cd BUILD_native_mixed/INSTALL/; ln -s ../../BUILD_native_static/INSTALL/include)
(cd BUILD_native_mixed/INSTALL/; meson configure -Dwrapper=java)
The fatal error: jni.h: No such file or directory compilation error was worked around as follows (I realize that it is just a hack but it worked):

cp -s /usr/lib/jvm/java-11-openjdk-amd64/include/jni.h BUILD_native_mixed/INSTALL/include
cp -s /usr/lib/jvm/java-11-openjdk-amd64/include/linux/jni_md.h BUILD_native_mixed/INSTALL/include

Java unit tests were run as follows:

cd SOURCE/kiwix-lib
src/wrapper/java/org/kiwix/testing/compile_and_run_test.sh ../../BUILD_native_mixed/kiwix-lib/src/wrapper/java/kiwixlib.jar ../../BUILD_native_mixed/kiwix-lib/src

All changes in c++ code are covered via the java unit tests which are not run by the CI, hence the failure of the codecov/patch check.

stale · 2020-12-20T22:19:47Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

mhutti1 · 2021-01-14T23:58:27Z

This looks like it is working for me on the Android side. Is it ready to merge and publish?

kelson42 · 2021-01-15T08:01:09Z

@veloman-yunkan Can we merge the two PRs in libzim and libkiwix soon? Before libzim 7.0.0 release?

veloman-yunkan · 2021-01-15T08:43:43Z

@veloman-yunkan Can we merge the two PRs in libzim and libkiwix soon? Before libzim 7.0.0 release?

Yes. I will submit both PRs for review over the weekend.

stale · 2021-01-23T05:52:44Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

kelson42 · 2021-02-03T16:36:47Z

@veloman-yunkan Do you have still a lot to do on this PR (now that the libzim sibling PR has been merged)?

veloman-yunkan · 2021-02-03T21:58:53Z

@veloman-yunkan Do you have still a lot to do on this PR (now that the libzim sibling PR has been merged)?

I don't think so

codecov · 2021-02-04T20:14:50Z

Codecov Report

Merging #429 (839fc10) into master (971374e) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
- Coverage   59.73%   59.62%   -0.12%     
==========================================
  Files          50       50              
  Lines        3410     3418       +8     
  Branches     1732     1736       +4     
==========================================
+ Hits         2037     2038       +1     
- Misses       1370     1377       +7     
  Partials        3        3

Impacted Files	Coverage Δ
include/entry.h	`100.00% <ø> (ø)`
include/reader.h	`100.00% <ø> (ø)`
src/reader.cpp	`47.01% <0.00%> (-1.36%)`	⬇️
src/tools/stringTools.cpp	`61.65% <0.00%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 971374e...839fc10. Read the comment docs.

kelson42 · 2021-02-05T11:17:35Z

@veloman-yunkan meson.build needs to be updated to require libzim7.0.0+

veloman-yunkan · 2021-02-07T11:42:47Z

@veloman-yunkan meson.build needs to be updated to require libzim7.0.0+

@kelson42 Done, but we better bring it into master via a separate PR.

kelson42 · 2021-02-07T11:47:25Z

@veloman-yunkan Thx, up to you, you might be right. Fell free to open a new PR. Just wanted that it does not get forgotten.

veloman-yunkan · 2021-02-07T16:26:07Z

@veloman-yunkan Thx, up to you, you might be right. Fell free to open a new PR. Just wanted that it does not get forgotten.

@kelson42 Version bump moved into a separate PR #452

The kiwixlib java wrapper unit test can be run manually via the src/wrapper/java/org/kiwix/testing/compile_test.sh script. The test ZIM files in src/wrapper/java/org/kiwix/testing were created using the create_test_zimfiles. They must be updated/re-generated and committed in git whenever their source data or the create_test_zimfiles script changes. Note: small.zim.embedded is not used at this point, it was created for testing the enhancement coming in a few commits.

... and a corresponding unit test

Opening ZIM archives by file descriptor (as well as embedded ZIM archives) is not supported under Windows.

kelson42 · 2021-02-10T13:20:40Z

We have to bump to a major release name. I have rebased on origin/master.

kelson42 · 2021-02-21T11:46:49Z

@mgautierfr We are expecting your review here to carry on.

mgautierfr

We are good.

As we will merge this before #446, we must not remove the srand(time(nullptr));. But we need to remove it in #446 (which was missing).

mgautierfr · 2021-02-22T16:50:27Z

src/reader.cpp

+{
+
+  /* initialize random seed: */
+  srand(time(nullptr));


Now that we implement random in libzim (openzim/libzim#476) this may be not necessary.

I see some inconsistency here. #446 doesn't drop the call to srand() in the other (pre-existing) constructor of kiwix::Reader. If that was an omission, then #446 can be amended for all three constructors of kiwix::Reader. Otherwise, if one constructor of kiwix::Reader contains that code, the others should do the same.

Yes, see my global comment in the PR. We should remove it in #446.

mgautierfr · 2021-02-22T17:04:27Z

We have to bump to a major release name

Yes, it is a pity that a change in the wrapper (Pair->DirectAccessInformation) force us to dump the major version of the whole kiwix-lib but we have to.
(Moving the wrapper in a separated repository as describe in #430 would help here)

kelson42 · 2021-02-26T06:15:02Z

I guess we could merge that one now?

veloman-yunkan · 2021-02-26T07:59:35Z

I guess we could merge that one now?

In fact, I was wondering why it wasn't merged

mgautierfr force-pushed the libzim_next branch from 59c70e4 to 1a5a2e7 Compare December 2, 2020 11:16

Base automatically changed from libzim_next to master December 3, 2020 08:11

veloman-yunkan force-pushed the open_zimfile_by_fd branch 2 times, most recently from d96433c to e1736b0 Compare December 5, 2020 20:25

stale bot added the stale label Dec 20, 2020

stale bot removed the stale label Jan 14, 2021

veloman-yunkan mentioned this pull request Jan 19, 2021

Support for opening a ZIM file by file descriptor openzim/libzim#449

Merged

stale bot added the stale label Jan 23, 2021

stale bot removed the stale label Feb 3, 2021

veloman-yunkan force-pushed the open_zimfile_by_fd branch from 526a4cc to 37d6ee3 Compare February 4, 2021 20:12

veloman-yunkan force-pushed the open_zimfile_by_fd branch 3 times, most recently from 0974192 to fdb4e9c Compare February 7, 2021 11:15

veloman-yunkan changed the title ~~[WIP] Open zimfile by fd~~ JNI interface to opening ZIM archives (including embedded ones) by fd Feb 7, 2021

veloman-yunkan marked this pull request as ready for review February 7, 2021 11:39

veloman-yunkan requested a review from mgautierfr February 7, 2021 11:40

veloman-yunkan force-pushed the open_zimfile_by_fd branch from fdb4e9c to 8f5ee8e Compare February 7, 2021 16:09

veloman-yunkan added 9 commits February 10, 2021 14:19

Renamed the java wrapper unit test runner script

e40827f

Added testReader unit-test for the java wrapper

98d69ef

JNIKiwixReader ctor taking a file descriptor

4d23e44

... and a corresponding unit test

JNIKiwixReader can open an embedded ZIM archive

9cdf7a4

Android's java.io.FileDescriptor is different

e028bcb

Java wrapper test checks favicon.png too

5a99634

Renamed org.kiwix.kiwixlib.{Pair->DirectAccessInfo}

7a465e6

Testing of JNIKiwixReader.getDirectAccessInformation()

5a8b825

Fixed the Windows build

839fc10

Opening ZIM archives by file descriptor (as well as embedded ZIM archives) is not supported under Windows.

kelson42 force-pushed the open_zimfile_by_fd branch from 8f5ee8e to 839fc10 Compare February 10, 2021 13:20

mgautierfr approved these changes Feb 22, 2021

View reviewed changes

kelson42 merged commit d3f2e08 into master Feb 26, 2021

kelson42 deleted the open_zimfile_by_fd branch February 26, 2021 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JNI interface to opening ZIM archives (including embedded ones) by fd #429

JNI interface to opening ZIM archives (including embedded ones) by fd #429

veloman-yunkan commented Nov 29, 2020 •

edited

Loading

stale bot commented Dec 20, 2020

mhutti1 commented Jan 14, 2021

kelson42 commented Jan 15, 2021

veloman-yunkan commented Jan 15, 2021

stale bot commented Jan 23, 2021

kelson42 commented Feb 3, 2021

veloman-yunkan commented Feb 3, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

kelson42 commented Feb 5, 2021

veloman-yunkan commented Feb 7, 2021

kelson42 commented Feb 7, 2021

veloman-yunkan commented Feb 7, 2021

kelson42 commented Feb 10, 2021

kelson42 commented Feb 21, 2021

mgautierfr left a comment

mgautierfr Feb 22, 2021

veloman-yunkan Feb 23, 2021

mgautierfr Feb 23, 2021

mgautierfr commented Feb 22, 2021

kelson42 commented Feb 26, 2021

veloman-yunkan commented Feb 26, 2021

JNI interface to opening ZIM archives (including embedded ones) by fd #429

JNI interface to opening ZIM archives (including embedded ones) by fd #429

Conversation

veloman-yunkan commented Nov 29, 2020 • edited Loading

stale bot commented Dec 20, 2020

mhutti1 commented Jan 14, 2021

kelson42 commented Jan 15, 2021

veloman-yunkan commented Jan 15, 2021

stale bot commented Jan 23, 2021

kelson42 commented Feb 3, 2021

veloman-yunkan commented Feb 3, 2021

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

kelson42 commented Feb 5, 2021

veloman-yunkan commented Feb 7, 2021

kelson42 commented Feb 7, 2021

veloman-yunkan commented Feb 7, 2021

kelson42 commented Feb 10, 2021

kelson42 commented Feb 21, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Feb 22, 2021

Choose a reason for hiding this comment

veloman-yunkan Feb 23, 2021

Choose a reason for hiding this comment

mgautierfr Feb 23, 2021

Choose a reason for hiding this comment

mgautierfr commented Feb 22, 2021

kelson42 commented Feb 26, 2021

veloman-yunkan commented Feb 26, 2021

veloman-yunkan commented Nov 29, 2020 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading