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?
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.
(The warnings are all like
C:\python\Include\pyconfig.h(117): warning C4005: 'MS_WIN64': macro
redefinition
so possibly a single fix can address them all.)
> [PATCH 2/8] cirrus/freebsd: run with more CPUs+RAM and do not repartition
I don't know enough about this. Maybe Andres or Thomas want to take
this. No concerns if it's safe.
> [PATCH 3/8] cirrus/freebsd: define
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
Looks sensible.
> [PATCH 4/8] pg_upgrade: tap test: exercise --link and --clone
I haven't been able to get any changes to the test run times outside of
noise from this. But some more coverage is sensible in any case.
I'm concerned that with this change, the only platform that tests --copy
is Windows, but Windows has a separate code path for copy. So we should
leave one Unix platform to test --copy. Maybe have FreeBSD test --link
and macOS test --clone and leave the others with --copy?
> [PATCH 5/8] WIP: ci/meson: allow showing only failed tests ..
7e09035f588 WIP: ci/meson: allow showing only failed tests ..
I'm not sure I like this one. I sometimes look up the logs of non-failed
tests to compare them with failed tests, to get context to could lead to
failures. Maybe we can make this behavior adjustable. But I've not been
bothered by the current behavior.
It's adjustable by un/setting the environment variable.
I'm surprised to hear that anyone using cirrusci (with or without cfbot)
wouldn't prefer the behavior this patch implements. It's annoying to
search find the logs for the (typically exactly one) failing test in
cirrus' directory of 200some test artifacts. We're also uploading a lot
of logs for every failure. (But I suppose this might break cfbot's new
client side parsing of things like build logs...)
One thing that actually annoys me that is that a successful run does not
upload any test artifacts at all. So, I guess I'm just of a different
opinion here.
> [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.)
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.
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>.
> [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.
How useful is this actually? People who want to look at the docs can
build them locally. There are no platform dependencies or anything like
that where having them built elsewhere is of advantage.
