meson: Don't build tests when CLang lacks -fsemantic-interposition

[PATCH] meson: Don't build tests when CLang lacks -fsemantic-interposition

Posted by Michal Privoznik 1 year, 1 month ago

There are some CLang versions that do not support
-fsemantic-interposition. If that's the case, the code is
optimized so much that our mocking no longer works.

Therefore, disable tests and produce a warning.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

Technically, this is a v2 of:

https://listman.redhat.com/archives/libvir-list/2023-March/238943.html

but a different approach is implemented, so I'm sending it anew.

 meson.build | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index a0682e8d0b..c15003ce02 100644
--- a/meson.build
+++ b/meson.build
@@ -2035,8 +2035,18 @@ subdir('src')
 
 subdir('tools')
 
-build_tests = not get_option('tests').disabled()
-if build_tests
+build_tests = [ not get_option('tests').disabled() ]
+if build_tests[0] and \
+   cc.get_id() == 'clang' and \
+   not supported_cc_flags.contains('-fsemantic-interposition') \
+   and get_option('optimization') != '0'
+   # If CLang doesn't support -fsemantic-interposition then our
+   # mocking doesn't work. The best we can do is to not run the
+   # test suite.
+   build_tests = [ false, '!!! Forcibly disabling tests because CLang lacks -fsemantic-interposition. Update CLang or disable optimization !!!' ]
+endif
+
+if build_tests[0]
   subdir('tests')
 endif
 
-- 
2.39.2

Re: [PATCH] meson: Don't build tests when CLang lacks -fsemantic-interposition

Posted by Martin Kletzander 1 year, 1 month ago

On Tue, Mar 21, 2023 at 05:47:19PM +0100, Michal Privoznik wrote:
>There are some CLang versions that do not support
>-fsemantic-interposition. If that's the case, the code is
>optimized so much that our mocking no longer works.
>
>Therefore, disable tests and produce a warning.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
>
>Technically, this is a v2 of:
>
>https://listman.redhat.com/archives/libvir-list/2023-March/238943.html
>
>but a different approach is implemented, so I'm sending it anew.
>
> meson.build | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/meson.build b/meson.build
>index a0682e8d0b..c15003ce02 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -2035,8 +2035,18 @@ subdir('src')
>
> subdir('tools')
>
>-build_tests = not get_option('tests').disabled()
>-if build_tests
>+build_tests = [ not get_option('tests').disabled() ]
>+if build_tests[0] and \
>+   cc.get_id() == 'clang' and \
>+   not supported_cc_flags.contains('-fsemantic-interposition') \
>+   and get_option('optimization') != '0'
>+   # If CLang doesn't support -fsemantic-interposition then our
>+   # mocking doesn't work. The best we can do is to not run the
>+   # test suite.
>+   build_tests = [ false, '!!! Forcibly disabling tests because CLang lacks -fsemantic-interposition. Update CLang or disable optimization !!!' ]
>+endif
>+

So at first I wanted to suggest something like:

if cc.get_id() == 'clang' and \
    not supported_cc_flags.contains('-fsemantic-interposition') \
    and get_option('optimization') != '0'
   # If CLang doesn't support -fsemantic-interposition then our
   # mocking doesn't work. The best we can do is to not run the
   # test suite.
   if get_option('tests').enabled()
     error('Cannot enable tests because CLang lacks -fsemantic-interposition. Update CLang or disable optimization')
   else
     build_tests = false
   endif
else
   build_tests = not get_option('tests').disabled()
endif

Which would simply error out if you want the tests to be enabled on such
system.  However that has the downside that some potential contributor
might not notice the tests are being disabled and we would also have to
have another place where to track the enablement of tests/optimization,
in our CI.  It is possible, but after some tries I think your approach
is better and the main thing is that we need something to be pushed in
order for us not to get into failing CI fatigue which I've seen in other
projects where CI failures are just being dismissed after couple of
issues.  So:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>+if build_tests[0]
>   subdir('tests')
> endif
>
>-- 
>2.39.2
>

Re: [PATCH] meson: Don't build tests when CLang lacks -fsemantic-interposition

Posted by Michal Prívozník 1 year, 1 month ago

