search

wjakob / tbb

Intel TBB with CMake build system
Apache License 2.0
369 stars 161 forks source link

ENH: Add policy for newer version of cmake #34

Closed hjmjohnson closed 6 years ago

hjmjohnson commented 7 years ago

Set policies so that newer versions of cmake provide newer behavior and enhanced debugging or feature support.

wjakob commented 7 years ago

This looks to me like a generic boilerplate that one would stick into a new project. However, it's unclear to me how any of these policies apply to TBB specifically.

hjmjohnson commented 7 years ago

@wjakob These are boiler plate. They are taken from recommendations of other projects, primarily written by Brad King (the primary developer of cmake).

These are recommendations that have improved robustness of cmake across different platforms and development environments for other large projects (ITK, VTK, Slicer3d, BRIANSTools, vxl, etc...)

These are particularly important when attempting to bundle large applications for binary distribution (which I am trying to do). Off the top of my head, the following behaviors need to be well defined for making external package distributions (some only for mac bundles):

Hans

wjakob commented 7 years ago

This makes sense, but I still don't understand the relation to TBB specifically. Just to give one example, what is the purpose of

CMP0056  Honor link flags in try_compile() source-file signature.

We don't use special linker flags in try_compile() flags, and it is not clear why we would need them.

hjmjohnson commented 7 years ago

This is to pass along link flags when doing introspection. It is needed when testing the compiler for C++11 or C++14 feature sets under specified compiler flags.

As compiler feature sets change with each new version of the compiler, and each compiler flag specified, this feature has been one that I have found provides much better debugging. Otherwise the linkage environment for your code is different than the linkage environment used in the try_compiles (either your try_compiles, or ones used internal to cmake.

Honor link flags in try_compile() source-file signature.

The try_compile() command source-file signature generates a CMakeLists.txt file to build the source file into an executable. In order to compile the source the same way as it might be compiled by the calling project, the generated project sets the value of the CMAKE_<LANG>_FLAGS variable to that in the calling project. The value of the CMAKE_EXE_LINKER_FLAGS variable may be needed in some cases too, but CMake 3.1 and lower did not set it in the generated project. CMake 3.2 and above prefer to set it so that linker flags are honored as well as compiler flags. This policy provides compatibility with the pre-3.2 behavior.

The OLD behavior for this policy is to not set the value of the CMAKE_EXE_LINKER_FLAGS variable in the generated test project. The NEW behavior for this policy is to set the value of the CMAKE_EXE_LINKER_FLAGS variable in the test project to the same as it is in the calling project.

hjmjohnson commented 7 years ago

@wjakob I have been maintaining my own fork with these features that I must have in order to be able to bundle tbb in a manner similar to the other projects I work on (ITK, VTK, vxl, etc) . I was hoping that the efforts of those development teams could be synchronized with a common TBB fork so that TBB could more naturally be integrated with other packages.

If this is contrary to you goals with this repository, then please let me know so that I can make a different fork and accomplish the packaging needs that I have.

Thank you for you consideration.

hjmjohnson commented 7 years ago

@wjakob

set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -stdlib=libc++ -lc++abi")

The try_compile should be with respect to these compiler flags.

wjakob commented 7 years ago

Regarding your earlier message: I'm more than happy to integrate things that improve the state of the build system, but there needs to be a good case for why that is indeed the case. Most importantly, I want to understand what I am merging, and why it is needed. For this PR, your initial description (" Set policies so that newer versions of cmake provide newer behavior and enhanced debugging or feature support.") was IMHO a bit on the short/generic side, hence I was not convinced.

The linkage flag involving c++abi made sense to me. Let's go through the rest of them then:

CMP0048 ##NEW project() behavior for setting VERSION variable
I don't think I understand why this is useful.

CMP0025 # CMake 3.0 AppleClang vs. regular Clang
Ditto

CMP0042 # CMake 3.0 MACOSX_RPATH is enabled by default.
I am wondering if this really makes sense. We actually set the RPATH

if(APPLE)
  set(CMAKE_MACOSX_RPATH ON)
endif()

and need to to so for CMake 2.8 compatibility. So this seems redundant.

CMP0063 # CMake 3.3.2 Honor visibility properties for all target types
This one is also not clear to me.

It would be great if you could explain why the other policies need to be set.