CMake: CMake config files are defining CURL_STATICLIB for static builds by Adnn · Pull Request #2823 · curl/curl

Adnn · 2018-08-01T12:28:15Z

This change allows to use the CMake config files generated by Curl's
CMake scripts for static builds of the library.
The symbol CURL_STATIC lib must be defined to compile downstream,
thus the config package is the perfect place to do so.

Fixes #2817
Reported-by: adnn

This change allows to use the CMake config files generated by Curl's CMake scripts for static builds of the library. The symbol CURL_STATIC lib must be defined to compile downstream, thus the config package is the perfect place to do so. Fixes curl#2817 Reported-by: adnn

MarcelRaad · 2018-08-02T07:39:33Z

The Travis failures were caused by download failures, I've restarted the jobs.

This looks generally good to me, but shouldn't the condition for library users defining CURL_STATICLIB match the condition used by the library itself here? (I'm currently still trying to get familiar with the CMake build myself.)

Adnn · 2018-08-02T09:01:41Z

@MarcelRaad Thank you for your review and restarting the job (is the failed result below the first attempt, or did the restart also fail?)

I am not sure to understand what is your suggestion? Sadly, I am not familiar with this build system, but I think you are referring to an Automake file, and it seems unlikely that CMake share the same variables?

Anyway, the CMake variable allowing the user to request a static build is indeed CURL_STATICLIB, defined here. And it is already used by the CMake logic as the condition to determine if it is a static build (example here).

Do you have an alternative in mind?

MarcelRaad · 2018-08-02T09:14:07Z

The Travis failure is from the restarted build, but looks unrelated as it's from an autotools build.

Sorry, you're right, Makefile.am is not used by the CMake build (Makefile.inc is at least partially).

What I was looking for is where CURL_STATICLIB is added to the preprocessor definitions when building libcurl itself. And it seems the answer is "never" according to https://github.com/Adnn/curl/blob/633c055cd9e6afec1b3a19e3cb4f6fdcbdd24404/CMakeLists.txt#L1250? I'm totally consued now, sorry.

bagder · 2018-08-02T09:56:39Z

