cmake: hiding symbols & unit tests

[jzakrzewski]

This PR brings symbol hiding to CMake build. It also disables building unit tests by default if the symbols are hidden. The appropriate targets will however be created and it will be possible to try to build them manually (even though it should fail).

The checks are mostly inspired by the autotools version but I can only understand these scripts partially, so hopefully I didn't screw this up too much.

Tested with VS 2015 and GCC on Gentoo.

[mention-bot]

@jzakrzewski, thanks for your PR! By analyzing the annotation information on this pull request, we identified @snikulov, @billhoffman and @Sukender to be potential reviewers

[jzakrzewski]

Forgot to mention: this should address concerns from #981

[Lekensteyn]

This adds the flags to every target, but autotools does this only for libcurl. What about adding this in lib/CMakeLists.txt:

target_compile_options(${LIB_NAME} PRIVATE ${_CFLAG_SYMBOLS_HIDE})

or (since it applies to the directory only):

add_compile_options(${_CFLAGS_SYMBOLS_HIDE})

[jzakrzewski]

@Lekensteyn I'll take a look at it again then. Unfortunately, reading autotools scripts is definitely not my strength.

[jzakrzewski]

So if I understood it correctly, there are two libraries where we apply the visibility flag and the define: libcurl itself and libhostname. Correct me if I'm wrong.

