#53841 closed enhancement (fixed)
CI: investigate switching to the Composer cache action
Reported by: | jrf | Owned by: | desrosj |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | Cc: |
Description
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: https://github.com/WordPress/wordpress-develop/pull/1511#discussion_r674130418
Change History (16)
This ticket was mentioned in PR #3456 on WordPress/wordpress-develop by @desrosj.
2 years ago
#1
- Keywords has-patch added
This ticket was mentioned in PR #3460 on WordPress/wordpress-develop by @desrosj.
2 years ago
#2
Trac ticket:
#3
@
2 years 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 emulatecomposer update
) - Any additional
composer-options
supplied (empty in our case, except when using PHP 8.2) - The hash of any
composer.json
orcomposer.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
@
2 years 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
@
2 years 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).
2 years ago
#6
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
@
2 years 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.
2 years ago
#8
@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?
2 years ago
#9
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.
2 years ago
#10
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.
2 years ago
#11
Also just saw that this was just published: GH Blog: Manage caches in your Actions workflows from Web Interface ;-)
22 months ago
#12
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
@
22 months ago
- Owner set to desrosj
- Resolution set to fixed
- Status changed from new to closed
In 54856:
Trac ticket: https://core.trac.wordpress.org/ticket/53841