Обсуждение: Explicitly enable meson features in CI

Поиск
Список
Период
Сортировка

Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
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

Вложения

Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> 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




Re: Explicitly enable meson features in CI

От
Peter Eisentraut
Дата:
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.)




Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> 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.

+  # 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?


   # 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?

--
Daniel Gustafsson




Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
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



Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> 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




Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Thu, 3 Jul 2025 at 17:07, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > 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?

I think this would make things complicated. Instead, we can set
Windows VS tasks' features in its task, then we can have a one common
feature variable at the top level and have a comment about this
variable not being used in the Windows VS task. Basically the same
with your approach but instead of using a common feature variable and
disabling features from it, we enable them again in the Windows VS
task. I used this approach in the v3-0002.

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

Yes, I think this is better. Done.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Вложения

Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> On 4 Jul 2025, at 09:33, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> On Thu, 3 Jul 2025 at 17:07, Daniel Gustafsson <daniel@yesql.se> wrote:

>> 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?
>
> I think this would make things complicated.

I think you're right.

> Instead, we can set
> Windows VS tasks' features in its task, then we can have a one common
> feature variable at the top level and have a comment about this
> variable not being used in the Windows VS task. Basically the same
> with your approach but instead of using a common feature variable and
> disabling features from it, we enable them again in the Windows VS
> task. I used this approach in the v3-0002.

The proposal in v3 strikes a good balance between avoiding repetition and being
readable.

--
Daniel Gustafsson




Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Tue, 8 Jul 2025 at 12:10, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> Hi,
>
> On Wed, 2 Jul 2025 at 10:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> >
> > Also, libcurl is disabled in the OpenBSD CI task until the fix in this
> > thread [1] is committed.
>
> This fix is committed in 7376e60854 so libcurl is enabled for OpenBSD in v4.

Andres off-list mentioned that if we explicitly enable features for
*all* of the tasks, then none of the tasks will be testing the auto
feature option and I agree with Andres. My suggestion is setting
features to auto for Debian - Meson task. I decided on this because I
think it is the most checked CI task so it would be easier to catch if
one of the features is disabled without anyone noticing.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft



Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> Andres off-list mentioned that if we explicitly enable features for
> *all* of the tasks, then none of the tasks will be testing the auto
> feature option and I agree with Andres. My suggestion is setting
> features to auto for Debian - Meson task. I decided on this because I
> think it is the most checked CI task

Hehe, that's certainly true for me...

> so it would be easier to catch if
> one of the features is disabled without anyone noticing.

Seems reasonable. If we do this, can we rename the job with a "- Meson
Auto" suffix or something, to try to call the difference out
explicitly?

--Jacob



Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> On 10 Jul 2025, at 19:12, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

>> so it would be easier to catch if
>> one of the features is disabled without anyone noticing.
>
> Seems reasonable.

Agreed.

> If we do this, can we rename the job with a "- Meson
> Auto" suffix or something, to try to call the difference out
> explicitly?

+1

--
Daniel Gustafsson




Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Thu, 10 Jul 2025 at 20:12, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > Andres off-list mentioned that if we explicitly enable features for
> > *all* of the tasks, then none of the tasks will be testing the auto
> > feature option and I agree with Andres. My suggestion is setting
> > features to auto for Debian - Meson task. I decided on this because I
> > think it is the most checked CI task
>
> Hehe, that's certainly true for me...
>
> > so it would be easier to catch if
> > one of the features is disabled without anyone noticing.
>
> Seems reasonable. If we do this, can we rename the job with a "- Meson
> Auto" suffix or something, to try to call the difference out
> explicitly?

I think renaming it would be better but then we have two Linux tasks:

- Linux - Debian Bookworm - Autoconf
- Linux - Debian Bookworm - Meson Auto

For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
can rename it as a 'Meson Auto Feature Detection' but that is a bit
longer. Do you have any ideas? If you think 'Meson Auto' is good
enough, we can continue with it, too.