i have used set_property(TARGET ... APPEND PROPERTY ...) because there was something fishy going on with set_target_properties for definitions and the other proposed functions are not present in target CMake version (we're still on 2.8.0 unfortunately)

[Lekensteyn]

Is the version limitation something in your environment or does it happen because it is the current minimum version? Since the current version is far from complete, I think there would be a case to bump the minimum version to 3.x.

Your changes look good to me by the way.

[jzakrzewski]

Well, in my opinion CMake is so easy to build on each platform it supports that we should pick much newer version but last time I proposed it, it met a veto from @bagder because version 3 hasn't been available in most repositories. I'd gladly bump it to at the very least 2.8.12 and anything newer wuold be even better but we have to collectively agree on it.

This time I have simply tested the changes with the minimum required version as stated in cmake_minimum_required and found out it didn't work as expected.

[Lekensteyn]

(Sorry, this turned out to be a version comparison rather than a new comment to your good patch.)

Requiring 2.8.0 (released 2009-11) is a bit silly. 2.8.7 (2012-01) would be a good start (shipped with Ubuntu 14.04, available in Travis' trusty environment). Even RHEL 6 ships with 2.8.12 (latest version of 2.8.x). target_compile_options and add_compile_options were added in 2.8.12 (see below).

See https://wiki.wireshark.org/Development/Support_library_version_tracking#CMake for the versions we use for the Wireshark project.

The support of cmake (next to autotools) in Wireshark was however already mature for some time, but not so much with curl. (It results in a warning that the system is not well-maintained.) Given this and given that a working autotools setup exist, I think we are in the position to bump the cmake minimum version requirements a bit if it helps.

Actually, after some versions comparison (see below) and given the support by distros (Ubuntu 14.04, RHEL6), I think that 2.8.12 is a pretty good version to target for without sacrificing too much compatibility.

Overview of cmake versions

For the cmake 2.8.x series there is also a commands compatibility matrix. Below are versions with their release date and announce mails (containing changelogs). CMake 2.8.x.y versions can be compared to CMake 3.x.y (y are patch levels with no new features).

I have tried to summarize these changes briefly and focused on the benefits for the cmake files, ignoring things like cmake-gui (one exception is the mention of ninja). Some changes could be missed here as I based them on the changelogs.

(supported by Ubuntu 12.04 (package info))
2.8.7 (2011-12, announce mail).

2.8.8 (2012-04, announce mail) adds the Ninja generator on Linux (speed!), new way to allow others to find "your package" (e.g. the curl library) without writing find_package file. Adds an "OBJECT" library type (can for example be used to build objects once and link them for (lib)curl and a testing program without rebuilding the object file twice).

(Supported by Debian Wheezy (package info))
2.8.9, (2012-09, announce mail) adds POSITION_INDEPENDENT_CODE property and support for ninja on Windows/OS X.

2.8.10 (2012-10, announce mail) supports MSVC2012, new generator expressions, generator expressions in INCLUDE_DIRECTORIES and COMPILE_DEFINITIONS

(Supported by Debian wheezy-backports (package info))
2.8.11 (2013-05, announce mail) enables targets to specify their include directories and macro definitions to their consumers (allows for example to add macros to curl tests which enables some function without duplicating the macro addition). Added target_include_directories and target_compile_definitions.

(Supported by RHEL/CentOS 6/7 (errata 2014:1506 for 6, errata 2016:2175 for 7), SLES 12 (first version with cmake, package info), Ubuntu 14.04 (package info))
2.8.12 (2013-10, announce mail) adds target_compile_options and add_compile_options. New COMPILE_OPTIONS target property (a list instead of the string options in COMPILE_FLAGS). target_link_libraries gained the INTERFACE. New file(GENERATE) option.

(Supported by Debian Jessie (package info))
3.0.0 (2014-06, announce mail) does not seem to have a lot of changes other than the number and removal of old cmake syntax.
Edit: actually, the project command gained a new VERSION option.

3.1.0 (2014-12, announce mail) allows for specifying the C standard, specification of generator expressions in the SOURCES target property (like add_library(name $<$<BOOL:${HAVE_X}>:$<TARGET_OBJECTS:y>>)). Furthermore it no longer implicitly dereferences variables in keywords (policy CMP0054). New target_sources command to append to the SOURCES property. cmake -E env option was added.

3.2.1 (2015-03, announce mail) allows BYPRODUCTS for add_custom_command and add_custom_target (useful for automatically generated files, don't know if curl has a lot of them though).

3.3.0 (2015-07, announce mail) introduces the COMPILE_LANGUAGE generator. if also gained an IN_LIST operator.

3.4.0 (2015-11, announce mail) does not seem to have too many interesting new features. Maybe the ability to find static OpenSSL libraries and some small Windows changes.

(Supported by Ubuntu 16.04 (package info), Ubuntu 14.04 as cmake3)
3.5.0 (2016-03, announce mail) seems to have even less interesting changes.

(Supported by Debian Stretch, EPEL6)
3.6.0 (2016-07, announce mail) has some fancy changes, but probably not relevant for curl.

[bagder]

For the record. As long as we have skilled and knowledgeable cmake hackers around, I trust your decisions and general level of clues in this area much better than my own so I leave the question on which a sensible lowest supported version in your capable hands.

[jzakrzewski]

OK then.
As said before - for me the newer, the better. Building it from scratch is like 3-5 commands, binary downloads are available for windows, linux and mac and it works without installation.
That being said, I can agree that having the right version in repository is a huge +, so I hold nothing against 2.8.12. I think that since the CMake support is relatively new, there's nothing wrong in choosing relatively up-to-date version. One can always fall back to autotools if needed.

[Lekensteyn]

Hmm, 2.8.12 is apparently not included in wheezy-backports (it has 2.8.11.2). So if RHEL7 and Debian Wheezy need to be supported, it is better to set 2.8.11 as minimum version. Otherwise 2.8.12 should still be an excellent target. (I have updated the list above with additional links and SLES).

This change looks ok to me otherwise. Let's merge it? If we decide to bump cmake to 2.8.11 (or 2.8.12) later, we could adjust the existing code accordingly.

[jzakrzewski]

+1 for merging as-is.
Q: now or after Wednesday's bugfix release? After all it is kinda bugfix.

[bagder]

I view it as a bugfix, so no objections from me to merge asap.

[jzakrzewski]

I can do it in the evening when I'm home. Or somebody can take it over.
I think it should be edited to two commits.

[Lekensteyn]

Would this also fix #981? Ah yes, it does fix the issue according to your earlier comment. (Might be worth noting in the commit message.)

I also found a useful link on the mailing list, mentioning it here in case you have not seen it before:
https://github.com/curl/curl/wiki/push-access-guidelines

[jzakrzewski]

Thanks for the link. I knew I saw it somewhere...
Anyway, hopefully I did it right.