🤔 MarcoFalke reviewed a pull request: "ci: Disable cache save for pull requests in GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28292#pullrequestreview-1583930912)
concept ACK

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298060542)
Maybe also disable it for branches other than `master`, to make sure that storage is constant, even if there was an intermittent push to a backport branch?

⚠️ mrnadyy opened an issue: " mrmohamednadi@gmail.com"
(https://github.com/bitcoin/bitcoin/issues/28293)

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298063957)
While we are interested in the `restore-keys` input only, the `key` input is [required](https://github.com/actions/cache/blob/f7ebb81a3f195b4fb88dab7c14e2f7aff52045aa/restore/action.yml), thus, it cannot be avoided.

💬 MarcoFalke commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1298064385)
```suggestion
base_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
```

The two should be the same, no? Seems better to just use one way to get one thing, instead of two.

✅ MarcoFalke closed an issue: " mrmohamednadi@gmail.com"
(https://github.com/bitcoin/bitcoin/issues/28293)

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298066081)
ok, maybe use the same value for both, or is there a reason to use different values?

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298067481)
GitHub Actions caches are immutable. Therefore, [this](https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache) pattern has been used for Ccache caches.

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298068770)
ugh, fair enough

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298069742)
ok, nvm. Looks like caches are immutable anyway, so this can't be achieved, I guess.

👍 MarcoFalke approved a pull request: "ci: Disable cache save for pull requests in GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28292#pullrequestreview-1583948672)
lgtm ACK

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298070445)
```suggestion
key: ${{ github.job }}-ccache-${{ github.run_id }} # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache
```

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298072161)
Maybe it is still better to only have caches for a single branch, as they can't be re-used for other branches anyway?

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298072950)
> ... is there a reason to use different values?

Yes, it is. `github.run_id` is unique for every run. That is why the `restore-keys` input contains prefixes that are expected to match. The latest matched cache will be restored.

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298075170)
In the main repository, caches for backport branches will be evicted after 7 days of non-using them.

However, the current logic seems useful for fork repos, no?

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298077810)
The comment has been added.

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298086606)
I was thinking that (assuming a cache size per push of `$n` GB, and a total limit of 10GB), 10/n pushes to a backport branch will evict the main cache. For example, for n=5, 2 pushes are enough.

Though, it is only a cache, so I guess it doesn't matter.

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298093118)
> However, the current logic seems useful for fork repos, no?

If you are worried about hard-coding `master`, I think, if possible, using the GitHub default branch name should achieve the same.

💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298095969)
> However, the current logic seems useful for fork repos, no?

I mean, topic branches in fork repos.

💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#issuecomment-1683465163)
I guess as an alternative, the ghcr can be used to store a ccache? This would require a bit more code on our side, and maybe a few more permissions to be enabled? Though, if the write-permissions are limited to the `master` branch, maybe that is fine?

💬 MarcoFalke commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#issuecomment-1683467914)
I guess neither solution is perfect, but longer term it would still be fun to explore if something can be achieved via ghcr (or some other registry), see https://github.com/bitcoin/bitcoin/pull/28265/files#r1295647557