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

Adapt wrapper to new API of libzim/libkiwix #23

Merged
merged 32 commits into from
Feb 9, 2023
Merged

Adapt wrapper to new API of libzim/libkiwix #23

merged 32 commits into from
Feb 9, 2023

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Dec 20, 2022

This PR is the adaptation of the wrapper itself. Change in the build system is PR #22

The current version mainly "remove" existing wrapper (not compile it) and start a new wrapper (libzim only for now) using ground fondation made previously.

TODO:

  • Add wrapper around libkiwix (reusing previous wrapper)
  • Finish libzim wrapper (search, iteration on items, ..)
  • Test things

@mgautierfr mgautierfr mentioned this pull request Dec 20, 2022
@mgautierfr mgautierfr changed the title Addapt wrapper to new API of libzim/libkiwix Adapt wrapper to new API of libzim/libkiwix Dec 20, 2022
@mgautierfr mgautierfr force-pushed the adapt_wrapper branch 2 times, most recently from 9472116 to c32bc00 Compare January 11, 2023 15:28
Base automatically changed from adapt_buildsystem to main January 11, 2023 16:35
@mgautierfr
Copy link
Member Author

I think we can start review this PR.

Few information about how wrapping is done (please be carruful about that when reviewing) :

From the java definition (lib/src/main/java/org/kiwix/libzim/Archive.java) we (javac) generate a c++ header with function declarations corresponding to java methods. (String Archive::getFilename() => JNIEXPORT jstring JNICALL Java_org_kiwix_libzim_Archive_getFilename(JNIEnv*, jobject)).
Then we have to implement the functions in c++ files. However, nothing check that we have actually implemented the declared function. If we forget a implementation, no warning at all. This will fail when java code try to call the java method because of undefined symbol. And as we don't have unit test for now, we have to be carreful about that.
This is the same in case of typos: In one side we define a function never called, on the other side java will try to call a undefined function. I have made a lot of use of macro to prevent that but few things may have missed my attention.

