Re: Adding NetBSD and OpenBSD to Postgres CI

Hi,

On 2024-11-12 11:38:11 +0300, Nazir Bilal Yavuz wrote:
> On Fri, 1 Nov 2024 at 21:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > + CCACHE_DIR: /tmp/ccache_dir
> > > +
> > > + PATH: /usr/sbin:$PATH
> > > +
> > > + # Postgres interprets LANG as a 'en_US.UTF-8' but it is 'C', then
> >
> > What does "Postgres interprets LANG as a 'en_US.UTF-8'" mean?
>
> It was because initdb was failing on NetBSD when the LANG and LC_ALL
> is not set to C. I rephrased the comment and moved this under NetBSD
> task.

Do you happen to have a reference to the failure? The environment variables +
the exact error message would be good. Kinda feels like that shouldn't
happen with a default netbsd install.

> > > + matrix:
> > > + - name: NetBSD - 10 - Meson
> > > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*netbsd.*'
> > > + env:
> > > + IMAGE_FAMILY: pg-ci-netbsd-postgres
> > > + INCLUDE_DIRS: -Dextra_lib_dirs=/usr/pkg/lib -Dextra_include_dirs=/usr/pkg/include
> > > + <<: *netbsd_task_template
> > > +
> > > + - name: OpenBSD - 7 - Meson
> > > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*openbsd.*'
> > > + env:
> > > + IMAGE_FAMILY: pg-ci-openbsd-postgres
> > > + INCLUDE_DIRS: -Dextra_include_dirs=/usr/local/include -Dextra_lib_dirs=/usr/local/lib
> > > + UUID: -Duuid=e2fs
> >
> > Shouldn't something be added to PKG_CONFIG_PATH / --pkg-config-path?
>
> I don't think so. Both OSes are able to find pkgconfig at
> '/usr/pkg/bin/pkg-config'. Am I missing something?

--pkg-config-path is about the the path to pkg-config files, not the path to
the pkg-config binary. If set we shouldn't need the
extra_lib_dirs/extra_include_dirs, I think.

> > Right now you don't seem to be collecting core files - but you're still
> > enabling them via ulimit -c unlimited. At least we shouldn't use ulimit -c
> > unlimited without collecting core files, but it'd probably be better to add
> > support for collecting core files. Shouldn't be too hard.
>
> Done. I separated this patch to make review easier.

+1

> From cbea598b11e85b5c7090ca8e9cc05c35f0359f54 Mon Sep 17 00:00:00 2001
> From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> Date: Thu, 7 Nov 2024 15:48:31 +0300
> Subject: [PATCH v2 1/3] Fix meson could not find bsd_auth.h
>
> bsd_auth.h file needs to be compiled together with the 'sys/types.h' as
> it has missing type definitions.
>
> See synopsis at https://man.openbsd.org/authenticate.3
> ---
> meson.build | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 5b0510cef78..84107955d5d 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -551,7 +551,8 @@ test_c_args = cppflags + cflags
> bsd_authopt = get_option('bsd_auth')
> bsd_auth = not_found_dep
> if cc.check_header('bsd_auth.h', required: bsd_authopt,
> - args: test_c_args, include_directories: postgres_inc)
> + args: test_c_args, prefix: '#include <sys/types.h>',
> + include_directories: postgres_inc)
> cdata.set('USE_BSD_AUTH', 1)
> bsd_auth = declare_dependency()
> endif
> --

Was about to apply that, but then started to wonder if we don't need the same
for configure? And it sure looks like that has the same problem?

Which also confuses me some, at some point this presumably worked?

> From cd5bb66a55e5226b543f5b7db9128cfa48e338e3 Mon Sep 17 00:00:00 2001
> From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> Date: Mon, 11 Nov 2024 13:23:22 +0300
> Subject: [PATCH v2 2/3] Add NetBSD and OpenBSD tasks to the Postgres CI
>
> NetBSD and OpenBSD Postgres CI images are generated [1] but their tasks
> are not added to the upstream Postgres yet. This patch adds them.
>
> Note: These tasks will be triggered manually to save CI credits but a
> related line is commented out for now to trigger CFBot.
>
> Author: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
>
> [1] https://github.com/anarazel/pg-vm-images
> ---
> .cirrus.tasks.yml | 84 +++++++++++++++++++++++++++++++++++++++++++++++
> .cirrus.yml | 10 ++++++
> 2 files changed, 94 insertions(+)
>
> diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
> index fc413eb11ef..f338af902aa 100644
> --- a/.cirrus.tasks.yml
> +++ b/.cirrus.tasks.yml
> @@ -213,6 +213,90 @@ task:
> cores_script: src/tools/ci/cores_backtrace.sh freebsd /tmp/cores
>
>
> +task:
> + depends_on: SanityCheck
> + # trigger_type: manual
> +
> + env:
> + # Below are experimentally derived to be a decent choice.
> + CPUS: 2

