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

Adding CMake visibility policy setting #1215

Merged
merged 1 commit into from Aug 18, 2017
Merged

Conversation

henryiii
Copy link
Contributor

This policy setting will silence a warning when using with a visibility settings on targets. Due to the forced cmake_minimum_version, policy settings in CMakeLists calling this one (including the main CMakeLists) are lost, forcing the change to be made here.

This policy setting will silence a warning when using with a visibility settings on targets. Due to the forced `cmake_minimum_version`, policy settings in CMakeLists calling this one (including the main CMakeLists) are lost, forcing the change to be made here.
@gennadiycivil
Copy link
Contributor

Could you please provide testing output before and after

@henryiii
Copy link
Contributor Author

henryiii commented Aug 18, 2017

Sure, this is with GitHub.com/GooFit/GooFit, using this patch for after and master for before. v1.8.0 also produces CMP0048 warnings, but those have been fixed in master.

After:

henrys-mbp:build henryiii$ cmake .
-- Submodule update
CUDA_TOOLKIT_ROOT_DIR not found or specified
-- Could NOT find CUDA (missing: CUDA_TOOLKIT_ROOT_DIR CUDA_NVCC_EXECUTABLE CUDA_INCLUDE_DIRS CUDA_CUDART_LIBRARY) (Required is at least version "7.0")
-- Auto device selection: CPP
-- CMake version: 3.9.0
-- Version: 4.0.1
-- Build type: RelWithDebInfo
-- Compiler supports IPO: NO
-- pybind11 v2.1.1
-- Found Python at /System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib, building bindings
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/henryiii/git/fitting/goofit/build

Before:

(Corrected, this was an old master before, and still had the 0048 warnings which were fixed previously)

henrys-mbp:build henryiii$ cmake .
-- Submodule update
CUDA_TOOLKIT_ROOT_DIR not found or specified
-- Could NOT find CUDA (missing: CUDA_TOOLKIT_ROOT_DIR CUDA_NVCC_EXECUTABLE CUDA_INCLUDE_DIRS CUDA_CUDART_LIBRARY) (Required is at least version "7.0")
-- Auto device selection: CPP
-- CMake version: 3.9.0
-- Version: 4.0.1
-- Build type: RelWithDebInfo
-- Compiler supports IPO: NO
-- pybind11 v2.1.1
-- Found Python at /System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib, building bindings
-- Configuring done
CMake Warning (dev) at extern/googletest/googletest/cmake/internal_utils.cmake:153 (add_library):
  Policy CMP0063 is not set: Honor visibility properties for all target
  types.  Run "cmake --help-policy CMP0063" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  Target "gmock" of type "STATIC_LIBRARY" has the following visibility
  properties set for CXX:

    CXX_VISIBILITY_PRESET

  For compatibility CMake is not honoring them for this target.
Call Stack (most recent call first):
  extern/googletest/googletest/cmake/internal_utils.cmake:176 (cxx_library_with_type)
  extern/googletest/googlemock/CMakeLists.txt:89 (cxx_library)
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at extern/googletest/googletest/cmake/internal_utils.cmake:153 (add_library):
  Policy CMP0063 is not set: Honor visibility properties for all target
  types.  Run "cmake --help-policy CMP0063" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  Target "gmock_main" of type "STATIC_LIBRARY" has the following visibility
  properties set for CXX:

    CXX_VISIBILITY_PRESET

  For compatibility CMake is not honoring them for this target.
Call Stack (most recent call first):
  extern/googletest/googletest/cmake/internal_utils.cmake:176 (cxx_library_with_type)
  extern/googletest/googlemock/CMakeLists.txt:94 (cxx_library)
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at extern/googletest/googletest/cmake/internal_utils.cmake:153 (add_library):
  Policy CMP0063 is not set: Honor visibility properties for all target
  types.  Run "cmake --help-policy CMP0063" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  Target "gtest_main" of type "STATIC_LIBRARY" has the following visibility
  properties set for CXX:

    CXX_VISIBILITY_PRESET

  For compatibility CMake is not honoring them for this target.
Call Stack (most recent call first):
  extern/googletest/googletest/cmake/internal_utils.cmake:176 (cxx_library_with_type)
  extern/googletest/googletest/CMakeLists.txt:96 (cxx_library)
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at extern/googletest/googletest/cmake/internal_utils.cmake:153 (add_library):
  Policy CMP0063 is not set: Honor visibility properties for all target
  types.  Run "cmake --help-policy CMP0063" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  Target "gtest" of type "STATIC_LIBRARY" has the following visibility
  properties set for CXX:

    CXX_VISIBILITY_PRESET

  For compatibility CMake is not honoring them for this target.
Call Stack (most recent call first):
  extern/googletest/googletest/cmake/internal_utils.cmake:176 (cxx_library_with_type)
  extern/googletest/googletest/CMakeLists.txt:95 (cxx_library)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Generating done
-- Build files have been written to: /Users/henryiii/git/fitting/goofit/build

@gennadiycivil
Copy link
Contributor

Looks good, just one more - how will this behave with old version of cmake?

@henryiii
Copy link
Contributor Author

henryiii commented Aug 18, 2017

It ignores it, that's why it is surrounded by the if statement. See https://cmake.org/cmake/help/v3.3/manual/cmake-policies.7.html#manual:cmake-policies(7)

The behavior of CMP0063 is described here: https://cmake.org/cmake/help/v3.3/policy/CMP0063.html . Setting this to OLD will still remove the warning, but will ignore the visibility settings in GoogleTest if statically linked on all CMake versions (current behavior). Set to ON will cause GoogleTest to keep it's requested visibility settings even if statically linked on CMake 3.3+. It's used in GoogleTest's code here:

if (gtest_hide_internal_symbols)
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)
endif()

Let me know if you'd like anything else. I really just care about the warnings, so OLD is fine, but it almost assuredly not ideal, and OLD may stop having an effect in upcoming versions of CMake. The same sort of decision was made to set CMP0048 to NEW.

@henryiii
Copy link
Contributor Author

PS: I believe the Travis CI testing is with the default Trusty CMake 2.8.

@gennadiycivil
Copy link
Contributor

LGTM

@gennadiycivil gennadiycivil merged commit 780bae0 into google:master Aug 18, 2017
@henryiii henryiii deleted the patch-1 branch August 19, 2017 11:32
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.

None yet

2 participants