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

Fix warnings when with master project set to hidden #719

Merged
merged 1 commit into from
Apr 27, 2018

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Apr 25, 2018

Setting the OLD version of this policy (or even if it is unset and CMake minimum_version is set below 3.3, which it is) does the wrong thing; it causes GCC to spew a mass of warnings if you try to use fmt from a project that has set hidden symbols. Setting this to NEW fixes the issue.

Patch to 4.x because both policy statements are missing from master.

@henryiii
Copy link
Contributor Author

henryiii commented Apr 25, 2018

PS:

CMake's own documentation says the following:

While setting policies individually is supported, we encourage projects to set policies based on CMake versions

In CMake 3.12 (not yet released), the following syntax is supported:

cmake_minimum_required(VERSION 3.1...3.12)

Which indicates you support 3.1+ but have tested it with 3.12 as well. This syntax, due to a trick, does not confuse older CMakes, though it does not set the policies.

A much better long term solution then would be the following:

cmake_minimum_required(VERSION 2.8.12...3.11)

if(${CMAKE_VERSION} VERSION_LESS 3.12)
    cmake_policy(VERSION ${CMAKE_VERSION})
endif()

(The if block would not need to be changed, and does not need an else).

This should be done after CMake 3.12 comes out, to make sure no policies come out that break fmt. Correction: it could be done now; when 3.12 comes out, the if block will no longer be true, but CMake 3.12 will understand the minimum required statement and will set the 3.11 policies!

@vitaut vitaut merged commit b6ac63f into fmtlib:4.x Apr 27, 2018
@vitaut
Copy link
Contributor

vitaut commented Apr 27, 2018

Merged, thanks!

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

Successfully merging this pull request may close these issues.

2 participants