Reorganise the gradle build script

[MohitMaliFtechiz]

Fixes #31

• In this PR we are avoiding to regenerate source files in test cases instead of this now we are generating the .so file when we are building the .aar file for android.

  • For this we have created a task buildLinuxSoFile , build task dependsOn buildLinuxSoFile (which means first buildLinuxSoFile executes and after that build task executes) .
  • buildLinuxSoFile generates the linux .so file (in build directory), which we are using in test cases.
  • We have modify the lib/src/main/cpp/CMakeLists.txt file for this , Here we have placed a condition, If it's linux target then it will generate the .so file (in build directory), If we will use the ./gradlew build command then it will generate the .aar file for android.
  • We have removed the test CMakeList file because now it's unused.

• We have removed the copyBuildKiwixSoFile gradle task because now we are directly generating the .so file in build directory.

• We have rename the generateHeaderFilesFromJavaWrapper method to generateHeaders for better readability as mention on ticket.

Important Note For now i have changed the linux .so file url (to test our new changes) https://download.openzim.org/nightly/2023-03-20/libzim_linux-x86_64-2023-03-20.tar.gz ,Because now Linux libzim tar file containing 0 byte .so file https://github.com/openzim/libzim/issues/772 , Once this issue is fixed we will remove this added url .

[codecov-commenter]

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@66ebc4e). Click here to learn what that means. Patch has no changes to coverable lines.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #32 +/- ## ======================================= Coverage ? 38.20% Complexity ? 15 ======================================= Files ? 24 Lines ? 89 Branches ? 6 ======================================= Hits ? 34 Misses ? 51 Partials ? 4 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kiwix). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kiwix)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[MohitMaliFtechiz]

@mgautierfr , Thanks for the review.

I don't see any dependency between build and generateHeader nor between test and build. It is sad that it is one of the goal of the issue.

Here, build depends on generateHeader . If generateHeader task fails then the build task will also automatically fail. If build task fails then test will also automatically fail, Because we are generating the linux .so file from buildLinuxSoFile with the build task which we are using in our test cases.

Why do you need to tell that build depends of buildLinuxSoFile ?

build task depends on buildLinuxSoFile which means first buildLinuxSoFile execute it will generate the linux .so file and after that build task execute it will generate the aar file for android.

About all the task copyLibzim*, would it be possible to move it in a function with parameters to avoid duplication. And why having differents tasks for that ? Do we have the possibility to use only the arm64 library ? If not, we don't need different tasks (and the tasks downloadLibzimSoHandHeaderFiles and unzipLibzim do all archs)

Good point, We should use downloadLibzimSoHandHeaderFiles for downloading zim files and rename unzipLibzim as unzipAndCopyLibZim to unzip and copy the header and .so files.

CMakeList.txt is a internal implementation detail of the build system. User should run gradle to build the wrapper (which will run cmake for us). They must not run cmake directly so we can remove this condition.

I'm not getting this point, but if we are generating the .aar file with build command for android, then IMO we can only generate the .aar or .so at a time.

[MohitMaliFtechiz]

About all the task copyLibzim*, would it be possible to move it in a function with parameters to avoid duplication. And why having differents tasks for that ? Do we have the possibility to use only the arm64 library ? If not, we don't need different tasks (and the tasks downloadLibzimSoHandHeaderFiles and unzipLibzim do all archs)

@mgautierfr for this i have create two new tasks (copyLibzimHeaderAndSoFiles , copyLibkiwixHeaderAndSoFiles ) we can not do unzip and copy in same task because for coping the headers and .so file we need date, so before coping we have to run checkCurrentLibkiwixDate checkCurrentLinuxLibkiwixDate these methods.

[mgautierfr]

for this i have create two new tasks (copyLibzimHeaderAndSoFiles , copyLibkiwixHeaderAndSoFiles ) we can not do unzip and copy in same task because for coping the headers and .so file we need date, so before coping we have to run checkCurrentLibkiwixDate checkCurrentLinuxLibkiwixDate these methods.

Could we use regex/glob with * to be independent of the date ?

[mgautierfr]

@MohitMaliFtechiz There is something I don't understand in gradle (and it is me not knowing gradle. I probably need a pointer or a explanation from you about that):

How gradle defines the dependencies between targets. Except for build depending of buildLinuxSoFile, you are never explicit about the dependencies. It is something you have forget or does gradle detect dependencies itself (and how?) ? Why you need to be explicit about build and buildLinuxSoFile ?

[MohitMaliFtechiz]

@MohitMaliFtechiz There is something I don't understand in gradle (and it is me not knowing gradle. I probably need a pointer or a explanation from you about that):

How gradle defines the dependencies between targets. Except for build depending of buildLinuxSoFile, you are never explicit about the dependencies. It is something you have forget or does gradle detect dependencies itself (and how?) ? Why you need to be explicit about build and buildLinuxSoFile ?

@mgautierfr , in android library project, CmakeList are configured with build command for generating .aar file for all android architectures. if we want to build .so file for linux variant, then we needs to run cmake . and make command for CMakeList. For this we made buildLinuxSoFile task in this PR. build depends on buildLinuxSoFile to generate both (.aar for all android architectures and .so for linux variant for test cases) at a same time.

