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

Gradle dependency cache on CI #11621

Draft
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented May 30, 2024

Description

This PR enables Gradle dependency cache.

Related toolkit PR: Automattic/a8c-ci-toolkit-buildkite-plugin#98

Testing instructions

Not needed

Images/gif

I re-run the same, :WooCommerce:dependencies task and results are as following:

Without cache With cache
Screenshot 2024-05-30 at 11 41 50 image
image image
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@dangermattic
Copy link
Collaborator

1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commita02d970
Direct Downloadwoocommerce-prototype-build-pr11621-a02d970.apk

@wzieba wzieba added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label May 30, 2024
@wzieba wzieba changed the title [WIP] Gradle dependency cache on CI Gradle dependency cache on CI May 30, 2024
@wzieba wzieba requested a review from oguzkocer May 30, 2024 09:44
@oguzkocer
Copy link
Contributor

I re-run the same, :WooCommerce:dependencies task and results are as following:

May I ask where you ran this command? Did you ssh into the CI, if so, can you share details about the state of the CI at that time?


set -e

save_gradle_dependency_cache || true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to save the cache every run? There is some race condition related risks, but beyond that, (as far as I know) the Gradle cache is restored into a read-only cache. Whereas, the cache that's saved is from the regular dependencies folder that Gradle writes to. So, unless I am missing some weird interaction, the dependencies of the current task will always override the ones in remote. Since detekt and assemble require different sets of dependencies, that makes this an always partial cache, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! What do you think would be the best strategy? Should have a different cache key for each Buildkite job, or save dependency cache only for the job which requires most dependencies, in this case: probably "Instrumented tests"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to update the dependency cache as infrequently as we can, without over-complicating things. Not so much for addressing any issues - although I do think it'll help with race conditions - it's more so we can reason about how our dependency caching works. If it's updated everywhere, it'll be hard to track things down if anything goes wrong.

So, picking one task to save the dependencies makes sense to me. Instrumented tests also sound reasonable, but could you do a quick check using :dependencies task that this encapsulates everything we expect it to? I can't remember if implementation dependencies made available for this task or if it only downloads the androidImplementation ones.

An alternative approach might be to add a Gradle task to force download the dependencies. With that approach we'd be able to download the dependencies for all configurations. However, the last time I checked this was a bit tricky, so there is some initial development cost to it.

Besides that, I think we should only update it after every tag. That's one of the least frequent CI tasks and yet it's more than frequent enough to update the dependency cache. Any dependency added between two releases will need to be downloaded for every run, but that's not a problem at all in my opinion.

@wzieba
Copy link
Contributor Author

wzieba commented May 30, 2024

May I ask where you ran this command? Did you ssh into the CI, if so, can you share details about the state of the CI at that time?

I modified pipeline: 7fa712b, I didn't ssh into the CI

@oguzkocer
Copy link
Contributor

I re-run the same, :WooCommerce:dependencies task and results are as following:

As far as I know - and it's been a while since I looked into this - dependencies task will not download dependencies. It's also not a task that represents any of our use cases, except maybe dependency-tree-diff, but that's just a tool. May I suggest the following methodology to validate that this is all working properly:

  • Take a snapshot of build scans at any commit. You can use any of the PRs, a build from trunk or make a small change and trigger CI in a draft PR.
  • Merge these changes with that commit (for this reason, it might be cleanest to start with a draft PR as suggested above)
  • Re-trigger the CI by pushing the changes
  • Share the build scans for these in the PR description, or as a comment. (we don't need to compare every type of build, some of the important ones such as prototype build, instrumented tests etc should be enough) You can share screenshots to highlight some things, but the important thing is the build scans themselves.

I am suggesting this methodology because I believe it represents the differences the best. I also need this information to properly review this PR. However, if you'd like to use a different methodology, I don't want to impose. I can do this myself as part of the review. Hope that all makes sense and sound reasonable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants