Skip to content
This repository was archived by the owner on May 3, 2024. It is now read-only.

Conversation

@tomeichlersmith
Copy link
Member

@tomeichlersmith tomeichlersmith commented Sep 20, 2023

Instead of writing the dictionary file and then compiling it with the library, we can use a slightly newer method where we attach the dictionary to the already prepared target. This lets us use CMake's ability to change the include paths depending on which interface they are being accessed from (BUILD or INSTALL) and have the generated dictionary files updated accordingly.

In addition to being slightly newer, it allows us to avoid writing files into the source tree. This means users will be able to just rm -r build instead of having to remember to delete the generated headers in Framework/include/Framework if a new event object is added.

This change does have downstream effects; however, namely

  1. Need to update the cmake module to wrap the include paths in the BUILD_INTERFACE generator
  2. Need to prune any use of the EventDef.h header which is now removed

The patches to resolve these effects are copied below for reference.

Still To Do

  • Make sure this still works when the build and source directories are removed (production container image)
  • Only rewrite the LinkDef if need be, might need some custom target nonsense for that

cmake

Besides the necessary change below, we can also completely remove the build_event_bus and build_dict macros since they are not used anymore.

diff --git a/BuildMacros.cmake b/BuildMacros.cmake
index 10739aa..1150cda 100644
--- a/BuildMacros.cmake
+++ b/BuildMacros.cmake
@@ -263,7 +263,7 @@ function(register_event_object)
 
   if(NOT ${PROJECT_SOURCE_DIR}/include IN_LIST include_paths)
     set(include_paths
-        ${PROJECT_SOURCE_DIR}/include ${include_paths}
+        "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>" ${include_paths}
         CACHE INTERNAL "include_paths")
   endif()

The rest are just the busy-work of pruning the leftover references to the EventDef header.