[MohitMaliFtechiz]

Could we use regex/glob with * to be independent of the date ?

I have try to use regex/glob with * to copy files, but in gradle it is not working with copy task.

[mgautierfr]

I have try to use regex/glob with * to copy files, but in gradle it is not working with copy task.

It seems from the doc that we can somehow use complex rules to know what to copy: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.Copy.html

[MohitMaliFtechiz]

I have try to use regex/glob with * to copy files, but in gradle it is not working with copy task.

It seems from the doc that we can somehow use complex rules to know what to copy: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.Copy.html

hi @mgautierfr , Thanks for the reference, i have try this way to copy the headers and .so files. But it includes the whole folder with date not only the files. There is some restriction in from method we can not use the * , But we can give the base path of the folder in from method and everything after * we can give in include method. But include method copies the full folder which we give in include method. Which occurs the problem while we are including in CMakeList. so for removing dependency from date i have use a method to replace date from folder name. Now it's working fine without depending on date.

[kelson42]

I’m sorry to bother you with details but I’m not really happy with a few target naming:

• generateHeaders shoukd better be buildHeaders
• buildLinuxSoFile is really wrong, better would be buildLibrary or buildBinding
• .aar package building should better be isolated in its own target, something like buildAar or buildPackage
• build target should probably only call a list of other targets in a row
• Regarding createCodeCoverageReport, we should not need this as the report is done in codecov.io. I believe here we run the tests only once on x86-64 architecture, therefore we just need to run this test with the codecoverage instrumentation and upload the resukt via codecov/codecov-action, see https://docs.codecov.com/docs/github-2-getting-a-codecov-account-and-uploading-coverage

@mgautierfr What do you think?

[mgautierfr]

buildLinuxSoFile is really wrong, better would be buildLibrary or buildBinding

But we are also building so file for other targeted platform than linux (android) buildLibrary is too generic here. But buildLinuxBinding is nice (and even better if we also have buildAndroid<Arch>Binding targets)

Regarding createCodeCoverageReport, we should not need this as the report is done in codecov.io. I believe here we run the tests only once on x86-64 architecture, therefore we just need to run this test with the codecoverage instrumentation and upload the resukt via codecov/codecov-action, see docs.codecov.com/docs/github-2-getting-a-codecov-account-and-uploading-coverage

I like to have the build script independent of the CI. User should be able to generate the codecoverage locally without using a external service. So even if we don't use createCodeCoverageReport in the CI, it is nice to have it in the build system.

Else I agree with all your other remarks.

[MohitMaliFtechiz]

@kelson42 ,

generateHeaders shoukd better be buildHeaders buildLinuxSoFile is really wrong, better would be buildLibrary or buildBinding

Done.

Regarding createCodeCoverageReport, we should not need this as the report is done in codecov.io. I believe here we run the tests only once on x86-64 architecture, therefore we just need to run this test with the codecoverage instrumentation and upload the resukt via codecov/codecov-action, see https://docs.codecov.com/docs/github-2-getting-a-codecov-account-and-uploading-coverage

Regrading this i am agree with @mgautierfr .

.aar package building should better be isolated in its own target, something like buildAar or buildPackage build target should probably only call a list of other targets in a row

build is the bydefault method for building the .aar file in android library project.

[kelson42]

Regrading this i am agree with @mgautierfr .

Then please update/remove from CI accordingly.

[MohitMaliFtechiz]

@kelson42 , I have did the changes but currently CI has some problem to run, so i have not tested yet. Once CI runs I'll test it.

[mgautierfr]

ubuntu-18.04 is deprecated. Please move to ubuntu-20.04. https://github.blog/changelog/2022-08-09-github-actions-the-ubuntu-18-04-actions-runner-image-is-being-deprecated-and-will-be-removed-by-12-1-22/

[MohitMaliFtechiz]

Then please update/remove from CI accordingly.

@kelson42 , I have changed in the CI. but runTest generates the jacoco.exec file which is not supported by the codecov https://docs.codecov.com/docs/supported-report-formats. You can check here https://app.codecov.io/github/kiwix/java-libkiwix/commit/6eede1e78950c7f2b755fe2ab52eb495c132ffe4 it uploads but the report is not extracted from it.

[MohitMaliFtechiz]

I don't know why the CI is not running but here my review anyway.

I'm not sure it will work as it seems you have forget to commit few things.

I hadn't pay attention since now but please rewrite (almost) all your commit messages. A good commit message is :

A title on one line

A full description (if needed), separated with a empty line
from the title line.

The description can be as long as needed, but take care that
the lines themeselves (both in description and title) are not too
long (keep them under 80 chars)

Almost all your commit is only one long line and it breaks the assertion about commit message format (and the way it is display in github and all other git tools)

this changes has being done.

[kelson42]

@mgautierfr any update?

[mgautierfr]

This PR is now kind of obsolete. The changes in this PR is mostly included in #30 and (for the few not included) in #33.