Date: Sat, 10 Sep 2022 15:05:42 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Andres Freund <andres@anarazel.de>
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>
Subject: Re: CI and test improvements
Message-ID: <20220910200542.GX31833@telsasoft.com>
References: <20220708002232.GF13040@telsasoft.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="tKkaNMvYmhQvRCRK"
Content-Disposition: inline
In-Reply-To: <20220828212802.r6eymfffrgr3lxxt@awork3.anarazel.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220910200542.GX31833%40telsasoft.com>
Precedence: bulk


--tKkaNMvYmhQvRCRK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Aug 28, 2022 at 02:28:02PM -0700, Andres Freund wrote:
> On 2022-08-28 12:10:29 -0500, Justin Pryzby wrote:
> > On Sun, Aug 28, 2022 at 09:07:52AM -0700, Andres Freund wrote:
> > > > --- /dev/null
> > > > +++ b/src/tools/ci/windows-compiler-warnings
> > >
> > > Wouldn't that be doable as something like
> > > sh -c 'if test -s file; then cat file;exit 1; fi"
> > > inside .cirrus.yml?
> >
> > I had written it inline in a couple ways, like
> > - sh -exc 'f=msbuild.warn.log; if [ -s "$f" ]; then cat "$f"; exit 1; else exit 0; fi'
> >
> > but then separated it out as you suggested in
> > 20220227010908.vz2a7dmfzgwg742w@alap3.anarazel.de
> >
> > after I complained about cmd.exe requiring escaping for && and ||
> > That makes writing any shell script a bit perilous and a separate script
> > seems better.
> 
> I remember that I suggested it - but note that the way I wrote above doesn't
> have anything needing escaping. 

It doesn't require it, but that still gives the impression that it's
normally possible to write one-liner shell scripts there, which is
misleading/wrong, and the reason why I took your suggestion to use a
separate script file.

> Anyway, what do you think of the multiline split I suggested?

Done, and sorted.

> That's what should have been in the commit message.

Sure.  I copied into the commit message the explanation that I had
written in June's email.

> > > > xi-os-only: freebsd
> > >
> > > Typo.
> >
> > No - it's deliberate so I can switch to and from "everything" to "this
> > only".
> 
> I don't see the point in posting patches to be applied if they contain lots of
> such things that a potential committer would need to catch and include a lot
> of of fixup patches.

I get that you disliked that I disabled the effect of a CI tag by
munging "c" to "x".  I've amended the message to avoid confusion.  But,
lots of what such things ?  "ci-os-only" would be removed before being
pushed anyway.

"catching things" is the first part of the review process, which (as I
understand) is intended to help patch authors to improve their patches.
If you found lots of problems in my patches, I'd need to know about
them; but most of what I heard seem like quibbles about the presentation
of the patches.  It's true that some parts are dirty/unclear, and that
seems reasonable for patches most of which haven't yet received review,
for which I asked whether to pursue the patch at all, and how best to
present them.  This is (or could be) an opportunity to make
improvements.

I renamed the two, related patches to Cluser.pm which said "f!", which
are deliberately separate but looked like "fixup" patches.  Are you
interested in any combination of those three, related changes to move
logic from Makefile to perl ?  If not, we don't need to debate the
merits of spliting the patch.

What about the three, related changes for ccache compression ?

Should these be dropped in favour of meson ?
 - cirrus/vcregress: test modules/contrib with NO_INSTALLCHECK=1
 - vcregress: add alltaptests

I added: "WIP: skip building if only docs have changed"

changesInclude() didn't seem to work right when I first tried to use it.
Eventually, I realized that it seems to use something like "git log",
and not "git diff" (as I'd thought).  It seems to work fine now that I
know what to expect.

git commit --amend --no-edit
git diff --stat @{1}..@{0} # this outputs nothing
git log  --stat @{1}..@{0} # this lists the files changed by the tip commit

It'd be nice to be have cfbot inject this patch into each commitfest
patch for awhile, to make sure everything works as expected.  Same for
the code coverage patch and the doc artifacts patch.  (These patches
currently assume that the base commit is HEAD~1, which is correct for
cfbot, and that would also provide code coverage and docs until such
time as cfbot is updated to apply and preserve the original series of
patches).

-- 
Justin

--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-cirrus-windows-add-compiler_warnings_script.patch"