Ecal
diff --git a/include/Ecal/EcalRecProducer.h b/include/Ecal/EcalRecProducer.h
index efd685f..a8a4fbf 100644
--- a/include/Ecal/EcalRecProducer.h
+++ b/include/Ecal/EcalRecProducer.h
@@ -19,7 +19,6 @@
 //----------//
 #include "DetDescr/DetectorID.h"
 #include "DetDescr/EcalID.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventProcessor.h"
 
 namespace ecal {
diff --git a/src/Ecal/EcalRecProducer.cxx b/src/Ecal/EcalRecProducer.cxx
index aeb5826..8133eb9 100644
--- a/src/Ecal/EcalRecProducer.cxx
+++ b/src/Ecal/EcalRecProducer.cxx
@@ -7,6 +7,9 @@
 #include "Ecal/EcalRecProducer.h"
 #include "DetDescr/EcalGeometry.h"
 #include "Ecal/EcalReconConditions.h"
+#include "Ecal/Event/EcalHit.h"
+#include "Recon/Event/HgcrocDigiCollection.h"
+#include "SimCore/Event/SimCalorimeterHit.h"
 
 namespace ecal {
Hcal
diff --git a/include/Hcal/HcalRecProducer.h b/include/Hcal/HcalRecProducer.h
index f3d335b..560f631 100644
--- a/include/Hcal/HcalRecProducer.h
+++ b/include/Hcal/HcalRecProducer.h
@@ -22,8 +22,8 @@
 #include "DetDescr/HcalDigiID.h"
 #include "DetDescr/HcalGeometry.h"
 #include "DetDescr/HcalID.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventProcessor.h"
+#include "Recon/Event/HgcrocDigiCollection.h"
 
 //---------//
 //  ROOT   //
diff --git a/src/Hcal/HcalDoubleEndRecProducer.cxx b/src/Hcal/HcalDoubleEndRecProducer.cxx
index c36bb37..d20ca07 100644
--- a/src/Hcal/HcalDoubleEndRecProducer.cxx
+++ b/src/Hcal/HcalDoubleEndRecProducer.cxx
@@ -3,10 +3,10 @@
 #include "DetDescr/HcalDigiID.h"
 #include "DetDescr/HcalGeometry.h"
 #include "DetDescr/HcalID.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventProcessor.h"
 #include "Hcal/HcalReconConditions.h"
 #include "Recon/Event/HgcrocDigiCollection.h"
+#include "Hcal/Event/HcalHit.h"
 
 namespace hcal {
 
diff --git a/src/Hcal/HcalRecProducer.cxx b/src/Hcal/HcalRecProducer.cxx
index c6f5551..4ac7728 100644
--- a/src/Hcal/HcalRecProducer.cxx
+++ b/src/Hcal/HcalRecProducer.cxx
@@ -8,6 +8,9 @@
 #include "Hcal/HcalRecProducer.h"
 
 #include "Hcal/HcalReconConditions.h"
+#include "Hcal/Event/HcalHit.h"
+#include "Recon/Event/HgcrocDigiCollection.h"
+#include "SimCore/Event/SimCalorimeterHit.h"
 
 namespace hcal {
 
diff --git a/src/Hcal/HcalSingleEndRecProducer.cxx b/src/Hcal/HcalSingleEndRecProducer.cxx
index 4321d84..c1ddf48 100644
--- a/src/Hcal/HcalSingleEndRecProducer.cxx
+++ b/src/Hcal/HcalSingleEndRecProducer.cxx
@@ -3,9 +3,9 @@
 #include "DetDescr/HcalDigiID.h"
 #include "DetDescr/HcalGeometry.h"
 #include "DetDescr/HcalID.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventProcessor.h"
 #include "Hcal/HcalReconConditions.h"
+#include "Hcal/Event/HcalHit.h"
 #include "Recon/Event/HgcrocDigiCollection.h"
 
 namespace hcal {
SimCore
diff --git a/include/SimCore/Simulator.h b/include/SimCore/Simulator.h
index 52de7ba..7a6b7d4 100644
--- a/include/SimCore/Simulator.h
+++ b/include/SimCore/Simulator.h
@@ -20,7 +20,6 @@
 /*   Framework   */
 /*~~~~~~~~~~~~~~~*/
 #include "Framework/Configure/Parameters.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventProcessor.h"
 #include "SimCore/ConditionsInterface.h"
 #include "SimCore/DetectorConstruction.h"
diff --git a/include/SimCore/SimulatorBase.h b/include/SimCore/SimulatorBase.h
index 87f0a6a..b104067 100644
--- a/include/SimCore/SimulatorBase.h
+++ b/include/SimCore/SimulatorBase.h
@@ -7,7 +7,6 @@
 #include <G4UIsession.hh>
 
 #include "Framework/Configure/Parameters.h"
-#include "Framework/EventDef.h"
 #include "Framework/EventFile.h"
 #include "Framework/EventHeader.h"
 #include "Framework/EventProcessor.h"

instead of writing the dictionary file and then compiling it with the
library, we can use a slightly newer method where we attach the
dictionary to the already prepared target. This lets us use CMake's
ability to change the include paths depending on which interface they
are being accessed from (BUILD or INSTALL) and have the generated
dictionary files updated accordingly

this change does have downstream effects; however, namely

1. Need to update the cmake module to wrap the include paths in the
BUILD_INTERFACE generator
2. Need to prune any use of the EventDef.h header which is now removed
@tomeichlersmith
Copy link
Member Author

Working in framework-testbench...

The current code is working well when issuing successive make calls.

After a clean build

We see that successive make calls simply re-consolidate links, only re-compile the necessary bits, and may re-compile the event if need be.

eichl008@framework-testbench:~/build$ make
Consolidate compiler generated dependencies of target Bench_Event
[  6%] Built target Bench_Event
Consolidate compiler generated dependencies of target Framework_Exception
[ 13%] Built target Framework_Exception
Consolidate compiler generated dependencies of target EventDict
[ 20%] Built target EventDict
Consolidate compiler generated dependencies of target Framework
[ 80%] Built target Framework
Consolidate compiler generated dependencies of target fire
[ 86%] Built target fire
Consolidate compiler generated dependencies of target Bench
[100%] Built target Bench
eichl008@framework-testbench:~/build$ touch ../Framework/src/Framework/Process.cxx 
eichl008@framework-testbench:~/build$ make
[  6%] Built target Bench_Event
[ 13%] Built target Framework_Exception
[ 20%] Built target EventDict
[ 23%] Building CXX object Framework/CMakeFiles/Framework.dir/src/Framework/Process.cxx.o
[ 26%] Linking CXX shared library libFramework.so
[ 80%] Built target Framework
[ 83%] Linking CXX executable fire
[ 86%] Built target fire
[ 90%] Linking CXX shared library libBench.so
[100%] Built target Bench
eichl008@framework-testbench:~/build$ touch ../Bench/src/Bench/Event/Hit.cxx 
eichl008@framework-testbench:~/build$ make
[  3%] Building CXX object BenchEvent/CMakeFiles/Bench_Event.dir/src/Bench/Event/Hit.cxx.o
[  6%] Linking CXX shared library libBench_Event.so
[  6%] Built target Bench_Event
[ 13%] Built target Framework_Exception
[ 20%] Built target EventDict
Consolidate compiler generated dependencies of target Framework
[ 23%] Linking CXX shared library libFramework.so
[ 80%] Built target Framework
[ 83%] Linking CXX executable fire
[ 86%] Built target fire
[ 90%] Linking CXX shared library libBench.so
[100%] Built target Bench

Issue: Calling cmake will re-write link def

This is a waste of time if the cmake call was just due to other things (like configuring our python modules or GDML detectors). It is not too much wasted time since the dictionary is just re-linked after a new linkdef is written, but it still is annoying.

eichl008@framework-testbench:~/build$ cmake ..
[ INFO ]: Building ROOT dictionary LinkDef
-- Configuring done
-- Generating done
-- Build files have been written to: /export/scratch/users/eichl008/ldmx/framework-testbench/build
eichl008@framework-testbench:~/build$ make
Consolidate compiler generated dependencies of target Bench_Event
[  6%] Built target Bench_Event
Consolidate compiler generated dependencies of target Framework_Exception
[ 13%] Built target Framework_Exception
[ 16%] Generating EventDict.cxx, libFramework_rdict.pcm, libFramework.rootmap
Scanning dependencies of target EventDict
Consolidate compiler generated dependencies of target EventDict
[ 20%] Building CXX object Framework/CMakeFiles/EventDict.dir/EventDict.cxx.o
[ 20%] Built target EventDict
Consolidate compiler generated dependencies of target Framework
[ 23%] Linking CXX shared library libFramework.so
[ 80%] Built target Framework
Consolidate compiler generated dependencies of target fire
[ 83%] Linking CXX executable fire
[ 86%] Built target fire
Consolidate compiler generated dependencies of target Bench
[ 90%] Linking CXX shared library libBench.so
[100%] Built target Bench

I'm trying to decide if this is a big enough issue, but I'll leave these notes here for now.

@tomeichlersmith
Copy link
Member Author

tomeichlersmith commented Oct 9, 2023

This cmake stuff still works in the production scenario. I tested this within the testbench:

# clean make and install of testbench
rm -r build .local
cmake -B build -S .
cd build
make install
# remove source and build
cd ..
rm -r Bench cmake Framework build
# check that we can still run
fire config/produce.py
# it does :tada:

To un-do source deletion

git checkout -- Bench cmake Framework
git submodule update --init
git -C Framework checkout update-dict-gen
git -C cmake checkout Framework-73-update-dict-gen

@tomeichlersmith
Copy link
Member Author

I can mock-up some CMake stuff to only generate the LinkDef when its inputs change, but - for some reason I cannot understand - the dictionary fails to link properly when re-compiling without re-writing the LinkDef.

eichl008@framework-testbench:~/build$ cmake ..
-- Configuring done
-- Generating done
-- Build files have been written to: /export/scratch/users/eichl008/ldmx/framework-testbench/build
eichl008@framework-testbench:~/build$ make                                                                         
Consolidate compiler generated dependencies of target Bench_Event
[  6%] Built target Bench_Event
Consolidate compiler generated dependencies of target Framework_Exception
[ 13%] Built target Framework_Exception
[ 16%] Generating EventDict.cxx, libFramework_rdict.pcm, libFramework.rootmap
Warning: Unused class rule: RunHeader
Warning: Unused class rule: EventHeader
Warning: Unused class rule: Hit
Scanning dependencies of target EventDict
Consolidate compiler generated dependencies of target EventDict
[ 20%] Building CXX object Framework/CMakeFiles/EventDict.dir/EventDict.cxx.o
[ 20%] Built target EventDict
Consolidate compiler generated dependencies of target Framework
[ 23%] Linking CXX shared library libFramework.so
[ 80%] Built target Framework
Consolidate compiler generated dependencies of target fire
[ 83%] Linking CXX executable fire
/usr/bin/ld: libFramework.so: undefined reference to `typeinfo for ldmx::RunHeader'
/usr/bin/ld: libFramework.so: undefined reference to `vtable for ldmx::EventHeader'
/usr/bin/ld: libFramework.so: undefined reference to `vtable for ldmx::RunHeader'
/usr/bin/ld: libFramework.so: undefined reference to `ROOT::GenerateInitInstance(ldmx::RunHeader const*)'
/usr/bin/ld: libFramework.so: undefined reference to `typeinfo for ldmx::EventHeader'
collect2: error: ld returned 1 exit status
make[2]: *** [Framework/CMakeFiles/fire.dir/build.make:121: Framework/fire] Error 1
make[1]: *** [CMakeFiles/Makefile2:248: Framework/CMakeFiles/fire.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

I'm leaving this here for future in case someone thinks of someway around this. For now, I think re-linking is a quick enough procedure that we can ignore that burden and just re-write the LinkDef if cmake is called again.

CMake Code to re-write LinkDef if Inputs Change
macro(write_linkdef)
  # the following writes the LinkDef file using the CMake global variables
  # namespaces and dict that are lists appended to by the register_event_object function
  message(STATUS "Building ROOT dictionary LinkDef")
  set(linkdef_filepath ${PROJECT_BINARY_DIR}/include/Framework/EventDictLinkDef.h)
  file(WRITE ${linkdef_filepath} "
#ifdef __CINT__

#pragma link off all globals;
#pragma link off all classes;
#pragma link off all functions;

#pragma link C++ nestedclass;
#pragma link C++ nestedtypedef;

")
  list(REMOVE_DUPLICATES namespaces)
  foreach(namespace ${namespaces})
    file(APPEND ${linkdef_filepath} "#pragma link C++ namespace ${namespace};\n")
  endforeach()

  foreach(entry ${dict})
    file(APPEND ${linkdef_filepath} "#pragma link C++ class ${entry}+;\n")
  endforeach()

  file(APPEND ${linkdef_filepath} "\n#endif\n")
endmacro()

set(linkdef_inputs "namespaces=${namespaces}\ndict=${dict}")
set(linkdef_inputs_filepath ${PROJECT_BINARY_DIR}/LinkDef.inputs)
if (NOT EXISTS ${linkdef_inputs_filepath})
  file(WRITE ${linkdef_inputs_filepath} "${linkdef_inputs}")
  write_linkdef()
else()
  file(READ ${linkdef_inputs_filepath} linkdef_current_inputs)
  if ("${linkdef_current_inputs}" STREQUAL "${linkdef_inputs}")
    # re-cmake without modification to linkdef
  else()
    # re-cmake with modification to linkdef
    file(WRITE ${linkdef_inputs_filepath} "${linkdef_inputs}")
    write_linkdef()
  endif()
endif()

@tomeichlersmith tomeichlersmith marked this pull request as ready for review October 9, 2023 18:04
@tomeichlersmith tomeichlersmith merged commit 79f9456 into trunk Oct 9, 2023
@tomeichlersmith tomeichlersmith deleted the update-dict-gen branch October 9, 2023 18:17
@EinarElen
Copy link
Contributor

This looks really good. One question, does the placement of the headers in the build directory mean that an install will depend on the contents of the build directory? For a normal C++ setup it shouldn't but ROOT isn't always a normal C++ setup

@tomeichlersmith
Copy link
Member Author

tomeichlersmith commented Oct 10, 2023

Short Answer: No

Long Answer: The LinkDef header is only used during the build and compilation of the dictionary. Once the *.pcm file is successfully built, that is the file that is used at run time (along with the installed headers of those classes and dynamic libraries). I've also manually checked this logic by installing framework-testbench and then removing the build and source files before running (which works like normal).

@EinarElen
Copy link
Contributor

Super!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants