Subject: Re: CI and test improvements

From: Justin Pryzby <[email-masked]>
Date: Wed, 4 Jan 2023 17:44:24 -0600

On Tue, Nov 22, 2022 at 04:57:44PM -0600, Justin Pryzby wrote:
> I shuffled my branch around and sending now the current "docs" patches,
> but I suppose this is waiting on the "convert CompilerWarnings task to
> meson" patch.

In case it's not, here's a version to do that now.
>From 59bbbce620fdf3c4f228c0569b1fac29beb06988 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Wed, 25 May 2022 21:53:22 -0500
Subject: [PATCH 1/7] cirrus/windows: add compiler_warnings_script

I'm not sure how to write this test in windows shell; it's also easy to
write something that doesn't work in posix sh, since windows shell is
interpretting && and ||...

https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de

See also:
8a1ce5e54f6d144e4f8e19af7c767b026ee0c956
https://cirrus-ci.com/task/6241060062494720
https://cirrus-ci.com/task/6496366607204352

ci-os-only: windows
---
 .cirrus.yml | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 69837bcd5ad..fd461048372 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -564,12 +564,21 @@ task:
 
   build_script: |
     vcvarsall x64
-    ninja -C build
+    ninja -C build |tee build.txt
+    REM Since pipes lose the exit status of the preceding command, rerun the compilation
+    REM without the pipe, exiting now if it fails, to avoid trying to run checks
+    ninja -C build > nul
 
   check_world_script: |
     vcvarsall x64
     meson test %MTEST_ARGS% --num-processes %TEST_JOBS%
 
+  # This should be last, so check_world is run even if there are warnings
+  always:
+    compiler_warnings_script:
+      # this avoids using metachars which would be interpretted by the windows shell
+      - sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0'
+
   on_failure:
     <<: *on_failure_meson
     crashlog_artifacts:
-- 
2.25.1

>From 4edbdf0337c75bd69e536b542a4cd06dac87c33d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 4 Nov 2022 15:43:56 -0500
Subject: [PATCH 2/7] cirrus/macos: update to macos ventura

ci-os-only: macos
---
 .cirrus.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index fd461048372..858454a3d08 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -404,7 +404,7 @@ task:

 task:
-  name: macOS - Monterey - Meson
+  name: macOS - Ventura - Meson
 
   env:
     CPUS: 4 # always get that much for cirrusci macOS instances
@@ -429,7 +429,7 @@ task:
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*(macos|darwin|osx).*'
 
   macos_instance:
-    image: ghcr.io/cirruslabs/macos-monterey-base:latest
+    image: ghcr.io/cirruslabs/macos-ventura-base:latest
 
   sysinfo_script: |
     id
-- 
2.25.1

>From 2f6a7102f717d7fe5e291d6e439fd32b122180a1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 24 Jun 2022 00:09:12 -0500
Subject: [PATCH 3/7] cirrus/freebsd: run with more CPUs+RAM and do not
 repartition

There was some historic problem where tests under freebsd took 8+ minutes (and
before 4a288a37f took 15 minutes).

This reduces test time from 10min to 3min.
4 CPUs 4 tests https://cirrus-ci.com/task/4880240739614720
4 CPUs 6 tests https://cirrus-ci.com/task/4664440120410112 https://cirrus-ci.com/task/4586784884523008
4 CPUs 8 tests https://cirrus-ci.com/task/5001995491737600

6 CPUs https://cirrus-ci.com/task/6678321684545536
8 CPUs https://cirrus-ci.com/task/6264854121021440

See also:
https://www.postgresql.org/message-id/flat/[email-masked]#f36c0b17e33e31e7925e7e5812998686
8 jobs 7min https://cirrus-ci.com/task/6186376667332608

//-os-only: freebsd
---
 .cirrus.yml | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 858454a3d08..2ba6b7cc2d8 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -131,11 +131,9 @@ task:
   name: FreeBSD - 13 - Meson
 
   env:
-    # FreeBSD on GCP is slow when running with larger number of CPUS /
-    # jobs. Using one more job than cpus seems to work best.
-    CPUS: 2
-    BUILD_JOBS: 3
-    TEST_JOBS: 3
+    CPUS: 4
+    BUILD_JOBS: 4
+    TEST_JOBS: 6
 
     CCACHE_DIR: /tmp/ccache_dir
     CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
@@ -160,8 +158,6 @@ task:
 
   ccache_cache:
     folder: $CCACHE_DIR
-  # Work around performance issues due to 32KB block size
-  repartition_script: src/tools/ci/gcp_freebsd_repartition.sh
   create_user_script: |
     pw useradd postgres
     chown -R postgres:postgres .
-- 
2.25.1