--
Regards,
Nazir Bilal Yavuz
Microsoft



Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Fri, 11 Jul 2025 at 14:00, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> Hi,
>
> On Thu, 10 Jul 2025 at 20:12, Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> >
> > On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > > Andres off-list mentioned that if we explicitly enable features for
> > > *all* of the tasks, then none of the tasks will be testing the auto
> > > feature option and I agree with Andres. My suggestion is setting
> > > features to auto for Debian - Meson task. I decided on this because I
> > > think it is the most checked CI task
> >
> > Hehe, that's certainly true for me...
> >
> > > so it would be easier to catch if
> > > one of the features is disabled without anyone noticing.
> >
> > Seems reasonable. If we do this, can we rename the job with a "- Meson
> > Auto" suffix or something, to try to call the difference out
> > explicitly?
>
> I think renaming it would be better but then we have two Linux tasks:
>
> - Linux - Debian Bookworm - Autoconf
> - Linux - Debian Bookworm - Meson Auto
>
> For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
> can rename it as a 'Meson Auto Feature Detection' but that is a bit
> longer. Do you have any ideas? If you think 'Meson Auto' is good
> enough, we can continue with it, too.

I renamed 'Meson Auto' as 'Meson Auto Feature Detection' in v5.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Вложения

Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Wed, Jul 16, 2025 at 4:12 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
> > can rename it as a 'Meson Auto Feature Detection' but that is a bit
> > longer. Do you have any ideas? If you think 'Meson Auto' is good
> > enough, we can continue with it, too.
>
> I renamed 'Meson Auto' as 'Meson Auto Feature Detection' in v5.

Sorry for the delay -- while that feels a little wordy to me, it's
also trivially changed. Not worth bikeshedding over unless someone
else complains, IMO. :D

Is there anything else in particular you'd like review on? (I've
marked this patchset RfC.)

Thanks,
--Jacob



Re: Explicitly enable meson features in CI

От
Andres Freund
Дата:
Hi,

On 2025-07-16 14:12:22 +0300, Nazir Bilal Yavuz wrote:
> On Fri, 11 Jul 2025 at 14:00, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > On Thu, 10 Jul 2025 at 20:12, Jacob Champion
> > <jacob.champion@enterprisedb.com> wrote:
> > >
> > > On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > > > Andres off-list mentioned that if we explicitly enable features for
> > > > *all* of the tasks, then none of the tasks will be testing the auto
> > > > feature option and I agree with Andres. My suggestion is setting
> > > > features to auto for Debian - Meson task. I decided on this because I
> > > > think it is the most checked CI task
> > >
> > > Hehe, that's certainly true for me...
> > >
> > > > so it would be easier to catch if
> > > > one of the features is disabled without anyone noticing.
> > >
> > > Seems reasonable. If we do this, can we rename the job with a "- Meson
> > > Auto" suffix or something, to try to call the difference out
> > > explicitly?
> >
> > I think renaming it would be better but then we have two Linux tasks:
> >
> > - Linux - Debian Bookworm - Autoconf
> > - Linux - Debian Bookworm - Meson Auto
> >
> > For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
> > can rename it as a 'Meson Auto Feature Detection' but that is a bit
> > longer. Do you have any ideas? If you think 'Meson Auto' is good
> > enough, we can continue with it, too.
> 
> I renamed 'Meson Auto' as 'Meson Auto Feature Detection' in v5.

FWIW, I don't think it's a good idea to the Auto bit to the name.  We have
several special things about various tests, if we add all of them to the task
name, we'll have very long task names. This one would already be

Linux - Debian Bookworm - Meson Auto Features Detection - 32 and 64 Bit build & tests - Alignment, Undefined Behaviour
Sanitizer- IO method=io_uring
 

And the task names would change a lot more, which is also a pain for things
like the commitfest / cfbot web apps.


But it *should* be added to the "SPECIAL:" comment.

Greetings,

