Make WordPress Core

Opened 3 years ago

Closed 18 months ago

Last modified 18 months ago

#53841 closed enhancement (fixed)

CI: investigate switching to the Composer cache action

Reported by: jrf's profile jrf Owned by: desrosj's profile desrosj
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:


Follow up on/iteration inspired by #47381

There is a Composer caching action available via the GitHub marketplace.

This action is already in use in the code style and PHPCompatibility workflows, but not yet in the unittest and code coverage action workflows.

Switching over to that action, would allow for further simplification of the workflow scripts as we would be able to replace four steps in each workflow with one.

Whether or not we can switch over, depends on how fine-grained the cache key is which is created by the action.

An issue has been opened asking for more details about the information used in the cache key generated by the action and is currently waiting for an answer.

The various requirements we have for a cache key are discussed in more detail in the PR which addressed #47381 - GitHub:

Change History (16)

This ticket was mentioned in PR #3456 on WordPress/wordpress-develop by @desrosj.

20 months ago

  • Keywords has-patch added

This ticket was mentioned in PR #3460 on WordPress/wordpress-develop by @desrosj.

20 months ago

Trac ticket:

#3 @desrosj
20 months ago

  • Keywords has-patch removed
  • Milestone changed from Awaiting Review to 6.2

I started looking into this related to #56820.

Some findings and notes:

The action uses a CACHE_RESTORE_KEY environment variable. At first, I was concerned this would potentially affect other caching steps within the same workflow, but this variable is not used in actions/cache, only within ramsey/install-composer, so it's a non issue.

The cache key used by the action is comprised of the following:

  • Runner machine operating system
  • PHP version (down to the bug fix number, ie. 7.4.32)
  • The dependency-versions value (highest in our case since we want to emulate composer update)
  • Any additional composer-options supplied (empty in our case, except when using PHP 8.2)
  • The hash of any composer.json or composer.lock files found.
  • The working directory

OR, if a custom cache key is provided, then the one provided is used without any of the previous items.

With these findings, I think it's safe and preferred to use this action to simplify our workflows. The only caveat is that the same issue as discussed in this PR related to a cache potentially losing it's effectiveness over time exists.

I don't think that's a blocker as cache entries are likely evicted from time to time. But the methodology used under the hood in GHA is not clear. This is the only guidance specified:

A repository can have up to 10GB of caches. Once the 10GB limit is reached, older caches will be evicted based on when the cache was last accessed. Caches that are not accessed within the last week will also be evicted.

In my testing of my PR, I've confirmed that cache hits for the keys specified in the first attempt occur on successive workflow runs be rerunning completed workflows.

#4 @desrosj
20 months ago

  • Keywords has-patch added

I also opened an issue on the action's repository suggesting better cache busting when using composer update related commands.

#5 @jrf
20 months ago

I don't think that's a blocker as cache entries are likely evicted from time to time. But the methodology used under the hood in GHA is not clear. This is the only guidance specified:

The biggest problem we have with the caches not being updated is PHPUnit.
Everything else is basically set to a fixed version in the composer.json file and goes through managed updates (so will never be stale as the cache is discarded on an update of the composer.json file).

PHPUnit and its dependencies, however, regularly release new versions and for all PHP versions using the PHPUnit 8.x and 9.x range (PHP 7.2 - 8.2), this means the cache will quickly go stale if we don't use a date based refresh.

AFAIK, caches which aren't used are evicted after a while, but caches which are used are not and as WP is a very active codebase, the caches created for the Composer downloads are used dozens of times a day, so will never be regarded as unused, even though they may have gone stale (which is why we added the "one week" bit to the cache-key in a previous iteration to ensure the cache would get refreshed).

@desrosj commented on PR #3460:

20 months ago

I updated the debug data so it no longer is printed before _and_ after configuring PHP, Node.js, etc. We only care about logging the versions _after_ these steps. But now we're straying a bit from the original intent of the PR. I'll consider splitting out the debug information improvements into a separate commit once we're happy with everything here.

#7 @jrf
20 months ago

@desrosj FYI - while looking at the ramsey/composer-install action runner to add the feature requested in the issue you opened about the caching, I noticed that the action also uses the now deprecated set-output.

I'm working on a fix for it.

@desrosj commented on PR #3460:

20 months ago

@jrf any objection to merging this now with the intention of following up in 2 weeks to either update to a new version of install-composer fixing this issue? Or perhaps using a custom cache key here temporarily until the update becomes available?

@desrosj commented on PR #3460:

20 months ago

As a general question - may just be me -, but why does each workflow have a "Performs the following steps" docblock ? The steps all have names, and if needs be, could be accompanied by a comment _within_ the workflow (above the step name).

The step docs at the top feel (to me) like something which can very easily get out of sync with the reality, making it a maintenance burden we can do without.

I think that initially, there were some steps that required additional context that was a little long for a name field, so I included it in this docblock.

In most cases, I think the need for extra context has disappeared. There could be scenarios in the future where it may be valuable, but It is a bit redundant. I wonder if these docblocks make it easier to follow and understand what is happening for anyone newer to GitHub Actions. I don't feel strongly either way.

@desrosj commented on PR #3460:

19 months ago

Today I discovered that there are cache related endpoints in the REST API now for GitHub Actions.

Having a workflow that periodically deletes certain cache entries could be added to solve the problem with cache keys entirely. An upstream way to handle this would still be nice, but I'm thinking there may be other times we want to clear some caches from the repository to optimize our usage of caching with actions.

One that comes to mind that I noticed recently looking at the caches stored on wordpress-develop was tags. The hashes for older branches probably don't need to be stored past a day or two since they don't have a lot of activity. Clearing these out would allow other cache keys with a higher likelihood of being reused to persist longer. Currently, it seems that the oldest cache entry on wordpress-develop is 3 days.

Definitely something to explore in its own ticket and discussion, but wanted to mention that here.

@jrf commented on PR #3460:

19 months ago

Also just saw that this was just published: GH Blog: Manage caches in your Actions workflows from Web Interface ;-)

@desrosj commented on PR #3460:

19 months ago

I've gone and cleaned up the PR after committing most of the changes unrelated to the goal of this PR/Trac ticket separately in Trac-54851 and implemented the new custom-cache-suffix feature from upstream.

#13 @desrosj
18 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 54856:

Build/Test Tools: Improve how Composer dependencies are installed.

To improve how Composer dependencies are installed and managed within GitHub Actions, the ramsey/composer-install third-party action is now used consistently throughout all workflows.

Previously, some workflows manually ran composer commands while others already used ramsey/composer-install.

The ramsey/composer-install action manages caching dependencies across workflow runs internally, which is something that was manually handled before this change.

Props jrf, desrosj.
Fixes #53841.

@jrf commented on PR #3460:

18 months ago

Nice! Glad it's sorted now!

#16 @desrosj
18 months ago

In 54921:

Build/Test Tools: Improve caching for PHPCS.

This improves the speed of the PHPCS scans between workflow runs in GitHub Action by creating a scan cache file for each unique set of arguments passed to phpcs.

Props jrf.
Fixes #57148. See #53841.

Note: See TracTickets for help on using tickets.