Reorganise the gradle build script by MohitMaliFtechiz · Pull Request #32 · kiwix/java-libkiwix

MohitMaliFtechiz · 2023-03-24T13:25:58Z

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 openzim/libzim#772 , Once this issue is fixed we will remove this added url .

codecov-commenter · 2023-03-27T11:39:07Z

Codecov Report

❗ 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

@@           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. Have a feature suggestion? Share it here.

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

mgautierfr

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.

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.

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.

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)

I'm not sure to understand build the dependency system in gradle.
You never tell gradle that build or generateHeader depends of other tasks, but they are run anyway. Why do you need to tell that build depends of buildLinuxSoFile ?

mgautierfr · 2023-03-27T15:06:24Z

The diff of this file is really difficult to read.
I don't know if you have change something (and what) or if you have simply moved code in the file.
It would have been better to have a commit moving code (without changing it) and another commit changing the code (if needed). Of course with nice commit message.

mgautierfr · 2023-03-27T15:11:10Z

+    commandLine 'bash', '-c', "cmake . -B ${buildDir} && make -C ${buildDir}"
+}
+
+build.dependsOn buildLinuxSoFile


What build target does more than buildLinuxSoFile ?

build target for generating the .aar file for android.

MohitMaliFtechiz · 2023-03-28T10:54:44Z

@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 · 2023-03-28T13:09:57Z

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 · 2023-03-28T13:19:36Z

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 · 2023-03-29T12:33:07Z

@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 · 2023-03-29T13:05:48Z

@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 · 2023-03-29T13:09:10Z

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 · 2023-03-29T13:11:06Z

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 · 2023-03-31T10:59:50Z

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 · 2023-04-02T11:04:42Z

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 · 2023-04-03T15:52:39Z

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 · 2023-04-04T07:42:07Z

@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 · 2023-04-04T16:09:23Z

Regrading this i am agree with @mgautierfr .

Then please update/remove from CI accordingly.

MohitMaliFtechiz · 2023-04-05T08:04:35Z

@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

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)

mgautierfr · 2023-04-05T09:47:26Z

