Re: Adding NetBSD and OpenBSD to Postgres CI

Hi,

On Fri, 1 Nov 2024 at 21:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> Thanks for the patch!
>
>
> On 2024-11-01 12:17:00 +0300, Nazir Bilal Yavuz wrote:
> > I made these tasks triggered manually like MinGW task to save CI credits
> > but a related line is commented out for now to trigger CFBot.
>
> Oh, I need to pick my patch which allows repo-level config of which tasks run
> back up. Then we can enable these by default for cfbot, but not for individual
> repos...
>
>
> > +task:
> > + depends_on: SanityCheck
> > + # trigger_type: manual
> > +
> > + env:
> > + # Below are experimentally derived to be a decent choice.
> > + CPUS: 2
> > + BUILD_JOBS: 8
> > + TEST_JOBS: 8
> > +
> > + CIRRUS_WORKING_DIR: /home/postgres/postgres
>
> Why do you need to set that?

Otherwise, it is set to /tmp path and the size of /tmp mount point is
1.2GB, which is not enough.

> > + 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.

> > + # Postgres tries to set 'LC_COLLATE' to 'en_US.UTF-8' but it is not
> > + # changeable. Initdb fails because of that. So, LANG is forced to be 'C'.
> > + LANG: "C"
> > + LC_ALL: "C"
>
> > + 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?

> For other OSs we have stanzas like
>
> setup_additional_packages_script: |
> #apt-get update
> #DEBIAN_FRONTEND=noninteractive apt-get -y install ...
>
> it'd be good to have something similar for openbsd/netbsd, given that most
> won't be as familiar with that.

Done.

> > + <<: *openbsd_task_template
> > + sysinfo_script: |
> > + locale
> > + id
> > + uname -a
> > + ulimit -a -H && ulimit -a -S
> > + env
> > +
> > + ccache_cache:
> > + folder: $CCACHE_DIR
> > +
> > + create_user_script: |
> > + useradd postgres
> > + chown -R postgres:users /home/postgres
> > + mkdir -p ${CCACHE_DIR}
> > + chown -R postgres:users ${CCACHE_DIR}
> > +
> > + # -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 debug \
>
> I suspect it'd be good to either add -Og to cflags (as done in a bunch of
> other tasks) or to use debugoptimized, given that the tests on these machines
> are fairly slow.

Done.

> > + -Dcassert=true -Dssl=openssl ${UUID} \
> > + -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
> > + ${INCLUDE_DIRS} \
> > + build
> > + EOF
>
> Should probably enable injection points.

Done.

> > + build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
> > + upload_caches: ccache
> > +
> > + test_world_script: |
> > + su postgres <<-EOF
> > + ulimit -c unlimited
> > + # Otherwise tests will fail on OpenBSD, due to the lack of enough processes.
> > + ulimit -p 256
> > + meson test $MTEST_ARGS --num-processes ${TEST_JOBS}
> > + EOF
> > +
> > + on_failure:
> > + <<: *on_failure_meson
> > +
> > +
>
> 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.

--
Regards,
Nazir Bilal Yavuz
Microsoft