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

[Dependency Cache] Enable Dependency Cache on CI #21584

Merged
merged 9 commits into from
Jan 17, 2025

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Jan 15, 2025

Project Thread: paaHJt-7CE-p2
Related: #21550 + #21540
Depends On: a8c-ci-toolkit#135 -> a8c-ci-toolkit#138
Required By: buildkite-ci#578


Description

This PR enables dependency cache on CI for this project. 💾

The dependency cache mechanism is split between:

  • A weekly scheduled save cache build (see commit and buildkite-ci#578 PR), which targets the below tasks only:
    • Mobile App (assembleJetpackJalapenoDebug)
    • Lint (lintJetpackJalapenoDebug)
    • Unit Tests (assembleJetpackJalapenoDebugUnitTest assembleDebugUnitTest testClasses)
    • Android tests (assembleJetpackJalapenoDebugAndroidTest)
  • A restore cache step that is run on targeted pipeline.yml related jobs, via their corresponding individual .sh scripts:
    • Jetpack App (prototype-build.sh)
    • WordPress App (prototype-build.sh)
    • Jetpack Lint (lint.sh)
    • WordPress Lint (lint.sh)
    • WordPress Unit Tests (run-unit-tests.sh)
    • Processors Unit Tests (run-unit-tests.sh)
    • Image Editor Unit Tests (run-unit-tests.sh)
    • FluxC Unit Tests (run-unit-tests.sh)
    • Login Unit Tests (run-unit-tests.sh)
    • [Disabled**] Jetpack Android Tests (run-instrumented-tests.sh)
    • [Disabled**] WordPress Android Tests (run-instrumented-tests.sh)

PS.1: The detekt and checkstyle job aren't targeted because it only takes about 30+ seconds of network request to download all the dependencies needed for those specific jobs (build + scan for detekt), which is about the same time it takes for the dependency cache to actually be restored, which is more or less about 20+ seconds (build). Thus, this optimization is not really worth it for these specific jobs. For all those other jobs targeted, it takes more than a minute of network requests to download all the dependencies needed (build + scan), and as such the diff is worth it.

PS.2: Although the Android Test are commented-out, I chose to include cache restoration into those as well, just in case they will be reactivated at some point in the future.

PS.3: Dependency cache isn't enabled on the release-builds.yml pipeline just yet, but just temporarily, and until we become comfortable with running this new process on the main pipeline.yml pipeline. Having said that, and due to the fact that the release-builds.yml pipeline is also utilizing the lint.sh for its Lint steps, due to the fact that the restore-cache.sh is within that lint.sh script, restoring cache will partially work on the release-builds.yml pipeline as well.


Future Improvements

Me and @wzieba has been discussing on DMs about some possible future improvement, which are including but are not limited to the below list:

  1. Only execute the assemble android test task, which depends on assemble task anyway, and would most likely resolve most of the dependencies, for all other tasks to benefit from. However, this needs further testing to make sure that running other tasks such us lint and assemble unit test are providing little to no help.
  2. Create a custom Gradle task and utilize Gradle's init scripts (init.d). Using the init script, go through all available modules for a project, and then, using that custom Gradle task, resolve all dependencies for all tasks that are available per module. We could take some inspiration from plugins such as this.
  3. Instead of scheduling the dependency cache build to run weekly (buildkite-ci#578), make it run more often, possibly scheduling it to even run daily. However, since dependencies are not being updated too often, and, even when they do on that specific week, it would only be for a few of them. This will indeed cause some extra network requests, but most of the dependencies cached would still benefit from the existing and weekly refreshed dependency cache.
  4. Enable dependency cache on the release-builds.yml pipeline.

To Test:

This dependency cache solution was thoroughly tested via this #21550 test-only PR, but to help you asserting that this change is indeed making an impact, plus not causing any trouble for the project, try and follow the below:

Job BT (Without) BT (With) BT (Diff*) NR (Without) NR (With) RC (Without) RC (With) Win
Jetpack App 6m 32s 4m 38s - 1m 54s 1m 33 0s 0s 20s 1m 13s
WordPress App 6m 36s 4m 21s - 2m 15s 1m 31s 0s 0s 18s 1m 13s
Jetpack Lint 6m 19s 4m 31s - 1m 48s 1m 5s 2s 0s 18s 47s
WordPress Lint 6m 35s 4m 51s - 1m 44s 1m 18s 2s 0s 19s 59s
WordPress Unit Tests 5m 52s 4m 18s - 1m 34s 1m 32s 4s 0s 18s 1m 14s
Processors Unit Tests 3m 43s 2m 6s - 1m 37s 1m 23s 2s 0s 19s 1m 4s
Image Editor Unit Tests 2m 53s 3m 11s + 18s 40s 1s 0s 34s (❗️) 6s
FluxC Unit Tests 3m 35s 2m 31s - 1m 4s 48s 1s 0s 19s 29s
Login Unit Tests 4m 12s 2m 44s - 1m 28s 1m 40s 1s 0s 22s 1m 18s

Note

  • Without: Without Dependency Cache
  • With: With Dependency Cache
  • BT: Build Time
  • NR: Network Requests
  • RC: Restore Cache
  • Win: NR (Without) - RC (With)

Warning

(*): The BT (Diff) presented above is relative, so don't read too much into it, it might not match with the WIN, which is a much more solid number. But still, it is nice to have and compare those two number together. To to be clear about that, although I tried to pick the most representative builds, without and with the dependency cache mechanism in place, there are other factors that can make any build faster, or slower for that matter (like build cache, firebase test lab, etc), so again, don't read too much into it.

(**): Both, the Jetpack and WordPress Android Tests are currently disabled and as such not included in the above Restore Cache related table.

Caution

(❗️) The dependency cache restoration for Image Editor Unit Tests job took almost twice as expected 34s. This is quite interesting and I am not sure how to explain it. For a cache of more-or-less 1 GB in size, and as per all the other jobs suggest, it takes about 20+ seconds to download the cache entry from s3.


Merge Instructions


Regression Notes

  1. Potential unintended areas of impact

    • CI builds failing or slower than expected.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • See To Test section above.
  3. What automated tests I added (or what prevented me from doing so)

    • N/A

PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones): N/A

Release Notes: https://github.com/Automattic/
a8c-ci-toolkit-buildkite-plugin/releases/tag/3.9.1

FYI: This change points to that version of the 'A8C CI Toolkit' where
both, the #135 and #138 PRs, are merged into 'trunk', and, the new
dependency cache mechanism, per project, without 'GRADLE_RO_DEP_CACHE',
is fully operational.

A8C CI Toolkit #135 PR: [Dependency Cache] Dependency Cache on CI per
Project [without GRADLE_RO_DEP_CACHE] #135
- Automattic/a8c-ci-toolkit-buildkite-plugin#135

A8C CI Toolkit #138 PR: [Dependency Cache] Fix Dependency Cache #138
- Automattic/a8c-ci-toolkit-buildkite-plugin#138
@ParaskP7 ParaskP7 added this to the Future milestone Jan 15, 2025
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 15, 2025

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21584-dfbef04
Commitdfbef04
Direct Downloadjetpack-prototype-build-pr21584-dfbef04.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 15, 2025

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21584-dfbef04
Commitdfbef04
Direct Downloadwordpress-prototype-build-pr21584-dfbef04.apk
Note: Google Login is not supported on these builds.

FYI: This job will be then used by 'buildkite-ci' and configured as a
'buildkite_pipeline_schedule' with a weekly frequency.

PS.1: The targeted 'pipeline' related jobs are:
- Mobile App
- Lint
- Unit Tests
- Android tests
PS.2: Since the 'WordPress' app is being build from the very same
codebase as the 'Jetpack' app, targeting only the 'Jetpack' related
'pipeline' jobs are enough to download and cache all the dependencies
required for both apps.
Otherwise, using 'multi-cmd' will produce 'no such file or directory'
type of errors.
FYI: The targeted 'pipeline' related jobs are:
- Mobile App
- Lint
- Unit Tests
- Android Tests (Commented Out)

PS.1: The 'detekt' and 'checkstyle' jobs aren't targeted because they
only take about 30+ seconds of network request to download all the
dependencies needed for those specific jobs, which is about the same
time it takes for the dependency cache to actually be restored (20+
seconds). Thus, this optimization is not really worth it for these
specific jobs. For all those other jobs targeted, it take more than a
minute of network requests to download all the dependencies needed, and
as such worth the diff is worth it.
PS.2: Although the 'Android Tests' are commented-out, I chose to include
cache restoration into those as well, just in case they will be
reactivated at some point in the future.
WCAndroid PR Comment: https://github.com/woocommerce/
woocommerce-android/pull/13303#discussion_r1916752265
When using 'single-cmd' and not using this double quotes structure it is
is producing 'did not find expected key' type of errors.
… into ci/enable-dependency-cache-on-ci-final
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.48%. Comparing base (21f3d59) to head (dfbef04).
Report is 9 commits behind head on trunk.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #21584   +/-   ##
=======================================
  Coverage   39.48%   39.48%           
=======================================
  Files        2117     2117           
  Lines       99464    99464           
  Branches    15296    15296           
=======================================
  Hits        39269    39269           
  Misses      56714    56714           
  Partials     3481     3481           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ParaskP7 ParaskP7 marked this pull request as ready for review January 16, 2025 12:48
@ParaskP7 ParaskP7 requested a review from wzieba January 16, 2025 12:49
Copy link
Contributor

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

LGTM!

… into ci/enable-dependency-cache-on-ci-final
@ParaskP7 ParaskP7 merged commit 968214f into trunk Jan 17, 2025
22 checks passed
@ParaskP7 ParaskP7 deleted the ci/enable-dependency-cache-on-ci-final branch January 17, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants