iLCSoft / LCIO

Linear Collider I/O
BSD 3-Clause "New" or "Revised" License
17 stars 34 forks source link

Replacing in-built sio with FetchContent mechanism #181

Closed arummler closed 7 months ago

arummler commented 7 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

Full build which can be still stripped down by setting the cmake args. Also explicit check for in-built SIO can be replaced by commented our FetchContent argument. Proposal for realising https://github.com/iLCSoft/LCIO/issues/179

tmadlener commented 7 months ago

One of the workflows seems to have an issue with the root dictionary genaration: https://github.com/iLCSoft/LCIO/actions/runs/7086894395/job/19286078781?pr=181#step:4:473

This looks a bit like some missing include directories, resp. root being too old in that environment to pick them up from the targets

tmadlener commented 7 months ago

My suspicion is that we are missing an SIO::sio here:

https://github.com/iLCSoft/LCIO/blob/189b332730d555e95ce2306d9f7502993c5378b8/src/cpp/CMakeLists.txt#L377

arummler commented 7 months ago

Okay, I had tested locally only with the newest root 6.30.02. Always cutting edge. I will try to add it.

tmadlener commented 7 months ago

Yes, it seems to get better with newer root version at least in CI. If we can get nothing to work here easily, we can also just upgrade CI, as we are most likely not using a builtin SIO on systems that still run on effectively LCG_97.

arummler commented 7 months ago

I guess you will have to update the CI to Alma 9 soon anyhow due to EOL of CC7. At least in ATLAS we are in the middle of the migration and should be concluded until the end of the YETS.

arummler commented 7 months ago

Hmm, I think the SIO includes might not be properly filled here:

# include directories to be passed over to rootcint

SET( ROOT_DICT_INCLUDE_DIRS ${LCIO_AID_HEADERS_OUTPUT_DIR} ${LCIO_CXX_HEADERS_DIR} ${SIO_INCLUDE_DIRS}

Probably one needs to have BUILD_ROOTDICT define a proper target which depends then on SIO.

tmadlener commented 7 months ago

Ah that would make sense because the SIO_INCLUDE_DIRS are only set n SIO when it is find_packaged:

https://github.com/iLCSoft/SIO/blob/37eeb0aba1b5161bcc8f60f1e76b2d0df889bff6/cmake/SIOConfig.cmake.in#L28-L30

So maybe a get_target_property to get back the include dirs from the SIO::sio target does the trick for now.