Skip to content
Snippets Groups Projects

Modernisation of CMake scripting to simplify build of FullSimLight

Merged Benjamin Morgan requested to merge bmorgan/GeoModel:modernize-cmake-fsl into master
All threads resolved!

With Athena now using Geant4 static/big libraries, optimization work has shifted to look at use of LTO/PGO. As these are relatively involved tasks, a first step is to use FullSimLight as a proof-of-principle for the flags and operations needed to support this. Though this work won't (at least at this stage) be integrated into GeoModel/FullSimLight, the need to hack at the build scripts has highlighted some modernizations/simplifications that can be made independently right now. These are not only useful to assist the LTO/PGO work, but for general efficiency and clarity of the build.

  • Bump CMake minimum to 3.16, tested up to 3.26. Could be reduced a little, but 3.16 or higher is now pretty common across all platforms (see Repology listing).
    • Allows removal of some workarounds needed with older versions
    • If FullSimLight also gets used to test use of Celeritas/AdePT GPU simulation libraries for ATLAS, a more modern CMake will be needed for better CUDA support.
  • Consistently create namespaces ALIAS targets in subprojects. Only link to namespaced GeoModel targets in subprojects
    • Avoids need to check whether a project is built in a full GeoModel build or standalone.
  • Make ExternalProject targets for JSON, Eigen, and Xerces-C dependencies of the internal imported targets
    • Avoids need to use add_dependencies on GeoModel targets that link to these. They simply link to the namespaced imported target, e.g. nlohmann_json::nlohmann_json and don't need to worry if this is coming from the builtin on the system.
  • Fully implement the FullSimLight target as an INTERFACE target so plugins just need to link to it.
  • Build the core of the FullSimLight application as an OBJECT library.
    • Applications just link to this instead of recompiling the sources each time for their own use.
  • General simplifications:
    • Keep all build commands for a target as close together as possible for clarity
    • Check subproject status using CMAKE_CURRENT_SOURCE_DIR instead of an artificial project command.
    • Remove dead code or no longer required/supported modules (e.g. Geant4_USE_FILE)

These have been tested locally on macOS with as many combinations of build options as I can enable and appears fine. CI is more than likely to highlight issues, but let's see what it throws up.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Resolved by Joseph Boudreau

      Once again, thanks for this modernization, @bmorgan , which is for sure very useful.

      However, I must say that now I recall why we still sticked to workaround solutions for earlier CMake versions: the main reason was the "official" version on Lxplus, which is also the one shipped with CC7 by default.

      So, I see two ways here:

      1. We keep the current setup, which works on earlier CMake versions too, and we postpone the modernization to when CERN officially will move to Lxplus8, which provides CMake 3.20.2
      2. We update our setup with your MR, but we have to warn people using the default Lxplus to either: a) set up a bare ATLAS environment with --cmakeversion option of asetup; b) connect to Lxplus8 instead of the default Lxplus, which still points to Lxplus7

      I would suggest @boudreau and @tsulaia should decide what to do.

      Best, -- Ric.

      Edited by Riccardo Maria Bianchi
  • Benjamin Morgan added 36 commits

    added 36 commits

    • db5e6fd6...d877c836 - 23 commits from branch GeoModelDev:master
    • f304c3c8 - Bump minimum CMake for core packages to 3.16
    • 3cd8f410 - Rationalize top level CMake script
    • b17e3d87 - Simplify use of COLOR_DEFS output
    • 4b9cb47e - Rationalize FullSimLight cmake script
    • 3d098064 - Simplify build/use of builtin/system JSON
    • 26d26704 - Simplify build/use of builtin/system XercesC
    • 1cc28ca0 - Simplify build/use of builtin/system Eigen
    • cd4f5c34 - Always link to namespaced CMake targets
    • 6dd1186f - Implement FullSimLight alias/exported target
    • f23434b2 - Streamline build of FullSimLight
    • 1b7c973a - Simplify build of GeoModelG4
    • 6da896ee - Simplify FullSimLight CMake script
    • 7602726d - Only set color strings if COLOR_DEFS is true

    Compare with previous version

  • Rebased to resolve conflicts, but let's see what CI throws up as the conflicts in the FullSimLight were a little knarly...

  • O.k., seems to be an issue with the FullSimLightConfig.cmake, will check locally as this was working before so likely I messed something up in the rebase.

  • I've rerun the CI pipeline and now appears green without any changes being needed, so likely the last failure was a glitch. So, I think this is ready for final review!

  • Sorry to bump this, but is there anything else I need to do so this can be merged? It would be useful to have this in master for the next step addressing @akraszna's suggestion to use FetchContent, and also to begin the testing of GPU offload with AdePT and Celeritas!

  • Joseph Boudreau resolved all threads

    resolved all threads

  • Hi @bmorgan , this MR looks fine on my side. I'd ask @rbianchi , @boudreau , @tsulaia a final confirmation. Thanks, Marilena

    Edited by Marilena Bandieramonte
  • Joseph Boudreau mentioned in commit 96396bed

    mentioned in commit 96396bed

  • Please register or sign in to reply
    Loading