Andres Freund



Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Wed, 16 Jul 2025 at 19:02, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2025-07-16 14:12:22 +0300, Nazir Bilal Yavuz wrote:
> > On Fri, 11 Jul 2025 at 14:00, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > > On Thu, 10 Jul 2025 at 20:12, Jacob Champion
> > > <jacob.champion@enterprisedb.com> wrote:
> > > >
> > > > On Thu, Jul 10, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > > > > Andres off-list mentioned that if we explicitly enable features for
> > > > > *all* of the tasks, then none of the tasks will be testing the auto
> > > > > feature option and I agree with Andres. My suggestion is setting
> > > > > features to auto for Debian - Meson task. I decided on this because I
> > > > > think it is the most checked CI task
> > > >
> > > > Hehe, that's certainly true for me...
> > > >
> > > > > so it would be easier to catch if
> > > > > one of the features is disabled without anyone noticing.
> > > >
> > > > Seems reasonable. If we do this, can we rename the job with a "- Meson
> > > > Auto" suffix or something, to try to call the difference out
> > > > explicitly?
> > >
> > > I think renaming it would be better but then we have two Linux tasks:
> > >
> > > - Linux - Debian Bookworm - Autoconf
> > > - Linux - Debian Bookworm - Meson Auto
> > >
> > > For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
> > > can rename it as a 'Meson Auto Feature Detection' but that is a bit
> > > longer. Do you have any ideas? If you think 'Meson Auto' is good
> > > enough, we can continue with it, too.
> >
> > I renamed 'Meson Auto' as 'Meson Auto Feature Detection' in v5.
>
> FWIW, I don't think it's a good idea to the Auto bit to the name.  We have
> several special things about various tests, if we add all of them to the task
> name, we'll have very long task names. This one would already be
>
> Linux - Debian Bookworm - Meson Auto Features Detection - 32 and 64 Bit build & tests - Alignment, Undefined
BehaviourSanitizer - IO method=io_uring 
>
> And the task names would change a lot more, which is also a pain for things
> like the commitfest / cfbot web apps.

I don't have a strong opinion on this. I think the naming change
clarifies things but as you said then we may/should add other specific
settings to the task name. So, I reverted the naming change.

> But it *should* be added to the "SPECIAL:" comment.

Done.

v6 is attached.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Вложения

Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Wed, 16 Jul 2025 at 18:24, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Wed, Jul 16, 2025 at 4:12 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > > For me it looks like 'Meson Auto' can be confused with 'Autoconf'. We
> > > can rename it as a 'Meson Auto Feature Detection' but that is a bit
> > > longer. Do you have any ideas? If you think 'Meson Auto' is good
> > > enough, we can continue with it, too.
> >
> > I renamed 'Meson Auto' as 'Meson Auto Feature Detection' in v5.
>
> Sorry for the delay -- while that feels a little wordy to me, it's
> also trivially changed. Not worth bikeshedding over unless someone
> else complains, IMO. :D

I reverted the naming change in v6 based on Andres' comments [1].

> Is there anything else in particular you'd like review on? (I've
> marked this patchset RfC.)

Not at the moment, thank you so much!

[1] https://postgr.es/m/zltxefhw7bej3o4oxvkfuoa2glbnip3jacp7rvacbrv5oepql3%40gs3lbwg6unmv

--
Regards,
Nazir Bilal Yavuz
Microsoft



Re: Explicitly enable meson features in CI

От
Daniel Gustafsson
Дата:
> On 17 Jul 2025, at 10:33, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> On Wed, 16 Jul 2025 at 18:24, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

>> Is there anything else in particular you'd like review on? (I've
>> marked this patchset RfC.)

Reading over the v6 version posted upthread, I agree with the patch being
ready.  Are you taking care of it Jacob?

-  # 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

I think this should be s/noticable/noticeable/ (which is a nitpick a commmitter
can deal with, no need for a new version IMO).

--
Daniel Gustafsson




Re: Explicitly enable meson features in CI

От
Andrew Dunstan
Дата:


