Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: fix to find snprintf #10155

Closed
wants to merge 2 commits into from
Closed

cmake: fix to find snprintf #10155

wants to merge 2 commits into from

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Dec 24, 2022

I haven't had the time to check other configurations, but on my macOS Ventura 13.1 with XCode 14.2 cmake does not find snprintf.

curl continues to build and work just fine, however since HAVE_SNPRINTF variable is cached, other dependencies (for example json-c) skip the check due to the way check_symbol_exists and assume that snprintf is not defined.

Solution: ensure stdio.h is checked for definitions

This way other dependencies have a fair shot at checking whether this function is available without being presented with a false negative.

I haven't had the time to check other configurations, but on my macOS Ventura 13.1 with XCode 14.2
cmake does not find `snprintf`.

curl continues to build and work just fine, however since HAVE_SNPRINTF variable is cached, other dependencies (for example [json-c](https://github.com/json-c/json-c)) skip the check due to the way `check_symbol_exists` and assume that `snprintf` is not defined.

Solution: ensure stdio.h is checked for definitions

This way other dependencies have a fair shot at checking whether this function is available without being presented with a false negative.
@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

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

@vszakats
Copy link
Member

vszakats commented Dec 24, 2022

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.

@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

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:

curl/lib/mprintf.c

Lines 972 to 975 in 1c567f7

#ifdef HAVE_SNPRINTF
(snprintf)(work, sizeof(work), formatbuf, p->data.dnum);
#else
(sprintf)(work, formatbuf, p->data.dnum);

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?

@bagder
Copy link
Member

bagder commented Dec 24, 2022

  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

@bagder
Copy link
Member

bagder commented Dec 24, 2022

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.

@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

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.

@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

  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).

@vszakats
Copy link
Member

vszakats commented Dec 24, 2022

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

@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

@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

@vszakats
Copy link
Member

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.

Solution: move stdio.h reference to the check that uses it
@yrashk
Copy link
Contributor Author

yrashk commented Dec 24, 2022

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

@bagder bagder added the cmake label Dec 24, 2022
@vszakats
Copy link
Member

vszakats commented Dec 24, 2022

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

@bagder bagder changed the title Problem: cmake does not find snprintf cmake: fix to find snprintf Dec 26, 2022
bagder
bagder approved these changes Dec 26, 2022
@bagder bagder closed this in 5ee81c3 Dec 26, 2022
@bagder
Copy link
Member

bagder commented Dec 26, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants