Hi, On 2026-06-08 13:30:03 +0300, Nazir Bilal Yavuz wrote: > I think it looks okay, no need to use composite actions for this.
Cool. > I tested the patch and I confirm that it works as mentioned. Here is my > review: > > All the points you explained in the commit message are nice improvements! > > Typo in commit message: > > + In my testing this utilizes the available cache space (10GB for personal > + accounts) much more effictively than before. > > Typo at 'effictively' in the commit message. Oops. > diff --git a/.github/workflows/pg-ci.yml b/.github/workflows/pg-ci.yml > index 8560e9389f6..86dc47de8db 100644 > --- a/.github/workflows/pg-ci.yml > +++ b/.github/workflows/pg-ci.yml > > + - &ccache_decide_save_step > + name: "ccache: Decide if cache should be uploaded" > + id: ccache-pre-save > + # [Decide to] store the cache whenever the cache was set up, so that > + # incrementally addressing compiler errors/warnings doesn't have to > + # start from scratch. > + if: | > + always() && > + steps.ccache-restore-branch.conclusion == 'success' > + run: python3 src/tools/ci/gha_ccache_decide.py > > Isn't the conclusion always true unless GitHub has some self errors? I mean, the cache restoration *could* fail? Or another earlier step could We don't want to upload a new cache entry if we never got to building... > Also, we are directly running this script with the 'python3' command > but it might not be available on the PATH. I had some problems with > this on BSD images when we were using Cirrus. I am not sure we would > have such problems with GitHub Actions but I just wanted to mention > it. I think we'll just have to address it if/when it becomes a problem. I don't really see the alternative... > diff --git a/src/tools/ci/gha_ccache_decide.py > b/src/tools/ci/gha_ccache_decide.py > new file mode 100644 > index 00000000000..920f7bf9685 > --- /dev/null > +++ b/src/tools/ci/gha_ccache_decide.py > > +def main(): > + on_default_branch = os.environ["ON_DEFAULT_BRANCH"] == "true" > + ccache_dir = os.environ["CCACHE_DIR"] > > ccache_dir isn't used. Ah, yea. It was earlier, but I removed that part (computed the cache size, when this was a shell script, by using du. But that seemed too awkward in python, so I removed it). > + # If there were either barely any misses, or the cache hit ratio was > high, > + # there no point in generating a new cache entry. We have limited cache > + # space. > + should_save = misses > 10 and hit_pct < target_rate > > We consider misses here but we don't mention it I was trying to mention it, via "If there were either barely any misses". > , we only mention hit rate and target rate. I think this is not very > important since we can't possibly have a case that misses < 10 and hit_pct < > target_rate. Why could we not have such a case? If we start building with some changes that trigger cache misses, but there's a compiler error a few seconds in, that seems like it'd precisely hit that case? Greetings, Andres Freund