>From b55c042ec467a03984a5e2132dc4e7fe0f49fef3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 9 Dec 2022 15:32:50 -0600
Subject: [PATCH 4/7] cirrus/freebsd: define
 ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

See also: 54100f5c6052404f68de9ce7310ceb61f1c291f8

ci-os-only: freebsd
---
 .cirrus.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 2ba6b7cc2d8..34c9d52b624 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -136,7 +136,7 @@ task:
     TEST_JOBS: 6
 
     CCACHE_DIR: /tmp/ccache_dir
-    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
+    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
     CFLAGS: -Og -ggdb
 
   depends_on: SanityCheck
-- 
2.25.1

>From a4ecb1a1bf008052108382f5833345680eec156c Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Tue, 19 Jul 2022 12:38:45 -0500
Subject: [PATCH 5/7] cirrus/warnings: use a single/common 'always' block

ci-os-only: warnings
---
 .cirrus.yml | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 34c9d52b624..123f6957513 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -696,8 +696,8 @@ task:
   # different compilers to build with different combinations of dtrace on/off
   # and cassert on/off.
 
-  # gcc, cassert off, dtrace on
   always:
+    # gcc, cassert off, dtrace on
     gcc_warning_script: |
       time ./configure \
         --cache gcc.cache \
@@ -707,8 +707,7 @@ task:
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} world-bin
 
-  # gcc, cassert on, dtrace off
-  always:
+    # gcc, cassert on, dtrace off
     gcc_a_warning_script: |
       time ./configure \
         --cache gcc.cache \
@@ -718,8 +717,7 @@ task:
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} world-bin
 
-  # clang, cassert off, dtrace off
-  always:
+    # clang, cassert off, dtrace off
     clang_warning_script: |
       time ./configure \
         --cache clang.cache \
@@ -728,8 +726,7 @@ task:
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} world-bin
 
-  # clang, cassert on, dtrace on
-  always:
+    # clang, cassert on, dtrace on
     clang_a_warning_script: |
       time ./configure \
         --cache clang.cache \
@@ -740,8 +737,7 @@ task:
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} world-bin
 
-  # cross-compile to windows
-  always:
+    # cross-compile to windows
     mingw_cross_warning_script: |
       time ./configure \
         --host=x86_64-w64-mingw32 \
@@ -755,7 +751,6 @@ task:
   # Verify docs can be built
   ###
   # XXX: Only do this if there have been changes in doc/ since last build
-  always:
     docs_build_script: |
       time ./configure \
         --cache gcc.cache \
@@ -765,16 +760,15 @@ task:
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} -C doc
 
-  ###
-  # Verify headerscheck / cpluspluscheck succeed
-  #
-  # - Don't use ccache, the files are uncacheable, polluting ccache's
-  #   cache
-  # - Use -fmax-errors, as particularly cpluspluscheck can be very verbose
-  # - XXX have to disable ICU to avoid errors:
-  #   https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
-  ###
-  always:
+    ###
+    # Verify headerscheck / cpluspluscheck succeed
+    #
+    # - Don't use ccache, the files are uncacheable, polluting ccache's
+    #   cache
+    # - Use -fmax-errors, as particularly cpluspluscheck can be very verbose
+    # - XXX have to disable ICU to avoid errors:
+    #   https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
+    ###
     headers_headerscheck_script: |
       time ./configure \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -786,5 +780,4 @@ task:
     headers_cpluspluscheck_script: |
       time make -s cpluspluscheck EXTRAFLAGS='-fmax-errors=10'
 
-  always:
     upload_caches: ccache
-- 
2.25.1

>From e523bd786c034587d6c45079393ba1563cd16eff Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Sat, 26 Feb 2022 19:39:10 -0600
Subject: [PATCH 6/7] cirrus: upload changed html docs as artifacts

This could be done on the client side (cfbot).  One advantage of doing
it here is that fewer docs are uploaded - many patches won't upload docs
at all.

https://www.postgresql.org/message-id/flat/[email-masked]
https://www.postgresql.org/message-id/CAB8KJ=[email-masked]

https://cirrus-ci.com/task/5396696388599808

ci-os-only: html
---
 .cirrus.yml                    | 24 +++++++++++++++++++++++-
 src/tools/ci/copy-changed-docs | 29 +++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/copy-changed-docs

diff --git a/.cirrus.yml b/.cirrus.yml
index 123f6957513..a6a246882c9 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -27,6 +27,14 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  #BASE_COMMIT: HEAD~1
+  # For demo purposes:
+  BASE_COMMIT: HEAD~11
 
 # What files to preserve in case tests fail
 on_failure_ac: &on_failure_ac
@@ -678,6 +686,7 @@ task:
     gcc -v
     clang -v
     export
