Thread: Explicitly enable meson features in CI
Hi,
Right now, many features are set to auto in Meson builds in CI. This
means Meson tries to detect these features automatically but does not
report an error if it fails to find them. Jacob stated [1] that this
behavior can lead to issues, such as missing problems in the build
system. Additionally, since CI images are updated automatically, an
update could cause a feature to go missing without any warning.
Therefore, Jacob suggested explicitly enabling these features in the
Meson build configuration. The attached patch implements this change.
I manually checked features from the end of the 'meson setup'
command's output in each task and stored these enabled features in the
MESON_FEATURES variable. I think this should be enough but of course
an automated way would be better if there is any.
One thing I’m unsure about the patch is that all these features are
stored in the MESON_FEATURES environment variable in each task. I
wonder if it might be clearer to rename these variables to
${TASK_NAME}_MESON_FEATURES to avoid confusion.
Also, libcurl is disabled in the OpenBSD CI task until the fix in this
thread [1] is committed.
Any feedback would be appreciated.
[1] https://www.postgresql.org/message-id/CAOYmi%2BkdR218ke2zu74oTJvzYJcqV1MN5%3DmGAPqZQuc79HMSVA%40mail.gmail.com
--
Regards,
Nazir Bilal Yavuz
Microsoft
Attachment
> On 2 Jul 2025, at 09:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > Right now, many features are set to auto in Meson builds in CI. This > means Meson tries to detect these features automatically but does not > report an error if it fails to find them. Jacob stated [1] that this > behavior can lead to issues, such as missing problems in the build > system. Additionally, since CI images are updated automatically, an > update could cause a feature to go missing without any warning. > Therefore, Jacob suggested explicitly enabling these features in the > Meson build configuration. The attached patch implements this change. Big +1 on this, thanks for tackling it. -- Daniel Gustafsson
On 02.07.25 09:22, Nazir Bilal Yavuz wrote:
> One thing I’m unsure about the patch is that all these features are
> stored in the MESON_FEATURES environment variable in each task. I
> wonder if it might be clearer to rename these variables to
> ${TASK_NAME}_MESON_FEATURES to avoid confusion.
I would hope we could do this without so much repetition. Why not a
global variable and then have each task override as needed?
(It would also be nice to extra the corresponding configure arguments
into a similar variable, so we can easily keep this aligned.)
Here is a sketch of what I mean:
env:
...
PG_TEST_EXTRA: ...
MESON_COMMON_ARGS: -Dauto_features=disabled -Ddocs=enabled ...
CONFIGURE_COMMON_ARGS: --with-gssapi --with-icu --with-ldap ...
...
configure_script: |
su postgres <<-EOF
meson setup \
${MESON_COMMON_ARGS} -Dpltcl=disabled \
--buildtype=debugoptimized \
--pkg-config-path ${PKGCONFIG_PATH} \
-Dcassert=true -Dinjection_points=true \
-Dssl=openssl ${UUID} ${TCL} \
-DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
EOF
(There are additional opportunities to refactor this, such as the
-DPG_TEST_EXTRA option that is repeated everywhere.)
Hi, Thank you for looking into this! On Thu, 3 Jul 2025 at 16:21, Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 3 Jul 2025, at 09:27, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > > On Wed, 2 Jul 2025 at 14:33, Peter Eisentraut <peter@eisentraut.org> wrote: > > >> Here is a sketch of what I mean: > >> > >> env: > >> ... > >> PG_TEST_EXTRA: ... > >> MESON_COMMON_ARGS: -Dauto_features=disabled -Ddocs=enabled ... > >> CONFIGURE_COMMON_ARGS: --with-gssapi --with-icu --with-ldap ... > > > > I agree that this repetition does not look good but my idea was to be > > able to see which features are enabled at one glance. I tried to apply > > grouping in v2: > > FWIW, I didn't mind the repetition in v1 but I won't argue against the current > approach either. I feel the same. > > + # Like 'MESON_COMMON_FEATURES' but not shared with 'Windows - VS' task too > + MESON_NON_VS_FEATURES: >- > > I'm not a fan of this name, it feel a bit unintuitive to describe what it isn't > instead of what it is. How about MESON_LINUX_UNIX_FEATURES or something along > those lines? I agree that MESON_NON_VS_FEATURES is not intuitive but can MinGW be considered as LINUX or UNIX? > > # disable -Dnls as the number of files it creates cause a noticable slowdown > configure_script: | > - %BASH% -c "meson setup -Ddebug=true -Doptimization=g -Dcassert=true -Dinjection_points=true -Db_pch=true -Dnls=disabled-DTAR=%TAR > % build" > + %BASH% -c "meson setup %MESON_COMMON_PG_CONFIG_ARGS% -Ddebug=true -Doptimization=g -Db_pch=true %MESON_COMMON_NON_VS_FEATURES%-D > TAR=%TAR% build" > > The MinGW build no longer disables nls, or am I missing something? Auto features are already disabled. So, there is no need to disable nls manually. By saying that, it would be better to remove this comment now. -- Regards, Nazir Bilal Yavuz Microsoft
> On 3 Jul 2025, at 15:50, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > On Thu, 3 Jul 2025 at 16:21, Daniel Gustafsson <daniel@yesql.se> wrote: >> + # Like 'MESON_COMMON_FEATURES' but not shared with 'Windows - VS' task too >> + MESON_NON_VS_FEATURES: >- >> >> I'm not a fan of this name, it feel a bit unintuitive to describe what it isn't >> instead of what it is. How about MESON_LINUX_UNIX_FEATURES or something along >> those lines? > > I agree that MESON_NON_VS_FEATURES is not intuitive but can MinGW be > considered as LINUX or UNIX? That is an excellent question, according to our fine documentation it is a "Unix-like build environment". An alternative approach would be to instead of having opt-in's for non-Windows have opt-outs for Windows, ie a MESON_WINDOWS_EXCLUDES which does =disabled on the specific features we dont want on Windows? >> The MinGW build no longer disables nls, or am I missing something? > > Auto features are already disabled. So, there is no need to disable > nls manually. By saying that, it would be better to remove this > comment now. Aha, that explains it. Maybe reword the comment to retain the knowledge for the future without making it sound like something is missing from the command. How about something like: - # disable -Dnls as the number of files it creates cause a noticable slowdown + # -Dnls need to be disabled as the number of files it creates cause a + # noticable slowdown -- Daniel Gustafsson