For cfbot it turns out to be easier to just use 4 cpus for everything. What
time difference do you get from 2 vs 4 CPUs?

> + BUILD_JOBS: 8
> + TEST_JOBS: 8

8 build/test jobs for 2 cpus sounds unlikely to be close to optimal. A bit
higher makes sense, but 4x?

> + CIRRUS_WORKING_DIR: /home/postgres/postgres

I'd add a comment explaining why we're setting this.

> + CCACHE_DIR: /tmp/ccache_dir

And is it ok to put the ccache dir here, given the limited size of /tmp?

> + PATH: /usr/sbin:$PATH
> +
> + matrix:
> + - name: NetBSD - 10 - Meson

Given the image name doesn't include the version it seems we shouldn't include
it here either...

> + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*netbsd.*'
> + env:
> + IMAGE_FAMILY: pg-ci-netbsd-postgres
> + INCLUDE_DIRS: -Dextra_include_dirs=/usr/pkg/include -Dextra_lib_dirs=/usr/pkg/lib
> + # initdb fails with: 'invalid locale settings' error on NetBSD.
> + # Force 'LANG' and 'LC_*' variables to be 'C'.
> + LANG: "C"
> + LC_ALL: "C"
> + setup_additional_packages_script: |
> + #pkgin -y install ...
> + <<: *netbsd_task_template
> +
> + - name: OpenBSD - 7 - Meson
> + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*openbsd.*'
> + env:
> + IMAGE_FAMILY: pg-ci-openbsd-postgres
> + INCLUDE_DIRS: -Dextra_include_dirs=/usr/local/include -Dextra_lib_dirs=/usr/local/lib
> + UUID: -Duuid=e2fs

So for netbsd we're not using any uuid support? Ah, I see, it's documented
further down. Maybe reference that, or move the comment around?

> + # -Duuid=bsd is not set since 'bsd' uuid option
> + # is not working on NetBSD & OpenBSD. See
> + # https://www.postgresql.org/message-id/17358-89806e7420797025@postgresql.org
> + # And other uuid options are not available on NetBSD.
> + configure_script: |
> + su postgres <<-EOF
> + meson setup \
> + --buildtype=debugoptimized \
> + -Dcassert=true -Dinjection_points=true \
> + -Dssl=openssl ${UUID} \
> + -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
> + ${INCLUDE_DIRS} \
> + build
> + EOF

https://cirrus-ci.com/task/4879081273032704?logs=configure#L320

[14:48:50.729] Run-time dependency krb5-gssapi found: NO (tried pkgconfig and cmake)
[14:48:50.729] Library gssapi_krb5 found: NO

https://cirrus-ci.com/task/6286456156585984?logs=configure#L109

[14:49:28.049] Run-time dependency krb5-gssapi found: NO (tried pkgconfig and cmake)
[14:49:28.049] Library gssapi_krb5 found: NO

Is gss really not available / installed on either?

https://cirrus-ci.com/task/4879081273032704?logs=configure#L49-L51
[14:48:50.729] Run-time dependency tcl found: NO (tried pkgconfig and cmake)
[14:48:50.729] Library tcl found: NO
[14:48:50.729] Has header "tcl.h" with dependency -ltcl: NO

Is TCL not available on openbsd?

> + build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
> + upload_caches: ccache
> +
> + test_world_script: |
> + su postgres <<-EOF
> + # Otherwise tests will fail on OpenBSD, due to the lack of enough processes.
> + ulimit -p 256

Should we also increase the number of semaphores?

https://postgr.es/m/db2773a2-aca0-43d0-99c1-060efcd9954e%40gmail.com

Perhaps it'd be better to update the system config earlier in the openbsd
specific portion of the test?

> build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop || true
> EOF
> <<: *on_failure_meson
> - cores_script: src/tools/ci/cores_backtrace.sh freebsd /tmp/cores
> + cores_script: src/tools/ci/cores_backtrace.sh bsd /tmp/cores

Hm, what's the deal with this change?

> on_failure:
> <<: *on_failure_meson
> + cores_script: |
> + # Although OSes are forced to core dump inside ${CORE_DUMP_DIR}, they may
> + # not obey this. So, move core files to the ${CORE_DUMP_DIR} directory.
> + find build/ -maxdepth 1 -type f -name '*.core' -exec mv '{}' ${CORE_DUMP_DIR} \;
> + src/tools/ci/cores_backtrace.sh ${OS_NAME} ${CORE_DUMP_DIR}

s/OSs are forced/we try to configure the OS/

Is -maxdepth 1 really sufficient? The tests run in subdirectories, don't they?

Greetings,

Andres Freund