I am also trying to patch CMake to solve the underlying issue I ran into: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8042

I deleted that check in 4d73854, because it wasn't used in the curl codebase.

It seems unexpected to check for features we do not use locally (for a CMake novice at least).

Could this be solved by fixing our snprintf() detection? Or to disable caching this to avoid side-effects in other projects?

If we end up checking for HAVE_STDIO_H, IMO a comment telling the reason would be helpful.

Of course, the more significant underlying issue is how CMake's check_symbol_exists caches defined variables regardless of which include files were checked. This is what I am trying to address with my merge requested I mentioned above. Even if it makes in some capacity, it'll be a long time before most of users will have that version of CMake installed.

I am not sure what you mean by "check for features we do not use locally"

Here's one use of HAVE_SNPRINTF here:

The surface is indeed tiny, but HAVE_SNPRINTF being optional, it would just revert to sprintf on platforms where snprintf.h can only be found in stdio.h. snprintf's man page does indicate that it should come from stdio.h (and so does POSIX standard). Not sure why curl would not check stdio.h for snprintf based on this.

I wonder if in fact curl never/rarely detects snprintf as being available? I just spun up a bare Ubuntu docker image and tried to build curl on it, and here's what I got:

-- Looking for snprintf
-- Looking for snprintf - not found

This suggests that curl may be not using snprintf at all right now.

Not sure about the implications of having HAVE_STDIO_H defined you had in mind. Can you elaborate?

1. it should not check for the function in an include file, it should check if it is present when linking.
2. I may have reintroduced the snprintf() use again (in 935b1bd) after @vszakats removed that check

This suggests that curl may be not using snprintf at all right now.

Sure it does, if you use configure or another build system that sets that define.

This suggests that curl may be not using snprintf at all right now.

Sure it does, if you use configure or another build system that sets that define.

Fair, I meant in the context of using CMake. Apologies for the lack of clarity on this.

1. it should not check for the function in an include file, it should check if it is present when linking.

This PR does not change the way the presence of this function is checked, it simply gives snprintf a chance to be found while employing the same method (check_symbol_exists).

@yrashk what I meant is that the patch restores HAVE_STDIO_H. That macro isn't used anywhere in curl. I understand (maybe wrongly) that if we're checking for snprintf, we'd define HAVE_SNPRINTF, which is the macro guarding its use after @bagder's patch.

@vszakats there's a check for snprintf:

https://github.com/curl/curl/blob/master/CMakeLists.txt#L1095

But it is failing because stdio.h is not included

Got it. Still looks odd, and maintaining my opinion that a comment documenting this peculiarity would be useful. This could prevent accidentally deleting this check in the future because of no local macro references. If there is a more self-documenting way to force-include stdio.h to make the snprintf test pass, it'd probably be a better solution. It also makes me wonder how many other necessary checks may I have deleted that was there for a similar hidden side-effect.

@vszakats got it! I've updated the PR to clarify which check requires stdio.h. Do you think this is any better?

@yrashk Thank you for your update, it clears up all concerns from my end indeed.

Thanks!