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: expand CURL_USE_PKGCONFIG to non-cross MINGW #14658

Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Aug 23, 2024

Enable CURL_USE_PKGCONFIG by default for more environments:

  • for MINGW targets when not using cross-compilation.
  • stop restricting vcpkg to MSVC. (this currently unlocks mingw,
    also unlocked by the update above.)

Also:

  • cache CURL_USE_PKGCONFIG in CURLConfig.cmake.
    Suggested-by: Kai Pastor

Follow-up to c555ab4 #14575


@vszakats vszakats added cmake feature-window A merge of this requires an open feature window Windows Windows-specific labels Aug 23, 2024
@github-actions github-actions bot added the build label Aug 23, 2024
@vszakats vszakats force-pushed the cm-pkg-config-expand-to-mingw-nox branch from 239e90d to 0236f8e Compare August 23, 2024 08:55
@Andarwinux
Copy link

Why? Cross-compiling or not, pkg-config is always much better than cmake modules for MinGW. Most cmake modules don't pass private dependencies and cflags at all, which makes them completely useless for static builds.

@vszakats vszakats force-pushed the cm-pkg-config-expand-to-mingw-nox branch from 0236f8e to 7d03667 Compare August 23, 2024 16:46
@vszakats
Copy link
Member Author

vszakats commented Aug 23, 2024

pkg-config picks up any native non-cross component by default, breaking builds that were not broken before. pkg-config has no concept of cross-building (I've seen one implementation that does, though that seems like an outlier).

It's a matter of enabling it with CURL_USE_PKGCONFIG=ON in case pkg-config is configured for cross-builds.

@vszakats vszakats force-pushed the cm-pkg-config-expand-to-mingw-nox branch from 7d03667 to caf612c Compare August 26, 2024 10:09
@vszakats
Copy link
Member Author

vszakats commented Aug 28, 2024

I just noticed that vcpkg also supports MINGW? In such case maybe the condition could be changed to allow pkg-config for any vcpkg compiler?:

-if(UNIX OR (MSVC AND VCPKG_TOOLCHAIN) OR (MINGW AND NOT CMAKE_CROSSCOMPILING))
+if(UNIX OR VCPKG_TOOLCHAIN OR (MINGW AND NOT CMAKE_CROSSCOMPILING))

(perhaps in a separate PR)

@dg0yt
Copy link
Contributor

dg0yt commented Aug 31, 2024

I just noticed that vcpkg also supports MINGW?

That's why I use it...


The CMake config must also use a cache variable, so that users can change it at time of use.

@vszakats
Copy link
Member Author

I just noticed that vcpkg also supports MINGW?

That's why I use it...

I guess that is something difficult to guess from a distance :)

In discussions/proposals, no one said a word about mingw support in vcpkg. This is now on track for the next release.

The CMake config must also use a cache variable, so that users can change it at time of use.

Would you mind posting a diff to illustrate what you mean?

@dg0yt
Copy link
Contributor

dg0yt commented Aug 31, 2024

The CMake config must also use a cache variable, so that users can change it at time of use.

I realize now that external input is respected in form of if(NOT DEFINED CURL_USE_PKGCONFIG).

Would you mind posting a diff to illustrate what you mean?

My suggestion is to use the same pattern as in CMakeLists.txt.

--- a/CMake/curl-config.cmake.in
+++ b/CMake/curl-config.cmake.in
@@ -23,13 +23,12 @@
 ###########################################################################
 @PACKAGE_INIT@
 
-if(NOT DEFINED CURL_USE_PKGCONFIG)
-  if(UNIX OR (MSVC AND VCPKG_TOOLCHAIN))  # Keep in sync with root CMakeLists.txt
-    set(CURL_USE_PKGCONFIG ON)
-  else()
-    set(CURL_USE_PKGCONFIG OFF)
-  endif()
+if(UNIX OR VCPKG_TOOLCHAIN OR (MINGW AND NOT CMAKE_CROSSCOMPILING))  # Keep in sync with root CMakeLists.txt
+  set(_curl_use_pkgconfig_default ON)
+else()
+  set(_curl_use_pkgconfig_default OFF)
 endif()
+option(CURL_USE_PKGCONFIG "Enable pkg-config to detect CURL dependencies" ${_curl_use_pkgconfig_default})
 
 include(CMakeFindDependencyMacro)
 if(@USE_OPENSSL@)

@vszakats
Copy link
Member Author

@dg0yt Thanks. If I read it right, option() is effectively adding CACHE to set(CURL_USE_PKGCONFIG ...), and keeping existing behavior other than that.

A nit I suggest changing is CURL to @PROJECT_NAME@.

Would you want to open a PR with this?

@vszakats vszakats closed this in e1ab01d Sep 20, 2024
@vszakats vszakats deleted the cm-pkg-config-expand-to-mingw-nox branch September 20, 2024 23:04
vszakats added a commit to vszakats/curl that referenced this pull request Sep 22, 2024
Enable `CURL_USE_PKGCONFIG` by default for MinGW cross-builds.

Note: This may cause fallouts in certain envs where pkg-config picks up
native packages.

Follow-up to e1ab01d curl#14658
Follow-up to c555ab4 curl#14575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build cmake feature-window A merge of this requires an open feature window Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

4 participants