+    git diff --name-only "$BASE_COMMIT"
 
   ccache_cache:
     folder: $CCACHE_DIR
@@ -748,7 +757,7 @@ task:
       time make -s -j${BUILD_JOBS} world-bin
 
   ###
-  # Verify docs can be built
+  # Verify docs can be built, and upload changed docs as artifacts
   ###
   # XXX: Only do this if there have been changes in doc/ since last build
     docs_build_script: |
@@ -759,6 +768,19 @@ task:
         CLANG="ccache clang"
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} -C doc
+      cp -r doc new-docs
+
+      # Re-build HTML docs from the base commit.
+      git checkout "$BASE_COMMIT" -- doc
+      make -s -C doc clean
+      time make -s -C doc html
+      cp -r doc old-docs
+
+    copy_changed_docs_script:
+      - src/tools/ci/copy-changed-docs "old-docs" "new-docs" "html_docs"
+
+    html_docs_artifacts:
+      paths: ['html_docs/*.html', 'html_docs/*.png', 'html_docs/*.css']
 
     ###
     # Verify headerscheck / cpluspluscheck succeed
diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
new file mode 100755
index 00000000000..1c921a8df6f
--- /dev/null
+++ b/src/tools/ci/copy-changed-docs
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Copy HTML which differ between $old and $new into $outdir
+set -e
+
+old=$1
+new=$2
+outdir=$3
+
+# The index is large and changes often
+skippages="bookindex.html"
+
+mkdir "$outdir"
+cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
+
+changed=`git diff --no-index --name-only "$old"/src/sgml/html "$new"/src/sgml/html` ||
+	[ $? -eq 1 ]
+
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	echo "$f" |grep -Ew "$skippages" >/dev/null &&
+		continue
+
+	cp -v "$f" "$outdir"
+done
+
+exit 0
-- 
2.25.1

>From e0ef15ae513d62a42887dcbe135e2bc2c477074d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 25 Nov 2022 13:57:17 -0600
Subject: [PATCH 7/7] WIP: ci/meson: allow showing only failed tests ..

It's simpler and seems to make more sense to integrate this with
testwrap, rather than to run it after check-world, but only if it
failed, and finding a way to preserve the exit code.

https://www.postgresql.org/message-id/[email-masked]

> It is wasteful to upload thousdands of logfiles to show a single
> failure.  That would make our cirrus tasks faster - compressing and
> uploading the logs takes over a minute.
>
> It's also a lot friendlier to show fewer than 8 pages of test folders to
> search through to find the one that failed.

macos
ci-os-only: linux-meson freebsd
---
 .cirrus.yml        | 13 +++++++------
 src/tools/testwrap |  6 ++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index a6a246882c9..f64aad64756 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -23,6 +23,7 @@ env:
   CHECKFLAGS: -Otarget
   PROVE_FLAGS: --timer
   MTEST_ARGS: --print-errorlogs --no-rebuild -C build
+  PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
   PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
@@ -48,9 +49,9 @@ on_failure_ac: &on_failure_ac
 on_failure_meson: &on_failure_meson
   testrun_artifacts:
     paths:
-      - "build*/testrun/**/*.log"
-      - "build*/testrun/**/*.diffs"
-      - "build*/testrun/**/regress_log_*"
+      - "failed.build*/**/*.log"
+      - "failed.build*/**/*.diffs"
+      - "failed.build*/**/regress_log_*"
     type: text/plain
 
   # In theory it'd be nice to upload the junit files meson generates, so that
@@ -206,10 +207,10 @@ task:
       ulimit -c unlimited
       meson test $MTEST_ARGS --quiet --suite setup
       export LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/:$LD_LIBRARY_PATH"
-      mkdir -p build/testrun
+      mkdir -p build/testrun ${PG_FAILED_TESTDIR}
       build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck --no-instructions -A trust
       echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >> build/runningcheck/postgresql.conf
-      build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l build/testrun/runningcheck.log start
+      build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l ${PG_FAILED_TESTDIR}/runningcheck.log start
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS} --setup running
       build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop
     EOF
@@ -397,7 +398,7 @@ task:
       test_world_32_script: |
         su postgres <<-EOF
           ulimit -c unlimited
-          PYTHONCOERCECLOCALE=0 LANG=C meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
+          PYTHONCOERCECLOCALE=0 LANG=C PG_FAILED_TESTDIR=`pwd`/failed.build-32 meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
         EOF
 
       on_failure:
diff --git a/src/tools/testwrap b/src/tools/testwrap
index 7a64fe76a2d..445ac595afc 100755
--- a/src/tools/testwrap
+++ b/src/tools/testwrap
@@ -44,4 +44,10 @@ if sp.returncode == 0:
 else:
     print('# test failed')
     open(os.path.join(testdir, 'test.fail'), 'x')
