Description of problem: New version of check (indirectly) added required parameter `msg` to `fail_if` macro. Even though it is not clear from macro definition. sh$ grep fail_if /usr/include/check.h #define fail_if(expr, ...)\ I read it as `expr` is required and we might have some optional parameters if needed. But simple code fails without msg Version-Release number of selected component (if applicable): check-0.15.0-1.fc33.x86_64 How reproducible: Deterministic Steps to Reproduce: 1. dnf install -y gcc check-devel 2. cat > test.c <<EOF #include <check.h> int main(void) { fail_if(1); } EOF 3. gcc -Wall -Wextra test.c Actual results: In file included from test.c:1: test.c: In function ‘main’: test.c:5:5: error: too few arguments to function ‘_ck_assert_failed’ 5 | fail_if(1); | ^~~~~~~ /usr/include/check.h:502:27: note: declared here 502 | CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line, | ^~~~~~~~~~~~~~~~~ Expected results: no failures. Additional info:
looks like related to downstream patch https://src.fedoraproject.org/rpms/check/blob/5f9827a06524eedf512bd988a76a1c232adc1f23/f/check-0.15.0-attribute-format.patch
Sorry about that. Upstream tried to add format arg checking but did it incorrectly. That was my attempt at fixing the issue. I've dropped the patch for now, but that means you get the upstream bug back. Expect lots and lots of warning messages from the format arg checker. I'll try to work out a way to fix the upstream bug without breaking fail_if.
(In reply to Jerry James from comment #2) > Sorry about that. Upstream tried to add format arg checking but did it > incorrectly. That was my attempt at fixing the issue. I've dropped the > patch for now, but that means you get the upstream bug back. Expect lots > and lots of warning messages from the format arg checker. > > I'll try to work out a way to fix the upstream bug without breaking fail_if. I am not against proper fix but it should be firstly fixed in the upstream.
Still reproducible even with newer version in rawhide [build@aec39e3ac883 ~]$ rpm -q check-devel check-devel-0.15.1-1.fc33.x86_64 [build@aec39e3ac883 ~]$ gcc -Wall -Wextra test.c In file included from test.c:1: test.c: In function ‘main’: test.c:4:5: error: too few arguments to function ‘_ck_assert_failed’ 4 | fail_if(1); | ^~~~~~~ /usr/include/check.h:502:27: note: declared here 502 | CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line, |
This breaks SSSD tests. Is there any ETA on the fix?
Lukas submitted patches to make SSSD compatible with the changes, so it no longer blocks us.
I have this bug with libqb too.
Affects proftpd too. I have a patch for proftpd that I'll be sending upstream later today. I see that the fail_* macros are shown as "deprecated" in the check documentation anyway.
I have applied upstream's fix and rebuilt in Rawhide. Please try your builds again. Also, it is true that the fail* macros are deprecated. The ck_assert* macros should be used instead.
(In reply to Jerry James from comment #9) > I have applied upstream's fix and rebuilt in Rawhide. Please try your > builds again. > It allows missing message but does not allow message at all. even though macro still allow them in the definition ``` #define fail_if(expr, ...)\ ``` sh$ cat >test.c <<EOF #include <check.h> int main(void) { fail_if(1); fail_if(1, "message"); fail_if(1, "message with format string: %d", 42); return 0; } EOF sh$ gcc -Wall -Wextra -Werror test.c -lcheck In file included from test.c:1: test.c: In function ‘main’: test.c:6:16: error: too many arguments for format [-Werror=format-extra-args] 6 | fail_if(1, "message"); | ^~~~~~~~~ test.c:7:16: error: too many arguments for format [-Werror=format-extra-args] 7 | fail_if(1, "message with format string: %d", 42); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors
It is more obvious after compiling source file after pre-processing sh$ gcc -E -P -Wall -Wextra -Werror test.c -o test2.c sh$ gcc -Wall -Wextra -Werror test2.c -lcheck test2.c: In function ‘main’: test2.c:847:71: error: too many arguments for format [-Werror=format-extra-args] 847 | (1) ? _ck_assert_failed("test.c", 6, "Failure '""1""' occurred" , "message", ((void *)0)) : _mark_point("test.c", 6); | ^~~~~~~~~ test2.c:848:71: error: too many arguments for format [-Werror=format-extra-args] 848 | (1) ? _ck_assert_failed("test.c", 7, "Failure '""1""' occurred" , "message with format string: %d", 42, ((void *)0)) : _mark_point("test.c", 7); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors
Looks like there is not an easy way for all possible options which could be used without any problem with check <= 0.15.0 ``` fail_if(1); fail_if(1, "message"); fail_if(1, "message with format string: %d", 42); ``` IMHO there is a possible option. Add required *msg* parameter to fail_if (yes backward incompatible change. but it will allow to use format string checking which is very useful #define fail_if(expr, msg ...)\ And allow fallback to old behavior if some macro is defined before check.h that would require either new copy od function _ck_assert_failed which does not require format string checking or disable it for _ck_assert_failed with ifdef
(In reply to Paul Howarth from comment #8) > I see that the fail_* macros are shown as "deprecated" in the check > documentation anyway. Sure they are deprecated but still widely used and useful in some situation. and IMHO it would be betler to drop deprecated macros rather than change behavior.
(In reply to Lukas Slebodnik from comment #10) > It allows missing message but does not allow message at all. > even though macro still allow them in the definition Sure it does. The compiler issues a warning, but the warning isn't fatal unless ... > sh$ gcc -Wall -Wextra -Werror test.c -lcheck ... you add -Werror to your build flags. Try using the ck_assert* macros instead of the deprecated fail* macros if you want a warning-free build.
(In reply to Lukas Slebodnik from comment #12) > Looks like there is not an easy way for all possible options > which could be used without any problem with check <= 0.15.0 > > ``` > fail_if(1); > fail_if(1, "message"); > fail_if(1, "message with format string: %d", 42); > ``` > > IMHO there is a possible option. > > Add required *msg* parameter to fail_if (yes backward incompatible change. > but it will allow to use format string checking which is very useful > > #define fail_if(expr, msg ...)\ > > And allow fallback to old behavior if some macro is defined before check.h > that would require either new copy od function _ck_assert_failed which does > not require > format string checking or disable it for _ck_assert_failed with ifdef Do you have a patch to do this? If so, let's show it to upstream and see what they think of it.
(In reply to Jerry James from comment #14) > (In reply to Lukas Slebodnik from comment #10) > > It allows missing message but does not allow message at all. > > even though macro still allow them in the definition > > Sure it does. The compiler issues a warning, but the warning isn't fatal > unless ... > > > sh$ gcc -Wall -Wextra -Werror test.c -lcheck > > ... you add -Werror to your build flags. It emits "-Wformat-extra-args". Werror is intentional for preventing unnecessary warnings. Otherwise you needn't notice them; especially if you rely on CI ehn compiling on many platforms. Sure we could Wformat-extra-args but that would be disabled for everything and not just for unit tests. IIUC, the goal of libcheck was to addt format string checking but it does not work for following cases. (and they are most usefull cases) fail_if(1, "message"); fail_if(1, "message with format string: %d", 42); Using specialized ck_asser__* macros do not have varargs and thus do not benefit a lot from format string checking. > Try using the ck_assert* macros > instead of the deprecated fail* macros if you want a warning-free build. sh$ git grep -E "fail_(if|unless)" | wc -l 1948 We can image a better way of spending time rather then changing all usage in older tests.
This bug appears to have been reported against 'rawhide' during the Fedora 33 development cycle. Changing version to 33.
Check release 0.15.2 is supposed to fix this: https://github.com/libcheck/check/releases/tag/0.15.2