kiwix / java-libkiwix

Libkiwix binding for Java & Kotlin
https://central.sonatype.com/artifact/org.kiwix/libkiwix
GNU General Public License v3.0
3 stars 4 forks source link

Adapt wrapper to new API of libzim/libkiwix #23

Closed mgautierfr closed 1 year ago

mgautierfr commented 1 year ago

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:

mgautierfr commented 1 year ago

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 commented 1 year ago

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.

mgautierfr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@mgautierfr Don't forget the bookmarks API

mgautierfr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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