ext/imap/config.m4: -Werror=implicit-function-declaration compatibility. #10948
Conversation
ext/imap/config.m4
Outdated
| $TST_LIBS | ||
| ], [ | ||
| #include <c-client.h> | ||
| char utf8_to_mutf7_php(){ return utf8_to_mutf7(""); } |
There was a problem hiding this comment.
Nice trick but now it complains about type-casting because the function return pointer
/mnt $ cat /usr/include/imap/utf8aux.h |grep utf8_to_mutf7
unsigned char *utf8_to_mutf7 (unsigned char *src);
There was a problem hiding this comment.
checking whether utf8_to_mutf7 function present... no
configure:46303: checking whether utf8_to_mutf7 function present
configure:46326: clang -o conftest -O2 -fomit-frame-pointer -g -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fvisibility=hidden -Os -fomit-frame-pointer -D_GNU_SOURCE -I/usr/include/imap -Wl,--as-needed,-O1,--sort-common -Wl,-rpath,/lib -L/lib conftest.c
-lc-client -lcrypt -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err -lssl -lcrypto
-lrt -lm -lacl -lxml2 -lpcre2-8 -lz >&5
conftest.c:327:48: warning: passing 'char[1]' to parameter of type 'unsigned char *' converts between pointers to integer types where one is of the unique plain 'char' type and the other is not [-Wpointer-sign]
char utf8_to_mutf7_php(){ return utf8_to_mutf7(""); }
^~
/usr/include/imap/utf8aux.h:43:46: note: passing argument to parameter 'src' here
unsigned char *utf8_to_mutf7 (unsigned char *src);
^
conftest.c:327:34: error: incompatible pointer to integer conversion returning 'unsigned char *' from a function with result type 'char' [-Wint-conversion]
char utf8_to_mutf7_php(){ return utf8_to_mutf7(""); }
^~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
There was a problem hiding this comment.
Argh, and it's using the wrong type because PHP_TEST_BUILD declares every function as type char. I'll have to use one of the bare autoconf macros, it looks like.
There was a problem hiding this comment.
For the same reason it fails for utf8_mime2text
configure:45093: checking for utf8_mime2text signature
configure:45116: clang -c -I/usr/include/imap -Os -fomit-frame-pointer -D_GNU_SOURCE conftest.c >&5
conftest.c:329:32: error: too few arguments to function call, expected 3, have 2
utf8_mime2text(src, dst);
~~~~~~~~~~~~~~ ^
/usr/include/imap/utf8aux.h:37:6: note: 'utf8_mime2text' declared here
long utf8_mime2text (SIZEDTEXT *src,SIZEDTEXT *dst,long flags);
^
1 error generated.
There was a problem hiding this comment.
I think the mime2text thing is intended? It's trying to figure out of the signature is the old one with two args or the new one with three.
|
Somehow tests now fails because it cannot detect the function anymore, not sure if this is legit or not, but maybe only the SKIPIF section of the relevant tests need to be amended |
Well, the test certainly needs a SKIPIF (it has none), since the feature is optional... but that doesn't explain why the function is no longer detected. Is there a way to get |
I'm on Fedora now so it's a pain to get ext/imap to compile since CClient is not part of the RPMs any more. And I'm not aware of a good way to get the config.log from CI :/ @iluuu1994 maybe knows? |
|
The latest force-push refactors to use a cached check, and I could try switching it to a compile test, as opposed to a link test, to see if that helps. But regardless of the result, I would not want to move forward without an explanation of why the test is failing on the CI machine. |
|
@orlitzky You could probably just - if: always()
run: cat config.log |
|
Brilliant, thanks. Let's see if I did that right... |
|
Switching away from The Of course, the only reason I switched away from it in the first place is because those signatures are wrong, although that was probably just for convenience. On the one hand, I'm now relatively confident that a compile test (as opposed to a link test) would be fine here. But on the other, this has made me curious why things work on my Gentoo system but not the Ubuntu CI. I'll think about it some more tomorrow and decide what to do. |
The recent clang-16 throws errors for implicitly defined functions by
default. In many ./configure tests, an undefined function (which is
"implicitly defined" when you try to call it) is undefined because it
really does not exist. But in one case, utf8_to_mutf7() is undefined
because we forgot to include the header that defines it.
This commit updates the test for utf8_to_mutf7:
* We now include the header (c-client.h) that defines it.
* A "checking... yes/no" message was added to the test.
* The test was switched from PHP_IMAP_TEST_BUILD to AC_COMPILE_IFELSE.
This was the easiest way to avoid a return-type mismatch that runs
afoul of -Werror=implicit-int.
* CPPFLAGS is temporarily amended with the -I flag needed to find
c-client.h.
Closes GH-10947.
The PHP_IMAP_TEST_BUILD macro defines a bunch of public no-op functions for apparently no reason before delegating to PHP_TEST_BUILD. Well, there is a reason, and now that I've rediscovered it, this commit adds a comment explaining it.
|
It should all be sorted now. First, c-client is in bad shape these days. If my count is correct, the last release was 16 years ago and we have 22 patches for it in Gentoo (14 of those come from Debian). It has two users that I know of, PHP and Asterisk, so it might be worthwhile to fork it. Right now it's being kept alive by distro patches, but we're duplicating a lot of work that could be avoided by having a canonical upstream repo. PHP is also working extra hard to detect all of the weird ways that the various distros might patch and package it. But back to this PR: the c-client patch monster that everyone is shipping was never actually designed to be a shared library. The I didn't notice it because (1) I have That satisfies my curiosity, and makes me confident that a compile test (not a link test) would be fine here. That way, the undefined symbols don't matter. (PHP does define those callbacks in the IMAP extension, so the real link phase works fine.) I've switched the test to |
@iluuu1994 Should we add that as a step to all CI by default? It would automatically be collapsed anyway. Alternatively it could be uploaded as an artifact. |
|
Is there a way to prevent this warning? |
I've not needed this output until now, but seems harmless. |
ext/imap/config.m4
Outdated
| AC_LANG_PUSH(C) | ||
| AC_CACHE_CHECK(for utf8_to_mutf7, ac_cv_utf8_to_mutf7, | ||
| AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <c-client.h>]],[[ | ||
| utf8_to_mutf7(""); |
There was a problem hiding this comment.
Please use unsigned char *c; utf8_to_mutf7(c);
There was a problem hiding this comment.
Or maybe unsigned char c; utf8_to_mutf7(&c); is better
There was a problem hiding this comment.
Or maybe
unsigned char c; utf8_to_mutf7(&c);is better
Yep, done. Thanks.
There was a problem hiding this comment.
Thank you, used as patch and it works for 8.2 and master
The existing test for utf8_to_mutf7() passes the empty string (of type char*) to it, but the function expects an unsigned char pointer. This raises a warning with -Wpointer-sign. We fix it by passing in an argument of the correct type.
|
It passed on all arches https://gitlab.alpinelinux.org/andypost/aports/-/pipelines/157950 |
Yeah, I think we really need to start considering moving ext/imap to PECL due to the whole c-client issue. But thank you for taking care of this! |
The recent clang-16 throws errors for implicitly defined functions by default. In many ./configure tests, an undefined function (which is "implicitly defined" when you try to call it) is undefined because it really does not exist. But in one case, utf8_to_mutf7() is undefined because we forgot to include the header that defines it.
This commit updates the test for utf8_to_mutf7:
Closes GH-10947.