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