Hi,
On Thu, 5 Nov 2026 at 22:29, Andres Freund <[email protected]> wrote:
>
> > > @@ -154,7 +148,8 @@ jobs:
> > > warn-if-not-opted-in:
> > > name: Report if not opted into CI
> > > if: ${{vars.PG_CI_ENABLED != '1'}}
> > > - runs-on: ubuntu-slim
> > > + # See the setup task for an explanation
> > > + runs-on: ${{ case(vars.PG_CI_RUNS_ON_SETUP != '',
> > > vars.PG_CI_RUNS_ON_SETUP, 'ubuntu-slim') }}
> > > steps:
> > > - name: Warn
> > > env:
> > > ...
> > > + runs_on_linux: ${{ case(vars.PG_CI_RUNS_ON_LINUX != '',
> > > vars.PG_CI_RUNS_ON_LINUX, 'ubuntu-24.04') }}
> >
> > I'd love if there were a neater way to do this, but I don't know of any :(.
> >
> > I'll play around with it to see if I can come up with something neater,
> > particularly the repetition of the variable name is annoying.
>
> It turns out one can just do ${{ vars.PG_CI_RUNS_ON_LINUX || 'ubuntu-24.04'
> }}.
> Not super well documented, but looks to be fairly widely used. Think that's a
> good bit neater.
>
> Also worked on making the image: simpler. There's now one format() template
> (still a long line), but the individual tasks now are
> - image: ${{ needs.setup.outputs.container_linux_ci_base
> }}/linux-autoconf:${{ needs.setup.outputs.container_tag }}
> + image: ${{ format(needs.setup.outputs.container_image,
> 'linux-autoconf') }}
>
> which seems a lot easier to read to me.
I agree, that is definitely better.
> Here's a mildly polished version:
>
> - Adoped the simpler runs-on: and image: formulations
>
> - An AI review raised the following, which I addressed:
>
> - location: ${{github.workspace}} is suboptimal, because it means that the
> directory will be searched for logs in case of a failure. Testing shows
> that setup-msys2 will pick a sane directory, so I think just not
> specifying a directory makes sense
I ran another AI review and it found similar problem for the
'PG_REGRESS_SOCK_DIR'. It said 'RUNNER_TEMP' could be too long and
cause a problem. From AI review:
```
PostgreSQL’s Windows sockaddr_un.sun_path is 108 bytes. The server
rejects socket paths longer than 107 bytes, including the appended
/.s.PGSQL.<port>. With a five-digit port, this leaves 85 bytes for
RUNNER_TEMP.
```
Currently it is set to `RUNNER_TEMP=D:\a\_temp` [1] so I don't expect
it to be a problem but just wanted to mention it.
> - CHERE_INVOKING is now obsolete
>
> - The socket dir could end up with a too long path. I don't entirely
> remember
> how I ended up with the previous formulation, I think it had to do with
> issues around the paths in the container being different than outside.
> The new formulation of just creating it in the container seemed better
>
> - added the discussion links, dropped references, as mentioned in my earlier
> email
>
>
> What do you think?
Rest LGTM.
[1]
https://docs.github.com/en/actions/reference/workflows-and-actions/variables#default-environment-variables
--
Regards,
Nazir Bilal Yavuz
Microsoft
