Date: Wed, 15 Mar 2023 09:56:12 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Andres Freund <andres@anarazel.de>,
	Andrew Dunstan <andrew@dunslane.net>,
	pgsql-hackers@lists.postgresql.org, Noah Misch <noah@leadboat.com>,
	Michael Paquier <michael@paquier.xyz>,
	Anastasia Lubennikova <lubennikovaav@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	samay sharma <smilingsamay@gmail.com>
Subject: Re: CI and test improvements
Message-ID: <ZBHcjJDnLmUArXj3@telsasoft.com>
References: <20221121224542.p2zapvyvb7objluw@alap3.anarazel.de>
 <20221122225744.GF11463@telsasoft.com>
 <CA+hUKGL=z6-kU80nPTjWqV=Xo=mpM36Lca=ngTr4LqcCf=tHzQ@mail.gmail.com>
 <CA+hUKG+kFpXfCsJQPJApkk7WrBFYYSao0V8q+cj88KQBV8XYRQ@mail.gmail.com>
 <1441145.1675300332@sss.pgh.pa.us>
 <CA+hUKGLNyuvxTcchJe+DK_=_+fZTYCKA4c4o8eDwO5OYc2=qXQ@mail.gmail.com>
 <20230203142656.GA1653@telsasoft.com>
 <96832c09-db85-8443-c654-dae928816830@enterprisedb.com>
 <ZA/+mKDX9zWfhD3v@telsasoft.com>
 <e3e8ef4b-fa7e-7c43-cd28-5be34cbdfb13@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e3e8ef4b-fa7e-7c43-cd28-5be34cbdfb13@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZBHcjJDnLmUArXj3%40telsasoft.com>
Precedence: bulk

On Wed, Mar 15, 2023 at 10:58:41AM +0100, Peter Eisentraut wrote:
> On 14.03.23 05:56, Justin Pryzby wrote:
> > I'm soliticing feedback on those patches that I've sent recently - I've
> > elided patches if they have some unresolved issue.
> 
> > [PATCH 1/8] cirrus/windows: add compiler_warnings_script
> 
> Needs a better description of what it actually does.  (And fewer "I'm not
> sure how to write this ..." comments ;-) )  It looks like it would fail the
> build if there is a compiler warning in the Windows VS task? Shouldn't that
> be done in the CompilerWarnings task?

The goal is to fail due to warnings only after running tests.

https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de
"Probably worth scripting something to make the windows task error out
if there had been warnings, but only after running the tests."

CompilerWarnings runs in a linux environment running with -Werror.  
This patch scrapes warnings out of MSVC, since (at least historically)
it's too slow to run a separate windows VM to compile with -Werror.

> Also, I see a bunch of warnings in the current output from that task. These
> should be cleaned up in any case before we can let a thing like this loose.

Yeah (and I mentioned those myself).  As it stands, my patch also
"breaks" everytime someone's else's patch introduces warnings.  I
included links demonstrating its failures.

I agree that it's not okay to merge the patch when it's currently
failing, but I cannot dig into that other issue right now.

> > [PATCH 6/8] cirrus: code coverage
> 
> This adds -Db_coverage=true to the FreeBSD task.  This has a significant
> impact on the build time.  (+50% at least, it appears.)

Yes - but with the CPUs added by the prior patch, the freebsd task is
faster than it is currently.  And its 8min runtime would match the other
tasks well.

> I'm not sure the approach here makes sense.  For example, if you add a new
> test, the set of changed files is just that test.  So you won't get any
> report on what coverage change the test has caused.

The coverage report that I proposed clearly doesn't handle that case -
it's not intended to.

Showing a full coverage report is somewhat slow to generate, probably
unreasonable to upload for every patch, every day, and not very
interesting since it's at least 99% duplicative.  The goal is to show a
coverage report for new code for every patch.  What fraction of the time
do you think the patch author, reviewer or committer have looked at a
coverage report?  It's not a question of whether it's possible to do so
locally, but of whether it's actually done.

> Also, I don't think I trust the numbers from the meson coverage stuff yet.
> See for example
> <https://www.postgresql.org/message-id/Y/3AI+/MqKcjLk/T@paquier.xyz>.

I'm not using the meson coverage target.  I could instead add
CFLAGS=--coverage.  Anyway, getting a scalar value like "83%" might be
interesting to show in cfbot, but it's not the main goal here.

> > [PATCH 7/8] cirrus: upload changed html docs as artifacts
> > [PATCH 8/8] +html index file
> 
> This builds the docs twice and then analyzes the differences between the two
> builds.  This also affects the build times quite significantly.

The main goal is to upload the changed docs.

> People who want to look at the docs can build them locally.

This makes the docs for every patch available for reviewers, without
needing a build environment.  An easy goal would be if documentation for
every patch was reviewed by a native english speaker.  Right now that's
not consistently true.

> How useful is this actually?

I'm surprised if there's any question about the merits of making
documentation easily available for review.  Several people have agreed;
one person mailed me privately specifically to ask how to show HTML docs
on cirrusci.

Anyway, all this stuff is best addressed either before or after the CF.
I'll kick the patch forward.  Thanks for looking.

-- 
Justin