On 2025-07-28 Mo 8:08 AM, Daniel Gustafsson wrote:
On 17 Jul 2025, at 10:33, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
On Wed, 16 Jul 2025 at 18:24, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
Is there anything else in particular you'd like review on? (I've
marked this patchset RfC.)
Reading over the v6 version posted upthread, I agree with the patch being
ready.  Are you taking care of it Jacob?

-  # 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

I think this should be s/noticable/noticeable/ (which is a nitpick a commmitter
can deal with, no need for a new version IMO).


At the same time they should do:

s/need/needs/


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Mon, Jul 28, 2025 at 5:08 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> Reading over the v6 version posted upthread, I agree with the patch being
> ready.  Are you taking care of it Jacob?

I was admittedly a bit nervous about pushing changes to the infra.
Andres, Bilal, do you have a list of things to check after pushing big
changes to .cirrus stuff, or is it just "a CI build still works on my
repo, so all good"?

--Jacob



Re: Explicitly enable meson features in CI

От
Andres Freund
Дата:
Hi,

On 2025-07-28 09:03:38 -0700, Jacob Champion wrote:
> On Mon, Jul 28, 2025 at 5:08 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > Reading over the v6 version posted upthread, I agree with the patch being
> > ready.  Are you taking care of it Jacob?
> 
> I was admittedly a bit nervous about pushing changes to the infra.
> Andres, Bilal, do you have a list of things to check after pushing big
> changes to .cirrus stuff, or is it just "a CI build still works on my
> repo, so all good"?

I don't think there's anything special to check for CI specific changes, so I
guess "... all good" covers it...

Greetings,

Andres Freund



Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Mon, Jul 28, 2025 at 10:48 AM Andres Freund <andres@anarazel.de> wrote:
> I don't think there's anything special to check for CI specific changes, so I
> guess "... all good" covers it...

Sounds good. I will take another look at this with a committer hat and
push Sometime Soon.

Thanks!
--Jacob



Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Mon, Jul 28, 2025 at 11:20 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Sounds good. I will take another look at this with a committer hat and
> push Sometime Soon.

Getting back to this -- for v7 I've rebased over the FASTLINK commit.
I've squashed 0002 and 0003 together, too -- I think it's easier to
see the new envvar hierarchy in one commit.

I also moved the `-Dnls=disabled` setting into a mingw-specific
MESON_FEATURES variable. I think the existing comment in v6 wasn't
really in a place to prevent a developer from adding it to
MESON_COMMON_FEATURES in the future, which would have regressed
967db242c. There shouldn't be a hazard now.

--Jacob

Вложения

Re: Explicitly enable meson features in CI

От
Nazir Bilal Yavuz
Дата:
Hi,

On Tue, 26 Aug 2025 at 02:37, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Mon, Jul 28, 2025 at 11:20 AM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> > Sounds good. I will take another look at this with a committer hat and
> > push Sometime Soon.
>
> Getting back to this -- for v7 I've rebased over the FASTLINK commit.
> I've squashed 0002 and 0003 together, too -- I think it's easier to
> see the new envvar hierarchy in one commit.
>
> I also moved the `-Dnls=disabled` setting into a mingw-specific
> MESON_FEATURES variable. I think the existing comment in v6 wasn't
> really in a place to prevent a developer from adding it to
> MESON_COMMON_FEATURES in the future, which would have regressed
> 967db242c. There shouldn't be a hazard now.

Thank you for doing these!

Patches LGTM. I also checked the CI tasks and everything looks good.

--
Regards,
Nazir Bilal Yavuz
Microsoft



Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Tue, Aug 26, 2025 at 12:27 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> Patches LGTM. I also checked the CI tasks and everything looks good.

Great, thanks! The full diff of the Meson configure output for each
task also looks good. So I'll plan to push v7.

--Jacob



Re: Explicitly enable meson features in CI

От
Jacob Champion
Дата:
On Tue, Aug 26, 2025 at 9:40 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Great, thanks! The full diff of the Meson configure output for each
> task also looks good. So I'll plan to push v7.

Pushed, finally, and all the Cirrus tasks look happy. Thank you very
much for this improvement!

--Jacob