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

New build System #22

Merged
merged 11 commits into from
Jan 11, 2023
Merged

New build System #22

merged 11 commits into from
Jan 11, 2023

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Dec 20, 2022

This branch is mainly a cleanup/rebase of #20

  • With far less commits
  • Less unnecessary files added and removed
  • No adaption of wrapper itself (so it doesn't compile with current version)

While it "compile" (with the change in PR #23) it is NOT TESTED.
The testing part itself doesn't work (and I'm not sure how it should work, @MohitMaliFtechiz we have to discuss about this).

@MohitMaliFtechiz please review this PR and check that I haven't forget something compared to #20.
There is few questions to you in the commits' messages.

There is still few things I disagree with this PR but I suggest we merge it (if there is nothing obviously wrong) as it is and fix the issues later, one by one.

This mainly do:
- move `src/wrapper/java` into `lib/src` (and remove `src/wrapper`)
- create `lib/src/cpp` and move all `*.(cpp|h)` in it
- move all gradle stuff in `android-kiwix-lib-publisher` into `.`
- remove `android-kiwix-lib-publisher`
This is mainly a update of gradle itself and new build files.
Build files are not working as the current structure is not the same
as `Issue#19`.
zim files has been changed but I don't know why.
Change came from commits 3cd8ed9 and 8bff08c but it is not explain
what are the changes. @MohitMaliFtechiz ?
Let's be a bit more closer than `android-libkiwixbuild/app/src/main`
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.

Project structure looks clean some of the changes required are requesting.

java-version: 11

- name: Build Project
run: bash contrib/build.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be bash install_deps.sh

java-version: 11

- name: Build Project
run: bash contrib/build.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be bash install_deps.sh

run: bash contrib/build.sh

- name: Compile Project
run: |
Copy link
Collaborator

Choose a reason for hiding this comment

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

For new folder structure we don't need to go into android-libkiwixbuild we should remove this. now we can directly use ./gradlew androidSourcesJar build

./gradlew androidSourcesJar build

- name: create unit coverage
run: |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too.

java-version: 11

- name: Building Project
run: bash contrib/build.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we should use bash install_deps.sh

run: bash contrib/build.sh

- name: Release build
run: |
Copy link
Collaborator

Choose a reason for hiding this comment

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

For new folder structure we don't need to go into android-libkiwixbuild we should remove this. Now we can directly use ./gradlew androidSourcesJar build

./gradlew assemble

- name: Genrate Source jar
run: |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too.

@MohitMaliFtechiz
Copy link
Collaborator

hi @mgautierfr ,

Related to testing part , Test cases were written purely in java they are not android specific test cases , so to run test cases written on java required .so files which are compiled on Linux variant not android variant so that why we are compiling its separately.

@mgautierfr
Copy link
Member Author

I've indeed forget to adapt the github workflow to the new file structure. Thanks. It should be ok now.

Related to testing part , Test cases were written purely in java they are not android specific test cases , so to run test cases written on java required .so files which are compiled on Linux variant not android variant so that why we are compiling its separately.

I don't think that testing part should compile the library to test. We should have a target to build the library (and different targets for different archs) and having the test target depending of the target building the library for linux x86_64

@MohitMaliFtechiz
Copy link
Collaborator

I've indeed forget to adapt the github workflow to the new file structure. Thanks. It should be ok now.

Related to testing part , Test cases were written purely in java they are not android specific test cases , so to run test cases written on java required .so files which are compiled on Linux variant not android variant so that why we are compiling its separately.

I don't think that testing part should compile the library to test. We should have a target to build the library (and different targets for different archs) and having the test target depending of the target building the library for linux x86_64

We are only building wrapper files to generate .so file while for libzim and libkiwiz we are downloading linux variant .so file from download.kiwix.org

@mgautierfr
Copy link
Member Author

Yes. Here what we should have (big picture):

  • 5 "targets" downloading the libzim.so for the 5 archs (4 android and 1 linux native)
  • 5 "targets" downloading the libkiwix.so for the 5 archs (as libzim)
  • 1 "target" generating the c headers of the wrapper from java classes (maybe 2 targets if we handle wrappers around libzim and libkiwix separately)
  • 5 "targets" creating the wrappers .so for the 5 archs. (Maybe 10 targets if we handle wrappers separately). The targets should depends of the downloading target and header generation target.
  • 1 "target" to build the aar from all .so. This target depends of the 4/8 "creating wrapper" targets (android only)
  • 1 test target which depends of the linux native "creating wraper" and "just" use the .so to test everything is ok

With the chain of dependencies. We would only have to launch ./gradlew build or ./gradlew test to have everything downloaded and compiled.

@mgautierfr
Copy link
Member Author

I propose to merge this PR as it is and fix testing and target dependencies later.
@MohitMaliFtechiz, is it ok for you ?

@MohitMaliFtechiz
Copy link
Collaborator

I propose to merge this PR as it is and fix testing and target dependencies later.
@MohitMaliFtechiz, is it ok for you ?

Yes its fine for now.

@kelson42 kelson42 merged commit 98388aa into main Jan 11, 2023
@kelson42 kelson42 deleted the adapt_buildsystem branch January 11, 2023 16:35
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