The same way, creating wrapper is made by creating java object, and to do so, we have to "search" for the java class.
This is made by env->FindClass("org/kiwix/libzim/Entry"). If there is a typos here (or the wrong class name), we will have a error only at runtime. (And the same when searching for method/field in a class).
All those thing must be tested by unittest to write (and that is the purpose of those test, we don't want them to test the libzim/libkiwix itself) but for now we must rely on our eyes.

@mgautierfr mgautierfr marked this pull request as ready for review January 24, 2023 13:18
The cpp method name is `getMimetype`, not `getMimeType`.
As `update` is overloaded, the jni method name include the argument type.
And we have changed the type of one version of `update`, so we need
to change its name.
A lot of methods are overloaded, so we must include the argument type
in the name.
This make the JNI wrapping *somehow* NOT threadsafe.

Few things are threadsafe "by nature":
- A lot of native method in libzim are threadsafe.
- Wrapping internal are threadsafe (shared_ptr).

What is not threadsafe is accessing the SAME java object from different
thread. But accessing the same wrapped cpp object using two different java
wrapper in two different thread is ok (assuming that the called cpp method
is threadsafe itself).
@mgautierfr
Copy link
Member Author

mgautierfr commented Jan 24, 2023

Exception handling (#2 #24 ) is not made in this PR.
It should not be complicated to add it correctly but it would be a lot of redondant (and boring) changes in all the code. I suggest we do that in other PR.

Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @mgautierfr, Since i'm not a c++ developer. But i have reviewed android related code. it succesfully generated the .so file.
and it's need lots of refactoring in android code according to latest changes in wrapper. i have stared refactoring code on android side.

  • I have not reviewed testing part yet. Because its broken right now(after adapting the file structure and latest wrapper) and i think you have not work on that yet.
  • Can you please check why codefactor is failing.

I have a questions.

  • We have removed (JNIKiwixBool.java , JNIKiwixInt.java , JNIKiwixString.java). What is the alernative for these classes?

lib/src/main/java/org/kiwix/libkiwix/JNIKiwix.java_ Outdated Show resolved Hide resolved
@mgautierfr
Copy link
Member Author

I have not reviewed testing part yet. Because its broken right now(after adapting the file structure and latest wrapper) and i think you have not work on that yet.

Indeed, I've not work on that. I think we should do it together. I'm not aware of testing tools in java.

We have removed (JNIKiwixBool.java , JNIKiwixInt.java , JNIKiwixString.java). What is the alernative for these classes?

There is no alternative. Those classes where use as out parameter in the old API (you pass a JNIKiwixBool to a method, and it will be modified to pass you a value). With the new api, you don't need them, so I remove them.

@mgautierfr
Copy link
Member Author

Can you please check why codefactor is failing.

Codefactor complain about the finalizer method which is deprecated.
The solution seems to use a Cleaner (see https://stackoverflow.com/questions/64459003/usage-of-java-lang-ref-cleaner-as-alternative-to-object-finalize).
I see how to do it but it will involve some refactoring. I will work on this but the easiest for now is to ignore the CodeFactor issues.

@kelson42
Copy link
Contributor

@mgautierfr Don't forget the bookmarks API

@mgautierfr
Copy link
Member Author

Codefactor complain about the finalizer method which is deprecated.
The solution seems to use a Cleaner (see stackoverflow.com/questions/64459003/usage-of-java-lang-ref-cleaner-as-alternative-to-object-finalize).
I see how to do it but it will involve some refactoring. I will work on this but the easiest for now is to ignore the CodeFactor issues.

This is WIP in #25

@mgautierfr
Copy link
Member Author

I think we are good now. At least as long as we don't try to run it.
The next step is to work on testing to be sure that everything is wrapped correctly but we will do it in another PR.
Except if there is something obviously wrong, I suggest we merge this PR and fix remaining issue in other PRs.

@mgautierfr
Copy link
Member Author

It seems I was wrong when I told JNIKiwix.java is fixed.
It indeed correctly compiles with ./gradle build but the generation of native header fails.
I suspect that it this level, java don't know about the dependencies. @MohitMaliFtechiz your help is welcome here as gradle is more in your competences.

@MohitMaliFtechiz
Copy link
Collaborator

MohitMaliFtechiz commented Feb 9, 2023

It seems I was wrong when I told JNIKiwix.java is fixed. It indeed correctly compiles with ./gradle build but the generation of native header fails. I suspect that it this level, java don't know about the dependencies. @MohitMaliFtechiz your help is welcome here as gradle is more in your competences.

hi @mgautierfr ,
i have debug it and find JNIKiwix.java is containing android spacific code (like context and ReLinker library. that's why javac command is not recognise this), we don't need to generate header file for this. so we should exclude this file while generating header files.

you can try this code snippet in lib/gradle file

task generateHeaderFilesFromJavaWrapper(type: Exec) {
    workingDir "${projectDir}/src/main/java/org/kiwix/"
    commandLine 'bash', '-c', "javac -h ${buildDir}/include/javah_generated/ -d ${buildDir}/libzim/ libzim/*.java ${getLibkiwixFiles()}"
}

String getLibkiwixFiles() {
    return "${projectDir}/src/main/java/org/kiwix/libkiwix/Book.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Bookmark.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Filter.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/JNIICU.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Illustration.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/JNIKiwixException.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Library.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Manager.java " +
            "${projectDir}/src/main/java/org/kiwix/libkiwix/Server.java"
}

@mgautierfr mgautierfr force-pushed the adapt_wrapper branch 2 times, most recently from a15de34 to 8ce4ed1 Compare February 9, 2023 13:46
@mgautierfr
Copy link
Member Author

Thanks @MohitMaliFtechiz. Now compilation is ok.
I think we can merge now and attack the testing part in another PR.

@kelson42 kelson42 merged commit 4b5cc44 into main Feb 9, 2023
@kelson42 kelson42 deleted the adapt_wrapper branch February 9, 2023 16:28
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.

3 participants