On 3/24/23 08:33, Martin Kletzander wrote:
> On Tue, Mar 21, 2023 at 05:47:19PM +0100, Michal Privoznik wrote:
>> There are some CLang versions that do not support
>> -fsemantic-interposition. If that's the case, the code is
>> optimized so much that our mocking no longer works.
>>
>> Therefore, disable tests and produce a warning.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> Technically, this is a v2 of:
>>
>> https://listman.redhat.com/archives/libvir-list/2023-March/238943.html
>>
>> but a different approach is implemented, so I'm sending it anew.
>>
>> meson.build | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index a0682e8d0b..c15003ce02 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -2035,8 +2035,18 @@ subdir('src')
>>
>> subdir('tools')
>>
>> -build_tests = not get_option('tests').disabled()
>> -if build_tests
>> +build_tests = [ not get_option('tests').disabled() ]
>> +if build_tests[0] and \
>> +   cc.get_id() == 'clang' and \
>> +   not supported_cc_flags.contains('-fsemantic-interposition') \
>> +   and get_option('optimization') != '0'
>> +   # If CLang doesn't support -fsemantic-interposition then our
>> +   # mocking doesn't work. The best we can do is to not run the
>> +   # test suite.
>> +   build_tests = [ false, '!!! Forcibly disabling tests because CLang
>> lacks -fsemantic-interposition. Update CLang or disable optimization
>> !!!' ]
>> +endif
>> +
> 
> So at first I wanted to suggest something like:
> 
> if cc.get_id() == 'clang' and \
>    not supported_cc_flags.contains('-fsemantic-interposition') \
>    and get_option('optimization') != '0'
>   # If CLang doesn't support -fsemantic-interposition then our
>   # mocking doesn't work. The best we can do is to not run the
>   # test suite.
>   if get_option('tests').enabled()
>     error('Cannot enable tests because CLang lacks
> -fsemantic-interposition. Update CLang or disable optimization')
>   else
>     build_tests = false
>   endif
> else
>   build_tests = not get_option('tests').disabled()
> endif
> 
> Which would simply error out if you want the tests to be enabled on such
> system.  However that has the downside that some potential contributor
> might not notice the tests are being disabled and we would also have to
> have another place where to track the enablement of tests/optimization,
> in our CI.  It is possible, but after some tries I think your approach
> is better and the main thing is that we need something to be pushed in
> order for us not to get into failing CI fatigue which I've seen in other
> projects where CI failures are just being dismissed after couple of
> issues.  So:
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Thanks. I think it's important to note that this affects systems with
old CLang and I don't expect many developers there (in fact none).
Except for our CI which can be easily fixed - just pass -O0 in CFLAGS.

Michal

Re: [PATCH] meson: Don't build tests when CLang lacks -fsemantic-interposition

Posted by Martin Kletzander 1 year, 1 month ago

On Fri, Mar 24, 2023 at 11:35:52AM +0100, Michal Prívozník wrote:
>On 3/24/23 08:33, Martin Kletzander wrote:
>> On Tue, Mar 21, 2023 at 05:47:19PM +0100, Michal Privoznik wrote:
>>> There are some CLang versions that do not support
>>> -fsemantic-interposition. If that's the case, the code is
>>> optimized so much that our mocking no longer works.
>>>
>>> Therefore, disable tests and produce a warning.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>
>>> Technically, this is a v2 of:
>>>
>>> https://listman.redhat.com/archives/libvir-list/2023-March/238943.html
>>>
>>> but a different approach is implemented, so I'm sending it anew.
>>>
>>> meson.build | 14 ++++++++++++--
>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index a0682e8d0b..c15003ce02 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -2035,8 +2035,18 @@ subdir('src')
>>>
>>> subdir('tools')
>>>
>>> -build_tests = not get_option('tests').disabled()
>>> -if build_tests
>>> +build_tests = [ not get_option('tests').disabled() ]
>>> +if build_tests[0] and \
>>> +   cc.get_id() == 'clang' and \
>>> +   not supported_cc_flags.contains('-fsemantic-interposition') \
>>> +   and get_option('optimization') != '0'
>>> +   # If CLang doesn't support -fsemantic-interposition then our
>>> +   # mocking doesn't work. The best we can do is to not run the
>>> +   # test suite.
>>> +   build_tests = [ false, '!!! Forcibly disabling tests because CLang
>>> lacks -fsemantic-interposition. Update CLang or disable optimization
>>> !!!' ]
>>> +endif
>>> +
>>
>> So at first I wanted to suggest something like:
>>
>> if cc.get_id() == 'clang' and \
>>    not supported_cc_flags.contains('-fsemantic-interposition') \
>>    and get_option('optimization') != '0'
>>   # If CLang doesn't support -fsemantic-interposition then our
>>   # mocking doesn't work. The best we can do is to not run the
>>   # test suite.
>>   if get_option('tests').enabled()
>>     error('Cannot enable tests because CLang lacks
>> -fsemantic-interposition. Update CLang or disable optimization')
>>   else
>>     build_tests = false
>>   endif
>> else
>>   build_tests = not get_option('tests').disabled()
>> endif
>>
>> Which would simply error out if you want the tests to be enabled on such
>> system.  However that has the downside that some potential contributor
>> might not notice the tests are being disabled and we would also have to
>> have another place where to track the enablement of tests/optimization,
>> in our CI.  It is possible, but after some tries I think your approach
>> is better and the main thing is that we need something to be pushed in
>> order for us not to get into failing CI fatigue which I've seen in other
>> projects where CI failures are just being dismissed after couple of
>> issues.  So:
>>
>> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
>
>Thanks. I think it's important to note that this affects systems with
>old CLang and I don't expect many developers there (in fact none).
>Except for our CI which can be easily fixed - just pass -O0 in CFLAGS.
>

Which is another thing that needs adjustment.  I think if you adjust it
right now just for macos-12 and then update the CI files in the same
commit then we'll get a green pipeline *with* no tests skipped because
of clang for a change ;)

>Michal
>