-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
New build System #22
Conversation
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`
There was a problem hiding this 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.
.github/workflows/pull_request.yml
Outdated
java-version: 11 | ||
|
||
- name: Build Project | ||
run: bash contrib/build.sh |
There was a problem hiding this comment.
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
.github/workflows/pull_request.yml
Outdated
java-version: 11 | ||
|
||
- name: Build Project | ||
run: bash contrib/build.sh |
There was a problem hiding this comment.
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: | |
There was a problem hiding this comment.
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: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
.github/workflows/release.yml
Outdated
java-version: 11 | ||
|
||
- name: Building Project | ||
run: bash contrib/build.sh |
There was a problem hiding this comment.
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: | |
There was a problem hiding this comment.
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: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
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. |
- Build everything in buildDir instead of copy/generate things in src.
3a2fa2d
to
c4b582c
Compare
I've indeed forget to adapt the github workflow to the new file structure. Thanks. It should be ok now.
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 |
Yes. Here what we should have (big picture):
With the chain of dependencies. We would only have to launch |
I propose to merge this PR as it is and fix testing and target dependencies later. |
Yes its fine for now. |
This branch is mainly a cleanup/rebase of #20
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.