-                .filter(f -> f.name.startsWith(matchesString))
-                .map(s -> s.name.replace(matchesString, ""))
-                .collect(Collectors.toList())[0]
+static void renameFolders(String path, String startWith) {


The method doesn't simply rename folders.
It is removing date information for the folder. It should be named accordingly.

mgautierfr · 2023-04-05T09:50:30Z

+    dependsOn testSourceJar
+    source = fileTree('src/test/')
+    destinationDirectory = file('src/test/')
+    classpath = files("src/test/junit-4.13.jar" , "src/test/hamcrest-core-1.4.jar", "build/libs/test-sources.jar")


Do we need to package the sources as a jar to compile it ?
Could be simply build from the sources directly ?

Yes, we need to package the sources as a jar to compile the test cases. I have tried to build from the sources directly, But it was not including the sources with classpath in test cases.

mgautierfr · 2023-04-05T09:54:30Z

 task generateHeader(type: Exec) {
    workingDir "${projectDir}/src/main/java/org/kiwix/"
-    commandLine 'bash', '-c', "javac -h ${buildDir}/include/javah_generated/ -d ${projectDir}/src/test/ ${getLibzimFiles()} ${getLibkiwixFiles()}"
+    commandLine 'bash', '-c', "javac -h ${buildDir}/include/javah_generated/ -d ${projectDir}/src/test/java/ ${getLibzimFiles()} ${getLibkiwixFiles()}"


We don't have java subdirectory in test.
Have you forget something in your commit ? Or I missing something ?

@mgautierfr , we are generating java subdirectory in test via buildHeaders, Because we are generating the test source jar from .class files, which we are using in our test cases.

Ok.
It would be better to not generate things in source directory. Everything should be generated in the build directory.

Since we are compile test.java file we required java sources in the same directory , I tried from build but its importing.

When you are building the kiwix-android application, you don't need the java sources of the wrapper.
Why do you need the source of the wrapper when you build the test application ?

mgautierfr · 2023-04-05T12:29:31Z

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 · 2023-04-05T14:05:50Z

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.

Currently linux libzim tar file containing 0 byte .so file, so for now we are changing the libzim linux tar file url. Once this issue is fixed we will remove this temporary url.

This method is for building headers files, so for the better naming of this method we have renamed this as buildHeaders.

To avoid recompiling the source in test cases. Now we are generating .so file while building the .aar file. For this we have created buildLinuxBinding task which dependsOn build target. As now test/CMakeList is unused so we have removed it.

Earlier in gradle, There are so many tasks for copying headers and .so files of libzim, libkiwix. Now, we are copying everything in copyLibzimHeaderAndSoFiles, copyLibkiwixHeaderAndSoFiles tasks.

We are compiling and running test cases via gradle task and removing compile_and_run_test.sh as it is only for running test cases.

Earlier we are depending on the date variable to copy headers and .so files from folder. Now we introduce removeDateFromFolderName method for removing the date from folders name, so it would be more easy to getting files from folder.

Earlier we are using the java commands to compile and run test cases, Now we are using gradle for building and running test cases.

…porary url

MohitMaliFtechiz · 2023-04-06T13:37:37Z

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 · 2023-04-11T16:20:43Z

@mgautierfr any update?

mgautierfr · 2023-06-14T08:17:42Z

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.

MohitMaliFtechiz marked this pull request as draft March 24, 2023 13:26

MohitMaliFtechiz marked this pull request as ready for review March 27, 2023 14:01

MohitMaliFtechiz requested a review from mgautierfr March 27, 2023 14:01

mgautierfr suggested changes Mar 27, 2023

View reviewed changes

MohitMaliFtechiz marked this pull request as draft March 28, 2023 13:10

MohitMaliFtechiz force-pushed the Issue#31 branch 2 times, most recently from 7c1dfed to 92a22a6 Compare March 29, 2023 12:21

mgautierfr suggested changes Mar 29, 2023

View reviewed changes

Comment thread lib/build.gradle Outdated

Comment thread lib/build.gradle Outdated

MohitMaliFtechiz marked this pull request as ready for review March 31, 2023 11:01

MohitMaliFtechiz requested a review from mgautierfr March 31, 2023 11:01

MohitMaliFtechiz force-pushed the Issue#31 branch from c39149c to 9488ddd Compare April 4, 2023 07:35

mgautierfr suggested changes Apr 5, 2023

View reviewed changes

MohitMaliFtechiz marked this pull request as draft April 5, 2023 14:06

Changed libzim linux url

a2c3153

Currently linux libzim tar file containing 0 byte .so file, so for now we are changing the libzim linux tar file url. Once this issue is fixed we will remove this temporary url.

MohitMaliFtechiz added 10 commits April 6, 2023 14:23

Renamed generateHeaderFilesFromJavaWrapper method

2bda547

This method is for building headers files, so for the better naming of this method we have renamed this as buildHeaders.

Generating .aar and .so file via CMakeList

392f544

To avoid recompiling the source in test cases. Now we are generating .so file while building the .aar file. For this we have created buildLinuxBinding task which dependsOn build target. As now test/CMakeList is unused so we have removed it.

Removed different copying tasks

14109ec

Earlier in gradle, There are so many tasks for copying headers and .so files of libzim, libkiwix. Now, we are copying everything in copyLibzimHeaderAndSoFiles, copyLibkiwixHeaderAndSoFiles tasks.

Run test cases via gradle task

fadf201

We are compiling and running test cases via gradle task and removing compile_and_run_test.sh as it is only for running test cases.

Removed dependency from date variable

b3f7c98

Earlier we are depending on the date variable to copy headers and .so files from folder. Now we introduce removeDateFromFolderName method for removing the date from folders name, so it would be more easy to getting files from folder.

Removed javac bash commands to compile test cases

3500f20

Earlier we are using the java commands to compile and run test cases, Now we are using gradle for building and running test cases.

Fixed source file path in compileTestFile task

d1c2194

Generating test binding and jacoco reports in build directory

c694f6f

Now we are using ubuntu-20.04 since ubuntu-18.04 is deprecated

1a1bd73

Since linux libzim tar file issue is fixed so we have removed the tem…

fea35c2

…porary url

MohitMaliFtechiz force-pushed the Issue#31 branch from 6eede1e to fea35c2 Compare April 6, 2023 13:34

MohitMaliFtechiz requested a review from mgautierfr April 6, 2023 13:35

MohitMaliFtechiz marked this pull request as ready for review April 6, 2023 13:35

mgautierfr closed this Jun 14, 2023

kelson42 deleted the Issue#31 branch October 28, 2023 12:20

Uh oh!

Conversation

MohitMaliFtechiz commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mgautierfr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MohitMaliFtechiz commented Mar 28, 2023

Uh oh!

MohitMaliFtechiz commented Mar 28, 2023

Uh oh!

mgautierfr commented Mar 28, 2023

Uh oh!

Uh oh!

Uh oh!

mgautierfr commented Mar 29, 2023

Uh oh!

MohitMaliFtechiz commented Mar 29, 2023

Uh oh!

MohitMaliFtechiz commented Mar 29, 2023

Uh oh!

mgautierfr commented Mar 29, 2023

Uh oh!

MohitMaliFtechiz commented Mar 31, 2023

Uh oh!

kelson42 commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgautierfr commented Apr 3, 2023

Uh oh!

MohitMaliFtechiz commented Apr 4, 2023

Uh oh!

kelson42 commented Apr 4, 2023

Uh oh!

MohitMaliFtechiz commented Apr 5, 2023

Uh oh!

mgautierfr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgautierfr commented Apr 5, 2023

Uh oh!

MohitMaliFtechiz commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MohitMaliFtechiz commented Apr 6, 2023

Uh oh!

kelson42 commented Apr 11, 2023

MohitMaliFtechiz commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 27, 2023 •

edited

Loading

kelson42 commented Apr 2, 2023 •

edited

Loading

MohitMaliFtechiz commented Apr 5, 2023 •

edited

Loading