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: more small tidy-ups and fixes #14450

Closed
wants to merge 14 commits into from
Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Aug 7, 2024

  • tidy up two MATCHES expression by avoiding macros expansion and
    adding quotes. Then convert then to STREQUAL to match other places
    in the code doing the same checks.

  • fix setting _ALL_SOURCE for AIX to match what autotools does.

  • delete stray _ALL_SOURCE reference from lib/config_riscos.h

  • simplify/fix two STREQUAL "" checks.
    The one in the openssl_check_symbol_exists() macro succeeded
    regardless of the value. The other could return TRUE when
    CMAKE_OSX_SYSROOT was undefined.

  • delete code for CMake versions (<3.7) we no longer support.

  • prefer LIST(APPEND ...) to extend CURL_LIBS.

  • use CURL_LIBS to add the network lib for Haiku.
    Before this patch it was done via raw C flags. I could not test this.

  • move _WIN32_WINNT-related code next to each other.
    It also moves detection to the top, allowing more code to use
    the result.

  • merge two WIN32 blocks.

  • rename internal variables to underscore + lowercase.

  • unwrap a line, indent, whitespace.

Closes #14450

- delete it from the macro. It seems to always check out TRUE.

- replace with simple boolean check which doesn't check out TRUE
  if the variable is undefined, unlike STREQUAL.

Closes #xxxxx
@vszakats vszakats added the cmake label Aug 7, 2024
@github-actions github-actions bot added the tests label Aug 7, 2024
@vszakats vszakats changed the title cmake: fixes cmake: assortment of fixes and tidy-ups Aug 8, 2024
@vszakats vszakats changed the title cmake: assortment of fixes and tidy-ups cmake: assortment of tidy-ups Aug 8, 2024
@vszakats vszakats changed the title cmake: assortment of tidy-ups cmake: more small tidy-ups Aug 8, 2024
@vszakats vszakats changed the title cmake: more small tidy-ups cmake: more small tidy-ups and fixes Aug 8, 2024
@vszakats vszakats closed this in 919394e Aug 8, 2024
@vszakats vszakats deleted the cm-strequal branch August 8, 2024 11:49
@vszakats
Copy link
Member Author

vszakats commented Aug 8, 2024

This odd AIX construct is also generated by autoreconf into lib/curl_config.h.in:

/* Define to 1 if OS is AIX. */
#ifndef _ALL_SOURCE
#  undef _ALL_SOURCE
#endif

What may be the purpose of this code?

Besides, autotools (like CMake after this PR) does define _ALL_SOURCE for AIX, which should work regardless of this code. What am I missing?

@dfandrich
Copy link
Contributor

dfandrich commented Aug 8, 2024 via email

@vszakats
Copy link
Member Author

vszakats commented Aug 8, 2024

Thanks Dan.

Do you know what purpose the #ifndef / #undef combo may have?
After staring at it for a while I cannot figure it out. It seems, just defining _ALL_SOURCE should be enough for AIX, or is it not?

@vszakats
Copy link
Member Author

vszakats commented Aug 8, 2024

Ah OK, I think I get it, autotools converts the odd construct to this, when defined:

/* Define to 1 if OS is AIX. */
#ifndef _ALL_SOURCE
#  define _ALL_SOURCE 1
#endif

I still cannot test it, but the CMake method of passing -D_ALL_SOURCE via the command-line would likely work without such trick. (Unless, AIX also defines it to a different value without checking if it's already set).

Probably making it -D_ALL_SOURCE=1 would be useful to sync it more with autotools.

vszakats added a commit to vszakats/curl that referenced this pull request Aug 8, 2024
@dfandrich
Copy link
Contributor

All the AIX references to _ALL_SOURCE are either #ifdef or defined() so it doesn't need to be set to a value like 1. AIX seems to set _ALL_SOURCE itself if none of _XOPEN_SOURCE, _POSIX_SOURCE or ANSI_C_SOURCE is already set.

You should get yourself a Compile Farm account to help with this kind of testing. They have two different AIX versions available as well as other obscure machines.

@vszakats
Copy link
Member Author

vszakats commented Aug 8, 2024

Thanks, they answer the questions and I've closed #14461 as not necessary.

Also for the AIX tip!

vszakats added a commit that referenced this pull request Aug 9, 2024
- prefix local variables with underscore and convert to lowercase.
- list variables accepted by `libcurl.pc` and `curl-config` templates.
- quote more string literals.

Follow-up to 919394e #14450
Closes #14462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants