Horton

Thread

Commits

  1. ci: Use newer LLVM version with gcc, to avoid compiler warnings

  2. ci: Upgrade to Debian Bookworm

  3. Apply COPT to CXXFLAGS as well

  1. Upgrade Debian CI images to Bookworm

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-05-13T10:57:08Z

    Hi,

Bookworm versions of the Debian CI images are available now [0]. The
patches to use these images are attached.

'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_16+.patch' patch can
be applied to both upstream and REL_16 and all of the tasks finish
successfully.

'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_15.patch' patch can
be applied to REL_15 but it gives a compiler warning. The fix for this
warning is proposed here [1]. After the fix is applied, all of the
tasks finish successfully.

Any kind of feedback would be appreciated.

[0] https://github.com/anarazel/pg-vm-images/pull/91

[1] postgr.es/m/CAN55FZ0o9wqVoMTh_gJCmj_%2B4XbX9VXzQF8OySPZ0R1saxV3bA%40mail.gmail.com

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  2. Re: Upgrade Debian CI images to Bookworm

    Peter Eisentraut <peter@eisentraut.org> — 2024-05-24T14:17:37Z

    On 13.05.24 12:57, Nazir Bilal Yavuz wrote:
> Bookworm versions of the Debian CI images are available now [0]. The
> patches to use these images are attached.
> 
> 'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_16+.patch' patch can
> be applied to both upstream and REL_16 and all of the tasks finish
> successfully.
> 
> 'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_15.patch' patch can
> be applied to REL_15 but it gives a compiler warning. The fix for this
> warning is proposed here [1]. After the fix is applied, all of the
> tasks finish successfully.
> 
> Any kind of feedback would be appreciated.

These updates are very welcome and look straightforward enough.

I'm not sure what the backpatching expectations of this kind of thing 
is.  The history of this CI setup is relatively short, so this hasn't 
been stressed too much.  I see that we once backpatched the macOS 
update, but that might have been all.

If we start backpatching this kind of thing, then this will grow as a 
job over time.  We'll have 5 or 6 branches to keep up to date, with 
several operating systems.  And once in a while we'll have to make 
additional changes like this warning fix you mention here.  I'm not sure 
how much we want to take this on.  Is there ongoing value in the CI 
setup in backbranches?

With these patches, we could do either of the following:

1) We update only master and only after it branches for PG18.  (The 
update is a "new feature".)

2) We update only master but do it now.  (This gives us the most amount 
of buffer time before the next release.)

3) We update master and PG16 now.  We ignore PG15.

4) We update master and PG16 now.  We update PG15 whenever that warning 
is fixed.

5) We update master, PG16, and PG15, but we hold all of them until the 
warning in PG15 is fixed.

6) We update all of them now and let the warning in PG15 be fixed 
independently.

  3. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-05-24T17:30:00Z

    Hi,

On 2024-05-24 16:17:37 +0200, Peter Eisentraut wrote:
> I'm not sure what the backpatching expectations of this kind of thing is.
> The history of this CI setup is relatively short, so this hasn't been
> stressed too much.  I see that we once backpatched the macOS update, but
> that might have been all.

I've backpatched a few other changes too.


> If we start backpatching this kind of thing, then this will grow as a job
> over time.  We'll have 5 or 6 branches to keep up to date, with several
> operating systems.  And once in a while we'll have to make additional
> changes like this warning fix you mention here.  I'm not sure how much we
> want to take this on.  Is there ongoing value in the CI setup in
> backbranches?

I find it extremely useful to run CI on backbranches before
batckpatching. Enough so that I've thought about proposing backpatching CI all
the way.

I don't think it's that much work to fix this kind of thing in the
backbranches. We don't need to backpatch new tasks or such. Just enough stuff
to keep e.g. the base image the same - otherwise we end up running CI on
unsupported distros, which doesn't help anybody.


> With these patches, we could do either of the following:
> 5) We update master, PG16, and PG15, but we hold all of them until the
> warning in PG15 is fixed.

I think we should apply the fix in <= 15 - IMO it's a correct compiler
warning, what we do right now is wrong.

Greetings,

Andres Freund

  4. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-07-15T16:43:26Z

    Hi,

