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

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(a)redhat.com&gt; >> --- >> >> 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(a)redhat.com&gt; 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