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

build/ops: CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC #7474

Merged
merged 1 commit into from Feb 5, 2016

Conversation

jack-changtao
Copy link

For CMake version <=2.8.11 ,it has no PUBLIC and PRIVATE keywords. It should use LINK_PRIVATE and LINK_PUBLIC instread to compile

Signed-off-by: changtao changtao@hihuron.com

@jack-changtao jack-changtao changed the title CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC … CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC Feb 2, 2016
@tchaikov
Copy link
Contributor

tchaikov commented Feb 2, 2016

see https://cmake.org/cmake/help/v2.8.11/cmake.html#command:target_link_libraries , https://cmake.org/cmake/help/v2.8.12/cmake.html#command:target_link_libraries. the PUBLIC and PRIVATE modes are introduced in 2.8.12.

and el7 comes with 2.8.11. while ubuntu 14.04 (trusty) has 2.8.12.

@tchaikov
Copy link
Contributor

tchaikov commented Feb 2, 2016

@cbodley and @alimaredia what do you think?

@cbodley
Copy link
Contributor

cbodley commented Feb 2, 2016

@tchaikov thanks for the links. i also looked at the docs for v3.5 (https://cmake.org/cmake/help/v3.5/command/target_link_libraries.html), and see that they've kept legacy support for LINK_PUBLIC/PRIVATE without saying anything about deprecating them in the future. so i'm fine with using them instead.

the hard part will be getting everyone to use them going forward, without a buildbot that runs cmake 2.8.11. because PUBLIC is the default behavior for target_link_libraries, I suggest that we remove the 2 instances of LINK_PUBLIC in this patch. then we'll just hope that when someone needs to use PRIVATE, they'll copy-paste from one of the examples of LINK_PRIVATE. and maybe add a comment to the first use of LINK_PRIVATE to point out that it's needed for 2.8.11

@jack-changtao
Copy link
Author

@tchaikov @cbodley

maybe, it is proper to use two variables to define it .
If some one will need to use PRIVATE or PUBLIC to going forward, they will copy-paste from one of the examples using ${PRIVATE} or ${PUBLIC}

if((CMAKE_MAJOR_VERSION GREATER 2) AND (CMAKE_MINOR_VERSION GREATER 8) AND (CMAKE_PATCH_VERSION GREATER 11))
set(PUBLIC PUBLIC)
set(PRIVATE PRIVATE)
else ((CMAKE_MAJOR_VERSION GREATER 2) AND (CMAKE_MINOR_VERSION GREATER 8) AND (CMAKE_PATCH_VERSION GREATER 11))
set(PUBLIC LINK_PUBLIC)
set(PRIVATE LINK_PRIVATE)
endif((CMAKE_MAJOR_VERSION GREATER 2) AND (CMAKE_MINOR_VERSION GREATER 8) AND (CMAKE_PATCH_VERSION GREATER 11))

@tchaikov
Copy link
Contributor

tchaikov commented Feb 3, 2016

agree with @cbodley , @jack-changtao please see the inlined comment. it might be simpler.

… of PRIVATE,PUBLIC for backward compatibility

Signed-off-by: Tao Chang <changtao@hihuron.com>
@jack-changtao
Copy link
Author

@tchaikov @cbodley

have modified as commented,Thanks !

@mattbenjamin
Copy link
Contributor

This is needed ASAP, can we merge this?

tchaikov added a commit that referenced this pull request Feb 5, 2016
CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC

Reviewed-by: Kefu Chai <kchai@redhat.com>
@tchaikov tchaikov merged commit cb2fc91 into ceph:master Feb 5, 2016
@ghost ghost changed the title CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC build/ops: CMake: For CMake version <= 2.8.11, use LINK_PRIVATE and LINK_PUBLIC Mar 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants