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

Migrate buildSrc to build-logic, etc #1173

Merged
merged 9 commits into from
Jan 16, 2024

Conversation

WhiredPlanck
Copy link
Collaborator

@WhiredPlanck WhiredPlanck commented Jan 12, 2024

Pull request

Issue tracker

Fixes will automatically close the related issues

Fixes # N/A

Feature

Describe features of pull request

This PR is still to improve build script quality.

  1. Migrate buildSrc to build-logic(/convention), since buildSrc has some limits (such as it may slow down the build speed). There is a official demo for this to reference
  2. Create conventions with custom Gradle plugins, make the build logic more clear.
  3. Adjust some Gradle properties and update deprecated API usages.
  4. implement custom DocumentProvider to prepare to utilizing app internal storage to work.
  5. Add ability that just build in curtain ABI(s) by specifying a environment variable.

Code of conduct

Style lint

  • make sytle-lint

Build pass

  • make debug

Manually test

  • Done

Code Review

  1. No wildcards import
  2. Manual build and test pass
  3. GitHub action ci pass
  4. At least one contributor reviews and votes
  5. Can be merged clean without conflicts
  6. PR will be merged by rebase upstream base

Daily build

Login and download artifact at https://github.com/osfans/trime/actions

Additional Info

- Remove deprecated `buildconfig` property
- Disable Jetifier
- Remove unnecessary jvmargs
prepare to use internal storage to comply with Google Play's policy
Migrate buildSrc to build-logic/convention
- Allow to use environment variables / Gradle properties to determine CMake and NDK version and build ABI
- Use legacy packing for JNI libs
@WhiredPlanck WhiredPlanck changed the title Migrate buildSrc to build-locgic, etc Migrate buildSrc to build-logic, etc Jan 12, 2024
@Bambooin
Copy link
Collaborator

I don't like spliting 3 build to 12 build with Action matrix personally.

We can build one abi(arm64-v8a) in local, but the CI should build all abi in one job.

@WhiredPlanck
Copy link
Collaborator Author

I don't like spliting 3 build to 12 build with Action matrix personally.

We can build one abi(arm64-v8a) in local, but the CI should build all abi in one job.

Make sense. I will make a way to do that.

@WhiredPlanck WhiredPlanck force-pushed the convention branch 2 times, most recently from e826d2c to 8892b4d Compare January 14, 2024 03:15
@WhiredPlanck
Copy link
Collaborator Author

@Bambooin Done.

@Bambooin
Copy link
Collaborator

Local make release will build all abi and package to universial apk.

7zz l trime-3.2.17-release.apk | grep lib
1981-01-01 01:01:02 .....      5093192      1695587  lib/arm64-v8a/librime_jni.so
1981-01-01 01:01:02 .....      3602280      1469936  lib/armeabi-v7a/librime_jni.so
1981-01-01 01:01:02 .....      5124812      1827727  lib/x86/librime_jni.so
1981-01-01 01:01:02 .....      5289240      1810192  lib/x86_64/librime_jni.so

And this patch will break cache-hash.sh logic which is very important for native cache.

@WhiredPlanck
Copy link
Collaborator Author

WhiredPlanck commented Jan 14, 2024

Local make release will build all abi and package to universial apk.

I didn't test the release build since it really takes time. You're right. It still makes sense to keep release packages splitted by API.

And this patch will break cache-hash.sh logic which is very important for native cache.

This is easy to fix, just change/add the file to grep. But I often thought it is too hacky to work. It seems that a better idea is to implement Reproducible Build:

It is generally desirable that building the same source code with the same set of tools is reproducible, i.e. the output is always exactly the same. This makes it possible to verify that the build infrastructure for a binary distribution or embedded system has not been subverted. This can also make it easier to verify that a source or tool change does not make any difference to the resulting binaries.

And I have attempted to do that in #1174.

@Bambooin
Copy link
Collaborator

Found one serious issue: the latest make release apk just shows up with black view after installation, nothing can be done.

@WhiredPlanck
Copy link
Collaborator Author

Found one serious issue: the latest make release apk just shows up with black view after installation, nothing can be done.

Wierd. I will take time to investigate.

@WhiredPlanck
Copy link
Collaborator Author

@Bambooin Fixed. It caused by that I exclude an "unwanted" library which is actually depended by the app.

@WhiredPlanck

This comment was marked as outdated.

@WhiredPlanck
Copy link
Collaborator Author

And this patch will break cache-hash.sh logic which is very important for native cache.

@Bambooin I improve this by the commit 5110c10, the mechanism of calculating hash may be more robust.

@Bambooin
Copy link
Collaborator

Fix blocking issue was fixed in the latest commit.

You can append another commit with removing cache-hash.sh, since we don't need this.

Then we can verify the gradle cache function is fine.

@WhiredPlanck
Copy link
Collaborator Author

@Bambooin Everything looks like OK.

@Bambooin
Copy link
Collaborator

Yeah, I checked the cache in ci too.

You can rebase your branch and merge to develop, then push directly to develop.

@WhiredPlanck WhiredPlanck merged commit 8490e0c into osfans:develop Jan 16, 2024
3 checks passed
@WhiredPlanck
Copy link
Collaborator Author

Yeah, I checked the cache in ci too.

You can rebase your branch and merge to develop, then push directly to develop.

@Bambooin Done. Thanks for review ~.

@WhiredPlanck WhiredPlanck deleted the convention branch January 16, 2024 13:09
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.

None yet

2 participants