Skip to content

added initial cmakelists.txt file. #17

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.

Sign up for GitHub

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

Open
madmongo1 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added initial cmakelists.txt file. #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
cmake_minimum_required(VERSION 3.8)
cmake_policy(VERSION 3.8)
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, the cmake_minimum_required already defaults to the same policy version.


###############################################
# NOTE FOR THOSE UNFORTUNATES WHO DON'T KNOW OF HUNTER
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a lot of overkill for just getting Boost. Not to mention that "those unfortunates" is a rather condescending tone to take. I get the impression from this text that you just like hunter a lot and as a result it's become your hammer for which every problem needs to be a nail.

Could you explain why find_package(Boost REQUIRED ${MIN_VERSION}) isn't enough? That approach will work with a system-installed boost, which I doubt this Hunter thing does as from the little info I've gleaned about it it's basically it's own package manager.

#
# Hunter is a dependency manager for CMake. It automatically downloads, configures and compiles
# libraries that your program depends on.
# Hunter is clever enough to understand the toolchain file you have specified on the command line, and
# build dependencies (including quite demanding ones like boost, openssl etc) with the correct command line arguments.
# Tt then caches the build by target, toolchain and configuration so that you never have to build that
# configuration again.
# see https://github.com/ruslo/hunter
#
# If you don't want to use Hunter then simply define the variable HUNTER_ENABLED=OFF or USE_HUNTER_FOR_DEPENDENCIES=OFF
# on the CMAKE command line
if (DEFINED HUNTER_ENABLED)
option(USE_HUNTER_FOR_DEPENDENCIES "Use the hunter package manager to find boost" ${HUNTER_ENABLED})
else ()
option(USE_HUNTER_FOR_DEPENDENCIES "Use the hunter package manager to find boost" ON)
endif ()

option(IMPL_PTR_BUILD_TESTS "build the tests" ON)

if (USE_HUNTER_FOR_DEPENDENCIES)
include(cmake/HunterGate.cmake)
HunterGate(
URL "https://github.com/ruslo/hunter/archive/v0.19.96.tar.gz"
SHA1 "8316d0491ee03a918d2e0973aaf5878d9bcfe472"
)
endif ()
#
# at this point, HUNTER_ENABLED is guaranteed to be set to ON or OFF
#

project(ImplPtr)
set(CMAKE_CXX_STANDARD 14)
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is wrong: right now C++11 can be used for building
  2. This fixes it to a specific version instead of requiring a minimal version. Try target_compile_features(impl_ptr INTERFACE cxx_std_11) instead (or better: the list of C++11 features we use).


if (HUNTER_ENABLED)
hunter_add_package(Sugar)
include(${SUGAR_ROOT}/cmake/Sugar)
include(sugar_files)
include(sugar_include)
hunter_add_package(Boost)
find_package(Boost CONFIG REQUIRED)
else ()
find_package(Boost REQUIRED)
endif ()


add_library(impl_ptr INTERFACE)
target_include_directories(impl_ptr SYSTEM INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no generated sources, so adding the binary dir is not necessary.

$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>)
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're adding a BUILD_INTERFACE, why not an INSTALL_INTERFACE?

Copy link
Author

@madmongo1 madmongo1 Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Haven't got round to the that bit yet. The first commit was simply to get this building and running tests on any platform without having to install any dependencies.

target_link_libraries(impl_ptr INTERFACE Boost::boost)


if (IMPL_PTR_BUILD_TESTS)
if (HUNTER_ENABLED)
sugar_include(test)
else ()
find_package(Boost REQUIRED)
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you repeat the package detection that you've already done above?

Copy link
Author

@madmongo1 madmongo1 Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a harmless oversight. Repeating a dependency in cmake has no ill effects.

Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but it is a reason to not merge this PR in. "It works" isn't good enough, it needs to be understandable too.

add_subdirectory(test)
endif ()

add_executable(impl_ptr_tests ${TEST_SOURCES})
Copy link
Collaborator

@muggenhor muggenhor Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erm? Why aren't you creating this target directly in the test dir?

Copy link
Author

@madmongo1 madmongo1 Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one way. Easy to do if that's where you want it.

target_link_libraries(impl_ptr_tests PRIVATE impl_ptr)
endif ()
Loading