Feedback-ID: id4a34324:Fastmail
Date: Mon, 21 Nov 2022 14:45:42 -0800
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers@lists.postgresql.org, Noah Misch <noah@leadboat.com>,
	Michael Paquier <michael@paquier.xyz>,
	Anastasia Lubennikova <lubennikovaav@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	samay sharma <smilingsamay@gmail.com>
Subject: Re: CI and test improvements
Message-ID: <20221121224542.p2zapvyvb7objluw@alap3.anarazel.de>
20221113235303.GA26337@telsasoft.comFrom: Andres Freund <andres@anarazel.de>
References: <20220623193125.GB22452@telsasoft.com>
 <20220528153741.GK19626@telsasoft.com>
 <20220828144447.GA21897@telsasoft.com>
 <20220828160752.l5l66k3eptokzhzj@awork3.anarazel.de>
 <20220828171029.GO2342@telsasoft.com>
 <20220828212802.r6eymfffrgr3lxxt@awork3.anarazel.de>
 <20220910200542.GX31833@telsasoft.com>
 <20221104235412.GE16921@telsasoft.com>
 <20221105015946.yrxijqb7h4rqhp6d@awork3.anarazel.de>
 <20221113235303.GA26337@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221113235303.GA26337@telsasoft.com>
From: Andres Freund <andres@anarazel.de>
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/20221121224542.p2zapvyvb7objluw%40alap3.anarazel.de>
Precedence: bulk

Hi,

On 2022-11-13 17:53:04 -0600, Justin Pryzby wrote:
> On Fri, Nov 04, 2022 at 06:59:46PM -0700, Andres Freund wrote:
> > > diff --git a/.cirrus.yml b/.cirrus.yml
> > > index 9f2282471a9..99ac09dc679 100644
> > > --- a/.cirrus.yml
> > > +++ b/.cirrus.yml
> > > @@ -451,12 +451,20 @@ task:
> > >
> > >    build_script: |
> > >      vcvarsall x64
> > > -    ninja -C build
> > > +    ninja -C build |tee build/meson-logs/build.txt
> > > +    REM Since pipes lose exit status of the preceding command, rerun compilation,
> > > +    REM without the pipe exiting now if it fails, rather than trying to run checks
> > > +    ninja -C build > nul
> > 
> > This seems mighty grotty :(. but I guess it's quick enough not worry about,
> > and I can't come up with a better plan.
> > 
> > It doesn't seem quite right to redirect into meson-logs/ to me, my
> > interpretation is that that's "meson's namespace".  Why not just store it in
> > build/?
> 
> I put it there so it'd be included with the build artifacts.

Wouldn't just naming it build-warnings.log suffice? I don't think we
want to actually upload build.txt - it already is captured.


> > > From e85fe83fd8a4b4c79a96d2bf66cd6a5e1bdfcd1e Mon Sep 17 00:00:00 2001
> > > From: Justin Pryzby <pryzbyj@telsasoft.com>
> > > Date: Sat, 26 Feb 2022 19:34:35 -0600
> > > Subject: [PATCH 5/8] cirrus: build docs as a separate task..
> 
> > > +  # Exercise HTML and other docs:
> > > +  ninja_docs_build_script: |
> > > +    mkdir build.ninja
> > > +    cd build.ninja
> > 
> > Perhaps build-ninja instead? build.ninja is the filename for ninja's build
> > instructions, so it seems a bit confusing.
> 
> Sure.
> 
> Do you think building docs with both autoconf and meson is what's
> desirable here ?

Not sure.


> I'm not sure if this ought to be combined with/before/after your "move
> compilerwarnings task to meson" patch?  (Regarding that patch: I
> mentioned that it shouldn't use ccache -C, and it should use
> meson_log_artifacts.)

TBH, I'm not quite sure a separate docs task does really still make
sense after the SanityCheck task. It's worth building the docs even if
some flappy test fails, but I don't think we should try to build the
docs if the code doesn't even compile, in all likelihood a lot more is
wrong in that case.


> > > From: Justin Pryzby <pryzbyj@telsasoft.com>
> > > Date: Tue, 26 Jul 2022 20:30:02 -0500
> > > Subject: [PATCH 6/8] cirrus/ccache: add explicit cache keys..
> > >
> > > Since otherwise, building with ci-os-only will probably fail to use the
> > > normal cache, since the cache key is computed using both the task name
> > > and its *index* in the list of caches (internal/executor/cache.go:184).
> > 
> > Seems like this would potentially better addressed by reporting a bug to the
> > cirrus folks?
> 
> You said that before, but I don't think so - since they wrote code to do
> that, it's odd to file a bug that says that the behavior is wrong.  I am
> curious why, but it seems delibrate.
> 
> https://www.postgresql.org/message-id/20220828171029.GO2342%40telsasoft.com

I suspect this is just about dealing with unnamed tasks and could be
handled by just mixing in CI_NODE_INDEX if the task name isn't set.


I pushed a version of 0007-cirrus-clean-up-windows-task.patch. I didn't
rename the task as I would like to add a msbuild version of the task at
some point (it's pretty easy to break msbuild but not ninja
unfortunately). In additional I also removed NO_TEMP_INSTALL.

Greetings,

Andres Freund