+    faileddir = os.getenv('PG_FAILED_TESTDIR')
+    if faileddir:
+        parentdir = os.path.dirname(testdir)
+        newdest = os.path.join(faileddir, os.path.basename(parentdir), os.path.basename(testdir))
+        shutil.copytree(testdir, newdest)
+
 sys.exit(sp.returncode)
-- 
2.25.1

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 17 Jan 2023 11:35:09 -0600

The autoconf system runs all tap tests in t/*.pl, but meson requires
enumerating them in ./meson.build.

This checks for and finds no missing tests in the current tree:

$ for pl in `find src contrib -path '*/t/*.pl'`; do base=${pl##*/}; dir=${pl%/*}; meson=${dir%/*}/meson.build; grep "$base" "$meson" >/dev/null || echo "$base is missing from $meson"; done

However, this finds two real problems and one false-positive with
missing regress/isolation tests:

$ for makefile in `find src contrib -name Makefile`; do for testname in `sed -r '/^(REGRESS|ISOLATION) =/!d; s///; :l; /\\\\$/{s///; N; b l}; s/\n//g' "$makefile"`; do meson=${makefile%/Makefile}/meson.build; grep -Fw "$testname" "$meson" >/dev/null || echo "$testname is missing from $meson"; done; done
guc_privs is missing from src/test/modules/unsafe_tests/meson.build
oldextversions is missing from contrib/pg_stat_statements/meson.build
$(CF_PGP_TESTS) is missing from contrib/pgcrypto/meson.build

I also tried but failed to write something to warn if "meson test" was
run with a list of tests but without tmp_install.  Help wanted.

I propose to put something like this into "SanityCheck".

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Tue, 17 Jan 2023 11:56:42 -0800

Hi,

> enumerating them in ./meson.build.

Yes. It was a mistake that we ever used t/*.pl for make. For one, it means
that make can't control concurrency meaningfully, due to the varying number of
tests run with one prove instance. It's also the only thing that tied us to
prove, which is one hell of a buggy mess.

> $ for pl in `find src contrib -path '*/t/*.pl'`; do base=${pl##*/}; dir=${pl%/*}; meson=${dir%/*}/meson.build; grep "$base" "$meson" >/dev/null || echo "$base is missing from $meson"; done

Likely because I do something similar locally.

# prep
m test --list > /tmp/tests.txt

# Check if all tap tests are known to meson
for f in $(git ls-files|grep -E '(t|test)/.*.pl$'|sort);do t=$(echo $f|sed -E -e 's/^.*\/([^/]*)\/(t|test)\/(.*)\.pl$/\1\/\3/');grep -q -L $t /tmp/tests.txt |\
| echo $f;done

# Check if all regression / isolation tests are known to meson
#
# Expected to find plpgsql due to extra 'src' directory level, src/test/mb
# because it's not run anywhere and sepgsql, because that's not tested yet
for d in $(find ~/src/postgresql -type d \( -name sql -or -name specs \) );do t=$(basename $(dirname $d)); grep -q -L $t /tmp/tests.txt || echo $d; done

> missing regress/isolation tests:

Which the above does *not* test for. Good catch.

I'll push the fix for those as soon as tests passed on my personal repo.

> guc_privs is missing from src/test/modules/unsafe_tests/meson.build

Yep. That got added during the development of the meson port, so it's not too surprising.

> oldextversions is missing from contrib/pg_stat_statements/meson.build

This one however, is odd. Not sure how that happened.

> $(CF_PGP_TESTS) is missing from contrib/pgcrypto/meson.build

Assume that's the false positive?

> run with a list of tests but without tmp_install.  Help wanted.

That doesn't even catch the worst case - when there's tmp_install, but it's
too old.

The proper solution would be to make the creation of tmp_install a dependency
of the relevant tests. Unfortunately meson still auto-propages those to
dependencies of the 'all' target (for historical reasons), and creating the
temp install is too slow on some machines to make that tolerable. I think
there's an open PR to change that. Once that's in a released meson version
that's in somewhat widespread use, we should change that.

The other path forward is to allow running the tests without
tmp_install. There's not that much we'd need to allow running directly from
the source tree - the biggest thing is a way to load extensions from a list of
paths. This option is especially attractive because it'd allow to run
individual tests without a fully built sourcetree. No need to build other
binaries when you just want to test psql, or more extremely, pg_test_timing.

> I propose to put something like this into "SanityCheck".

Perhaps we instead could add it as a separate "meson-only" test? Then it'd
fail on developer's machines, instead of later in CI. We could pass the test
information from the 'tests' array, or it could look at the metadata in
meson-info/intro-tests.json

Greetings,

Andres Freund