First implementation of ITS3 geometry - #3699

I'll wait for #3702 to be merged, in order to implement the ENABLE_UPGRADES guards.

I'll wait for #3702 to be merged, in order to implement the ENABLE_UPGRADES guards.

It should be sufficient to add the add_subdirectory in Detectors/Upgrades/CMakeLists.txt which is already protected. Or do we need to protect anything outside of that part?

I'll wait for #3702 to be merged, in order to implement the ENABLE_UPGRADES guards.

It should be sufficient to add the add_subdirectory in Detectors/Upgrades/CMakeLists.txt which is already protected. Or do we need to protect anything outside of that part?

Some other parts of files, like the ones involved in extending the simulation tool have to be protected. Not a big deal at the moment.

@mconcas I think you can go ahead and implement the updates using ENABLE_UPGRADES. Let me know if anything is missing.

@mconcas I think you can go ahead and implement the updates using ENABLE_UPGRADES. Let me know if anything is missing.

The ENABLE_UPGRADES option is a CMake option, I am currently figuring out how to pass the definition of the preprocessor macro in a way that is accepted by the build, because at the moment it behave like ENABLE_UPGRADES macro is not passed.

Hi @sawenzel I think I adjusted the code.
I had to manually add the ENABLE_UPGRADE macro in DataHeader.h.
I was not able to propagate it by using the option added by @qgp, but it does not work by passing it to the cmake command in the O2 recipe.
Any suggestion on that?
For the rest the guards have been included and build should be safe, I think it should be ready to be tested.

PS. I noticed that for loops on detectors ids usually go from DetID::First to <= DetID::Last.
Is there a specific reason to that?

Thanks and Cheers

Taking a look now. I think the cmake is just offering the option ENABLE_UPGRADE but not yet setting the compiler definitions.

@mconcas @qgp : This PR needs #3764 as well as alisw/alidist#2296. Thereafter you should be able to compile this with
ENABLE_UPGRADES=ON aliBuild build O2 --defaults o2.

I would suggest to wait until these things are merged and then we can adjust this PR.

@mconcas For the targets that need the definition, you should add something like

if (ENABLE_UPGRADES)
  target_compile_definitions(<target> PUBLIC ENABLE_UPGRADES)
endif()

Or did I miss what you were asking for?

@mconcas For the targets that need the definition, you should add something like

if (ENABLE_UPGRADES)
  target_compile_definitions(<target> PUBLIC ENABLE_UPGRADES)
endif()

Or did I miss what you were asking for?

That is also something I was looking for (i tried now to put it here).
The actual point is that the option(...) does not seem to play how I expect, I am doing some test now, the build directory everytime. No success at the moment.

Also the CI is complaining:

  CMake Error at run/CMakeLists.txt:16 (target_compile_definitions):
  target_compile_definitions may only set INTERFACE properties on INTERFACE
  targets

I'm trying by using the INTERFACE keyword.

@mconcas For the targets that need the definition, you should add something like

if (ENABLE_UPGRADES)
  target_compile_definitions(<target> PUBLIC ENABLE_UPGRADES)
endif()

Or did I miss what you were asking for?

Please note that this is no longer necessary. Upgrade macro definitions are now enabled in the main CMake file.

@sawenzel @mconcas Ok, so we can globally assume to use #ifdef ENABLE_UPGRADES as guard for upgrade only stuff. Are such conventions documented somewhere?

I guess it can and should be documented in a dedicated Run4WhateverUpgrade.md file.

I guess it can and should be documented in a dedicated Run4WhateverUpgrade.md file.

Sounds good, I will take care of adding such a file in doc/.

So shall we go ahead merging this PR?

Fine to me