jrmadsen / PTL

Parallel Tasking Library (PTL) - Lightweight C++11 mutilthreading tasking system featuring thread-pool, task-groups, and lock-free task queue
MIT License
43 stars 13 forks source link

Springclean CMake scripts and developer/client build/use interfaces #13

Closed drbenmorgan closed 3 years ago

drbenmorgan commented 3 years ago

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.

Even with all of that, there are a couple of things missing:

Anyway, there's way more than enough already in this PR...

jrmadsen commented 3 years ago

@drbenmorgan Can you check that #8 (reproducible build) is still valid?

drbenmorgan commented 3 years ago

Good spot! I've fixed a mistake that was exporting a build tree path to the installed PTLConfig.cmake file. I've checked that different build/install dirs result in identical installed versions of PTLConfig.cmake per the issue in #8.

jrmadsen commented 3 years ago

Don't force PIC for static libs as this might be unexpected and can be left up to the builder/packager through the CMAKE_POSITION_INDEPENDENT_CODE variable

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 libG4tasking.a as non-PIC then the PIC libptl.a symbols are copied into libG4tasking.a and effectively converted into non-PIC -- eliminating the runtime performance penalty.

drbenmorgan commented 3 years ago

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 libG4tasking.a as non-PIC then the PIC libptl.a symbols are copied into libG4tasking.a and effectively converted into non-PIC -- eliminating the runtime performance penalty.

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 CMAKE_POSITION_INDEPENDENT_CODE option (--with-pic in lib tool). If that's forced to ON in the scripts or the target property hard-coded, there's no way to change it.

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.

# default, builds ptl-static non-PIC
cmake <args>

# at user discretion, build ptl-static with PIC
cmake -DCMAKE_POSITION_INDEPENDENT_CODE=ON <args>
jrmadsen commented 3 years ago

@drbenmorgan Sorry for the delay, I will get this merged before the end of this next week.

drbenmorgan commented 3 years ago

@jrmadsen The remaining failure on clang-12 looks to be down to the change to not export nominally developer-only flags like sanitisers. I'm not totally sure why the other builds are fine with similar configurations though. Will try and reproduce locally.

jrmadsen commented 3 years ago

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 libtsan or libasan is linked into the final exe.

jrmadsen commented 3 years ago

@drbenmorgan LMK if these changes work for you

drbenmorgan commented 3 years ago

Thanks for the updates @jmadsen, and apologies this totally slipped my mind. I'll try these out ASAP next week and post back here.

jrmadsen commented 3 years ago

@drbenmorgan I am just going to merge this