Cache Box PHAR in reusable binary workflow

[rahimahisah17]

Description

Cache the Box PHAR used by the reusable binary build workflow.

Changes

• Resolve the current Box release version to create a version-specific cache key.
• Cache box.phar using actions/cache@v4.
• Download Box only when the cache does not contain the requested version.
• Set the PHAR permissions after either a cache hit or download.

Related issue

Closes #200

[jrfnl]

@rahimahisah17 Thank you for this PR and your willingness to contribute to PHP Parallel Lint!

I've reviewed the PR and have left some feedback in line.

Additionally:

1. I believe that this workflow will now need a use-cache "input" which defaults to true, but can be passed as false from calling workflows, and should be respected in the workflow steps.
   The reason for this, is that this job is also used in the release.yml workflow and I wouldn't want to run the risk of a potentially poisoned cache infecting our release, so, while the test workflow could use the cache, the release workflow should definitely not do so.
2. I believe that, to optimize the caching, it may be beneficial to not make the caching dependent on the rest of the workflow succeeding.
   See for more info: https://github.com/actions/cache/blob/main/save/README.md#always-save-cache
3. Lastly, I'd like to ask you whether you have tested the PR ? I mean, I can see the cache being created in the workflow run from this PR, but what I cannot see (or confirm via proof at this moment) is a cache having been created and then being re-used.

I'd also like to ask you why the commit was created by "DevUser" and not you ?

I know it's a lot of feedback. Please do not let this discourage you and feel free to ask questions if anything is unclear. I'd rather you ask me than ask an LLM (which I suspect you used ?) as they get too much wrong.

[jrfnl]

This comment should be moved down (or rather, the new code should be moved up) as it belongs with the "Install box"/"Validate configuration" steps.

The comment in its current place doesn't make sense anymore.

[rahimahisah17]

Thanks for taking the time to review this PR and for the detailed feedback.

I understand the points raised in the review. I’ll go through the inline comments and make the necessary adjustments to the workflow, particularly around the Box version detection, cache key handling, cache hit/miss behavior, and the download/permission steps. I’ll also make sure the comments accurately reflect the behavior of the workflow.

Regarding DevUser and dev@skill-sch.com: these values came directly from a Skill.sch Git lab that I was completing. The lab specifically instructed me to configure my global Git username as DevUser and email as dev@skill-sch.com. They were used for that lab exercise and were not intended to represent another contributor or user of this project.

I’ll address the feedback and push the updated changes so the checks can be run again.

Thanks again for the review.

[jrfnl]

I realize this mirrors the current version download of the latest release, but I wonder if we should start to take into account that the latest release may not always be the semantically latest release. This is typically something which would/could happen if a project releases for multiple majors.

Not sure whether that is applicable here at this time, but whether it is or isn't, isn't even that relevant as it could still happen in the future and if it does, we don't want to get bitten by it.

We may also want to consider limiting to the "latest of the current major", as if Box releases a new major, the configuration we use may need updates etc.

This action runner might be helpful to solve the above feedback:
https://github.com/oprypin/find-latest-tag

I've used it successfully in another repo.

Note: if the above suggestions get implemented, the "Install box" step will also need to be updated to download the PHAR based on the version number, rather than on latest.

[rahimahisah17]

Thanks again for the detailed review, and sorry for the long silence.

To answer your question about LLMs: yes, I used an AI assistant when writing the original PR, and I should have said so when you asked. I'm also using one for this follow-up: it helped me compare the branch with your review, find the cache evidence in the workflow logs, and draft this comment.

On testing the cache: the run for the latest commit shows it being created earlier and then reused. In the "Build binary / Build binary" job of https://github.com/php-parallel-lint/PHP-Parallel-Lint/actions/runs/33920260753 the "Cache Box PHAR" step logs "Cache hit for: box-phar-4.7.0" and "Cache restored from key: box-phar-4.7.0", the "Install Box" step is skipped, and the save step is skipped because there was nothing new to save. release.yml passes use-cache: false, so the release build never touches the cache.

Still open from your review, and what I plan to do (please tell me if you'd prefer something different):

1. Bump actions/cache/restore and actions/cache/save from v4 to v6, which also removes the Node 20 warning.
2. Download Box by the version resolved in the previous step, so the cache key and the downloaded file always match.
3. On latest not always being the newest release of the current major: Box is on 4.x now. Would you prefer limiting to the latest 4.x with oprypin/find-latest-tag, or with a small gh api call so no extra third-party action is needed?

[jrfnl]

Version is very much out of date. Please update.

Note: Updating the action runner should also get rid of the warnings about the use of Node 20...

[rahimahisah17]

Thanks for the detailed review. I’ve addressed the requested changes, including the use-cache input, disabling the cache for the release workflow, separating cache restore/save, and moving the Box compatibility comment. I also corrected the Git author information and verified the updated workflows pass the checks.

The changes are now pushed to the PR and ready for another review. Thanks again!

[rahimahisah17]

I've pushed the two changes I mentioned: actions/cache/restore and actions/cache/save are now at v6, and the "Install Box" step downloads the version resolved in the previous step instead of latest. All checks pass on the new commit. The restore step reported a cache miss on that run only because the cache from my earlier run had expired after 7 days without use. The save step created a fresh box-phar-4.7.0 entry, so the next run on this PR should show a hit. I'll leave the latest versus latest-4.x question until you've had a chance to weigh in.