Springclean CMake scripts and developer/client build/use interfaces #13
Conversation
|
@drbenmorgan Can you check that #8 (reproducible build) is still valid? |
|
Good spot! I've fixed a mistake that was exporting a build tree path to the installed |
So I generally prefer to distribute these static libraries as PIC since the general idea behind static vs. shared libraries are static libraries copy the symbols by value whereas shared libraries copy the symbols by reference. Thus, its my understanding that if one is building |
The argument to have non-PIC the default is that it's just that - generally users/packagers etc (and CMake itself) expect/default to non-PIC statics. CMake defaults, as does Autotools, to build non-PIC statics, but anticipates the use case of building them PIC by offering the Looking around different Linux distro packaging guidelines, some require non-PIC statics, others PIC, but the general expectation seems to be that statics are default built without PIC, or at least provide a documented/easy way to change this. One could provide an additional CMake option in PTL to disable PIC, but given CMake provides a good and clear one already, it just feels simpler/cleaner/less surprising to use that along with the default non-PIC case, i.e. |
|
@drbenmorgan Sorry for the delay, I will get this merged before the end of this next week. |
|
@jrmadsen The remaining failure on clang-12 looks to be down to the change to not export nominally developer-only flags like sanitisers. I 8000 'm not totally sure why the other builds are fine with similar configurations though. Will try and reproduce locally. |
|
Ok @drbenmorgan the only way to fix it is to do: if(PTL_USE_SANITIZER AND NOT ("${PTL_SANITIZER_TYPE}" STREQUAL "leak"))
string(REPLACE " " ";" _sanitize_args "${ptl_sanitize_args}")
set_target_properties(ptl-shared PROPERTIES
INTERFACE_LINK_OPTIONS "${_sanitize_args}")
endif()This is effectively just ensuring that |
|
@drbenmorgan LMK if these changes work for you |
|
Thanks for the updates @jmadsen, and apologies this totally slipped my mind. I'll try these out ASAP next week and post back here. |
|
@drbenmorgan I am just going to merge this |
0ec3baf to
bed35ba
Compare
- Use version range for minimum CMake version Follow recommendation in: https://cliutils.gitlab.io/modern-cmake/chapters/basics.html to support current 3.8 minimum whilst always setting policy settings to those of the running version. - Don't set policies which will automatically be NEW - Let CMake fully handle versions without caching Caching allows user to set version values, but these are not user configurable values. Let CMake handle the setting of PTL_... and PROJECT_... version variables through the primary call to project() after version info has been read from VERSION file.
- Use modern CMake style of lowercase commands and empty else(),
end...() commands
- Remove all unused custom functions and macros
- Refactor remaining functionality into dedicated modules
- Remove obsolete/duplicate options and those that only apply to
examples
- Only report relevant enabled features
- Not possible to reliably report flags as these can change over
the build tree
- Do not cache PTL_INSTALL_... variables, but allow override when
PTL is a subproject, otherwise defaulting to the CMAKE_INSTALL_...
equivalents
- Only expose ptl_add_option options when PTL is the primary project
- parent projects can still set these manually
- Use compile features to export CXX Standard
- Add cxx_std_<VERSION> to all targets built through ptl_build_library
to ensure this usage requirement is propagated to clients.
- Remove CMAKE_CXX_EXTENSIONS and CMAKE_CXX_STANDARD_REQUIRED from the
cache as these should not be set by builders of the project.
- Remove CMAKE_CXX_STANDARD setting in config files as the targets
propagate this requirement.
- Do not default CMAKE_INSTALL_RPATH_USE_LINK_PATH setting
This variable should be left up to the builder/installer as it
hardcodes RPATH information that may not be wanted.
- Create header to propgate public preproc symbols
PTL can be built with support for TBB and locks in user queues.
These are enabled in code via #ifdef blocks on preprocessor symbols
passed through -D flags. However, the choice is hardcoded at build
time as it affects the API and ABI of the resultant PTL libraries.
Promote TBB/Lock symbols to configured values in a new Config.hh file
generated by CMake. Include this file in PTL code depending on the
symbols. Install it along side other PTL headers.
Remove setting of -D flags for TBB and locks from both public and
private compile definitions.
- Update FindTBB module using version from VTK/Ogre, adding FPHSA support
- Link directly to Threads and TBB targets for simplicity and clarity
- Rationalize CMake/Build settings
- Prefer use of CMAKE_.._FLAGS for passing sanitizer/coverage flags
down the build tree. Whilst use of targets/usage requirements is
preferred in modern CMake, it may not be the simplest solution for
things like compiler flags that are pure BUILD_INTERFACE. Propagating
flags through targets may result in these appearing in the install
interface for the project. This may be confusing, or at worst, lead
to unexpected behaviour.
NB: Once CMake >= 3.13 is required, add_{compile,link}_options can
be used instead.
- Unify build/install CMake config files
- Handle primary target difference between build/install trees
transparently via the PTLTargets.cmake file
- Only use component arguments to find_package to decide on default
for static/shared linking. Use of BUILD_{STATIC/SHARED}_LIBS may
not result in the behaviour intended by the client project.
- Generate and install pkg-config file for PTL
Comment out GPU example as it cannot currently build either with updated CMake scripts or those on master
Set path to custom FindTBB module using a @PACKAGE_...@ variable to that it will not export build tree information to the install tree.
6c3ec17 to
9df5f13
Compare
Whilst this may look a significant change from the diffs, it's primarily a clean and tidy of the CMake scripts and use for both developers and clients of PTL.
else()/endif()/end...()commands.target_compile_featuresto propagate the C++ standard to clients of PTLCMAKE_POSITION_INDEPENDENT_CODEvariablePTL/Config.hhfile to#define/#undefthePTL_USE_LOCKSandPTL_USE_TBBsymbols. Since these determine (AFAICT) the API/ABI of PTL, they've got to be propagated to clients and this saves having to transport-Dflags around.Threads::ThreadsandTBB::tbbtargets are now linked directly to PTL rather than an intermediaryINTERFACEtarget given the simplicity of the link required.CMAKE_..._FLAGSinstead of viaINTERFACEtargets. Given that these flags aren't propagated to the usage requirements of an installed PTL, setting via theFLAGSvariables is o.k (and even suggested by CMake and used in VTK among others). This also results in less code and prevents the need to export and setup what become essentially empty interface targets in the install (since the flags only apply to theBUILD_INTERFACE), or to deal with potentially complex genexes (to only link in theBUILD_INTERFACE).add_subdirectoryGEANT4_USE_TBBoption, so we don't want to expose thePTL_USE_TBBequivalent, just set it on the basis of the Geant4 one).PTLConfig.cmake.intemplate for the build and install treesPTLTargets.cmakefiles that's generated for each.pkg-configfile for TBBEven with all of that, there are a couple of things missing:
masteror here, so I'm not sure of its state or which CUDA/setup is needed.PTL_ALLOC_EXPORTset as a-Dflag globally at build time (so it is set when compiling examples)LIB_BUILD_DLLinsource/PTL/Types.hhPTLConfig.cmakefile sets_PTL_ARCHIVEas an interface compile definition ofPTL::ptlbut this symbol isn't present in the code anywhere.Anyway, there's way more than enough already in this PR...