On 2024-05-24 10:30:00 -0700, Andres Freund wrote:
> On 2024-05-24 16:17:37 +0200, Peter Eisentraut wrote:
> > I'm not sure what the backpatching expectations of this kind of thing is.
> > The history of this CI setup is relatively short, so this hasn't been
> > stressed too much.  I see that we once backpatched the macOS update, but
> > that might have been all.
> 
> I've backpatched a few other changes too.
> 
> 
> > If we start backpatching this kind of thing, then this will grow as a job
> > over time.  We'll have 5 or 6 branches to keep up to date, with several
> > operating systems.  And once in a while we'll have to make additional
> > changes like this warning fix you mention here.  I'm not sure how much we
> > want to take this on.  Is there ongoing value in the CI setup in
> > backbranches?
> 
> I find it extremely useful to run CI on backbranches before
> batckpatching. Enough so that I've thought about proposing backpatching CI all
> the way.
> 
> I don't think it's that much work to fix this kind of thing in the
> backbranches. We don't need to backpatch new tasks or such. Just enough stuff
> to keep e.g. the base image the same - otherwise we end up running CI on
> unsupported distros, which doesn't help anybody.
> 
> 
> > With these patches, we could do either of the following:
> > 5) We update master, PG16, and PG15, but we hold all of them until the
> > warning in PG15 is fixed.
> 
> I think we should apply the fix in <= 15 - IMO it's a correct compiler
> warning, what we do right now is wrong.

I've now applied the guc fix to all branches and the CI changes to 15+.

Thanks Bilal!

Greetings,

Andres

  5. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-07-15T18:30:59Z

    Hi,

On 2024-07-15 09:43:26 -0700, Andres Freund wrote:
> I've now applied the guc fix to all branches and the CI changes to 15+.

Ugh. I see that this fails on master, because of

commit 0c3930d0768
Author: Peter Eisentraut <peter@eisentraut.org>
Date:   2024-07-01 07:30:38 +0200
 
    Apply COPT to CXXFLAGS as well

I hadn't seen that because of an independent failure (the macos stuff, I'll
send an email about it in a bit).

Not sure what the best real fix here is, this is outside of our code. I'm
inclined to just disable llvm for the compiler warning task for now.

Greetings,

Andres Freund

  6. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-07-15T19:37:54Z

    Hi,

On 2024-07-15 11:30:59 -0700, Andres Freund wrote:
> On 2024-07-15 09:43:26 -0700, Andres Freund wrote:
> > I've now applied the guc fix to all branches and the CI changes to 15+.
> 
> Ugh. I see that this fails on master, because of
> 
> commit 0c3930d0768
> Author: Peter Eisentraut <peter@eisentraut.org>
> Date:   2024-07-01 07:30:38 +0200
>  
>     Apply COPT to CXXFLAGS as well
> 
> I hadn't seen that because of an independent failure (the macos stuff, I'll
> send an email about it in a bit).
> 
> Not sure what the best real fix here is, this is outside of our code. I'm
> inclined to just disable llvm for the compiler warning task for now.

Oh - there's a better fix: Turns out bookworm does have llvm 16, where the
warning has been fixed. Upgrading the CI image to install llvm 16 should fix
this.   Any arguments against that approach?

Greetings,

Andres

  7. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-07-15T21:35:14Z

    Hi,

On 2024-07-15 12:37:54 -0700, Andres Freund wrote:
> On 2024-07-15 11:30:59 -0700, Andres Freund wrote:
> > On 2024-07-15 09:43:26 -0700, Andres Freund wrote:
> > > I've now applied the guc fix to all branches and the CI changes to 15+.
> >
> > Ugh. I see that this fails on master, because of
> >
> > commit 0c3930d0768
> > Author: Peter Eisentraut <peter@eisentraut.org>
> > Date:   2024-07-01 07:30:38 +0200
> >
> >     Apply COPT to CXXFLAGS as well
> >
> > I hadn't seen that because of an independent failure (the macos stuff, I'll
> > send an email about it in a bit).
> >
> > Not sure what the best real fix here is, this is outside of our code. I'm
> > inclined to just disable llvm for the compiler warning task for now.
>
> Oh - there's a better fix: Turns out bookworm does have llvm 16, where the
> warning has been fixed. Upgrading the CI image to install llvm 16 should fix
> this.   Any arguments against that approach?

Specifically, something like the attached.

Due to the CI failure this is causing, I'm planning to apply this soon...

Arguably we could backpatch this, the warning are present on older branches
too. Except that they don't cause errors, as 0c3930d0768 is only on master.

Greetings,

Andres

  8. Re: Upgrade Debian CI images to Bookworm

    Andres Freund <andres@anarazel.de> — 2024-07-15T21:58:16Z

    Hi,

On 2024-07-15 14:35:14 -0700, Andres Freund wrote:
> Specifically, something like the attached.
>
> Due to the CI failure this is causing, I'm planning to apply this soon...
>
> Arguably we could backpatch this, the warning are present on older branches
> too. Except that they don't cause errors, as 0c3930d0768 is only on master.

Here's a v2, to address two things:
- there was an error in the docs build, because LLVM_CONFIG changed
- there were also deprecation warnings in headerscheck/cpluspluscheck

So I just made the change apply a bit more widely.

Greetings,

Andres Freund