The failed travis build is the mysterious new mac fail due some weirdo SecureTransport thing that started showing up recently! =(

@nickzman can you figure out why this happens now on travis? It looks like it doesn't happen every time which is even worse...

jzakrzewski · 2018-08-02T11:12:00Z

@MarcelRaad The CURL_STATICLIB define sits here: https://github.com/Adnn/curl/blob/master/lib/curl_config.h.cmake#L80 . If the CURL_STATICLIB CMake option is set, this line https://github.com/Adnn/curl/blob/master/lib/CMakeLists.txt#L3 will bring it to life ;)

Adnn · 2018-08-02T11:20:27Z

@jzakrzewski Thank you for that explanation, TIL about #cmakedefine functionality ; )

nickzman · 2018-08-03T01:23:24Z

Well, that's bizarre. What's happening is SSLCopyALPNProtocols() and SSLSetALPNProtocols() are defined in the Secure Transport headers, but Apple did not actually implement the functions in the Security framework until macOS 10.13.4 (which is Darwin 7.5.0), and this particular computer is using a pre-.4 point release of macOS 10.13.

Those functions are behind __builtin_available(), so they won't be executed if the user is using an older OS than 10.13.4. But the SecureTransport.h header incorrectly specifies that the function became available in 10.13.0, so I think the linker is trying to hard-link to the function, and that would explain the undefined symbols error you are seeing.

Upgrading the OS on Travis' side would solve the problem. You could also solve it by using the -mmacosx-version-min compiler flag set to 10.12 or earlier to force it to weak-link the symbol. I would recommend you just upgrade the OS, since stock 10.13 had a catastrophic security hole you may have heard about, and there have been other security holes patched as well.

MarcelRaad

Note that I'm none of the CMake experts, but this looks good to me.

snikulov · 2018-08-08T08:03:39Z

As @jzakrzewski already mentioned, CURL_STATICLIB will be set through curl_config.h and not through compiler command line.

Latest master (ref #2755) slightly changed behavior to control build static/shared by using
cmake .. -DBUILD_SHARED_LIBS=OFF/ON

Resulting curl_config.h will be

╭─snikulov@snikulov-lin ~/work/github/curl/build.static  ‹master*›
╰─$ diff lib/curl_config.h ../build.shared/lib/curl_config.h
80c80
< #define CURL_STATICLIB 1
---
> /* #undef CURL_STATICLIB */
╭─snikulov@snikulov-lin ~/work/github/curl/build.static  ‹master*›

@Adnn should we close this or you have objections?

MarcelRaad · 2018-08-08T09:05:41Z

@snikulov But curl_config.h is not meant for inclusion by users of libcurl, or is it? If I understood correctly, INTERFACE_COMPILE_DEFINITIONS sets CURL_STATICLIB only for users of the library, not for building libcurl itself.

Adnn · 2018-08-08T09:16:10Z

@snikulov I am not sure to follow, do you propose to discard the PR?

@MarcelRaad You are right about the INTERFACE_xxx properties of CMake: they will be propagated to downstreams (consumers of the target), but will not define it for the target itself. So the point you are making seems to be the good one: if curl_config.h is not intended to be included by the clients, then it will of course not help in defining the required symbols for the client. In which case, the CMake package config remains a very good place to define it imo.

bagder · 2018-08-08T09:19:38Z

curl_config.h is strictly used when builting libcurl itself. The public headers are all in the include/curl/ directory.

snikulov · 2018-08-08T10:03:06Z

@Adnn @MarcelRaad now I see what you mean.
The imported target contains CURL_STATIC definition after this PR.
This definitely useful addition!

╰─$ diff -u /home/snikulov/work/github/curl/install.shared/lib/cmake/curl/libcurl-target.cmake /home/snikulov/work/github/curl/install.static/lib/cmake/curl/libcurl-target.cmake
--- /home/snikulov/work/github/curl/install.shared/lib/cmake/curl/libcurl-target.cmake  2018-08-08 12:49:56.000000000 +0300
+++ /home/snikulov/work/github/curl/install.static/lib/cmake/curl/libcurl-target.cmake  2018-08-08 12:52:04.000000000 +0300
@@ -51,9 +51,10 @@
 endif()

 # Create imported target CURL::libcurl
-add_library(CURL::libcurl SHARED IMPORTED)
+add_library(CURL::libcurl STATIC IMPORTED)

 set_target_properties(CURL::libcurl PROPERTIES
+  INTERFACE_COMPILE_DEFINITIONS "CURL_STATICLIB"
   INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
   INTERFACE_LINK_LIBRARIES "lber;ldap;dl;-lpthread;OpenSSL::SSL;OpenSSL::Crypto;/usr/lib64/libz.so"
 )

bagder · 2018-08-09T15:04:29Z

@snikulov should I take that as a 👍 from you on merging this?

snikulov · 2018-08-09T17:00:54Z

@bagder not exactly.
I have a proposal regarding the variable usage.
Since we agreed to use the standard BUILD_SHARED_LIBS option to control shared/static build with the previous PR (#2755), perhaps it is better to use these option in condition.
Awaits for @Adnn and @MarcelRaad comments or agreement.

snikulov

Use BUILD_SHARED_LIBS option in condition check

snikulov · 2018-08-08T14:19:11Z

  set_target_properties(${LIB_NAME} PROPERTIES STATIC_LIBRARY_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
 endif()

+if(CURL_STATICLIB)


@Adnn maybe change it to if(NOT BUILD_SHARED_LIBS) ?

Adnn · 2018-08-13T09:25:18Z

Hi,
@snikulov I can only agree that using standard CMake mechanisms wherever available is a great idea. So, going one step further than that, I would even test the actual TYPE property of the curl library target, because BUILD_SHARED_LIBS is a global setting that could potentially be overriden per-target.

If that sounds sensible, I'll update my PR

snikulov · 2018-08-13T10:54:07Z

@Adnn I see no cases where BUILD_SHARED_LIBS will be different with the TYPE property for libcurl target. IMO, value from option should be enough.

Adnn · 2018-08-13T11:15:05Z

@snikulov Thank you for your feedback. It should not be different, but it could be more future-proof. Any reason for avoiding TYPE though?

snikulov · 2018-08-13T11:26:16Z

@Adnn another reason - less code.

…e mechanism

snikulov · 2018-08-15T10:03:02Z

@bagder could you please merge it? Thank you.

bagder · 2018-08-15T11:46:30Z

Thanks!

bagder added the cmake label Aug 1, 2018

MarcelRaad approved these changes Aug 3, 2018

View reviewed changes

bagder mentioned this pull request Aug 3, 2018

Travis CI: Undefined symbols for architecture x86_64: "_SSLCopyALPNProtocols" on mac #2835

Closed

snikulov self-requested a review August 9, 2018 17:01

snikulov reviewed Aug 9, 2018

View reviewed changes

Changes the static vs. shared lib detection, to use the standard CMak…

a3aede0

…e mechanism

snikulov approved these changes Aug 14, 2018

View reviewed changes

bagder closed this in ab66a80 Aug 15, 2018

lock Bot locked as resolved and limited conversation to collaborators Nov 13, 2018

Uh oh!

Conversation

Adnn commented Aug 1, 2018

Uh oh!

MarcelRaad commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adnn commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcelRaad commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Aug 2, 2018

Uh oh!

jzakrzewski commented Aug 2, 2018

Uh oh!

Adnn commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickzman commented Aug 3, 2018

Uh oh!

MarcelRaad left a comment

Choose a reason for hiding this comment

Uh oh!

snikulov commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcelRaad commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adnn commented Aug 8, 2018

Uh oh!

bagder commented Aug 8, 2018

Uh oh!

snikulov commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Aug 9, 2018

Uh oh!

snikulov commented Aug 9, 2018

Uh oh!

snikulov left a comment

Choose a reason for hiding this comment

Uh oh!

snikulov Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

Adnn commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snikulov commented Aug 13, 2018

Uh oh!

Adnn commented Aug 13, 2018

Uh oh!

snikulov commented Aug 13, 2018

Uh oh!

snikulov commented Aug 15, 2018

Uh oh!

bagder commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

MarcelRaad commented Aug 2, 2018 •

edited

Loading

Adnn commented Aug 2, 2018 •

edited

Loading

MarcelRaad commented Aug 2, 2018 •

edited

Loading

Adnn commented Aug 2, 2018 •

edited

Loading

snikulov commented Aug 8, 2018 •

edited

Loading

MarcelRaad commented Aug 8, 2018 •

edited

Loading

snikulov commented Aug 8, 2018 •

edited

Loading

Adnn commented Aug 13, 2018 •

edited

Loading