Skip to content

CMake refactor - #443

Open
scott-degraw wants to merge 5 commits into
rat-pac:mainfrom
scott-degraw:cmake-refactor
Open

scott-degraw wants to merge 5 commits into
rat-pac:mainfrom
scott-degraw:cmake-refactor

Conversation

@scott-degraw

Copy link
Copy Markdown
Contributor

CMake refactor

This is a fairly significant refactor of the CMake build system.

Target based

The main improvement is using modern target based usage requirements instead of the hardcoded "make style" commands from before. Most of the core dependencies are now packaged into the ratpac_common target. Each module's CMakeLists.txt goes from

target_include_directories(ratbase SYSTEM PUBLIC 
        $<BUILD_INTERFACE:${RATPAC_INCLUDE_DIR}/stlplus>)
target_include_directories(ratbase PUBLIC
        $<BUILD_INTERFACE:${RATPAC_INCLUDE_DIR}>
        $<INSTALL_INTERFACE:include>)

to

target_link_libraries(ratbase PUBLIC ratpac_common)

which includes all include, linking, preprocessor definitions (not properly implemented before) and more to the target. Optional module dependencies can now be more easily included e.g.

target_link_libraries(mimir PUBLIC ratpac_common Ratpac::NLopt)

The new cmake/RatpacDependencies.cmake finds all the dependencies to define the new ratpac_common and Ratpac::... targets.

Configure time copies

The configure time copy of all the module include files to a single include directory has been removed and each module objects library gets the include directories through ratpac_common. This removes a bug where modifying a header file and building without reconfiguring would not include the header file changes. The install target still copies all the headers to a single include when installing but this is done using install functions instead of the configure time copy. The install command now also means that the downstream Ratpac target is a proper target with target dependencies instead of hardcoded strings.

The generated config header files are now configured straight into the binary directory. This means the awkward .gitignore entries (e.g. src/core/config.hh) are removed. ratpac_common has the generated header files properly included.

libRATEvent

I'm not sure if this shared library was ever used very much (is it just genie?) but it was causing build issues for me on my mac. The issue was that it was never linked properly and had undefined symbols due to incomplete linking declarations. You can see this by running ldd -r libRATEvent.so. Linux's linker ignored this waiting until runtime whereas macOS's linker refused to accept this at link time. The workaround was to just link it with the same targets as RATPAC in which it just becomes byte-by-byte identical to libRATPAC. If this is meant to be a lightweight version of the RATPAC library, some source changes will need to be made so that it can be linked against a subset of ratpac.

CMake version bump

Min version bump from 3.11 to 3.16

@JamesJieranShen JamesJieranShen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @scott-degraw , for your hard work as always! I did this myself when I was working on SNO+ RAT's cmake build chain, and obviously have been hit by the bad compile_command reference resolution a couple times myself, so it is good to see this fixed in ratpac2. Some comments on the specific places certain lines are put, mostly to make things a bit more maintainable. As mentioned in the review, it would be good to see some relevant changes being propagated to the experiment template and Eos.

I've tested this myself against the latest version of EosSimulations, and all works well.

Comment thread CMakeLists.txt
Comment thread CMakeLists.txt Outdated
Comment thread src/CMakeLists.txt Outdated
Comment thread src/CMakeLists.txt Outdated
Comment thread src/CMakeLists.txt
target_include_directories(RATEvent PUBLIC ${RATPAC_INCLUDE_DIR})
target_link_libraries(RATEvent PUBLIC
${Geant4_LIBRARIES} ${ROOT_LIBRARIES} RATDict stlplus)
### library for ROOT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RATEvent was recently deprecated by me in favor of linking the entire libRATPAC with ROOT. I'm just now realizing that this was actually always broken, but kind of working (until someone called a symbol they are not supposed to). Personally, I'm in favor of completely removing it -- nothing should need to use it anymore, and this double-build is completely unnecessary. but we should come to a decision in a RAT call.

Comment thread cmake/RatpacDependencies.cmake Outdated
Comment thread src/CMakeLists.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants