Cmake migration - #2172

@aphecetche @ktf : Can it be that the CI server for build/o2checkcode/o2 does not have simulation packages pythia and geant? Or why is it failing?

Mmm... Pythia and geant3 we build our own, so I would say no, it's not possible.

As far as I can tell from the logs of other PRs, indeed it seems the CI workers for o2checkcode don't seem to find simulation related packages (see e.g. https://ali-ci.cern.ch/repo/logs/AliceO2Group/AliceO2/2169/529687bc5f8c57ea8595cd7a10121ead688c1fbb/build_o2checkcode_o2/fullLog.txt) . Which is a thing I kind of knew, but then overlooked.

Now this is a bit of a problem because in my cmake-migration I now have a BUILD_SIMULATION option (that is ON by default). If that one is set then missing simulation-related packages (pythia6,pythia,geant3,geant4,geant4vmc...) is considered a fatal error.

So the options are :

1. add the simulation dependencies to the CI workers for o2checkcode or make them aware of the existing deps if they are indeed there
2. add the -DBUILD_SIMULATION=OFF for the o2checkcode builds
3. = ~2, i.e. make BUILD_SIMULATION=OFF the default in this PR and change o2.sh to set BUILD_SIMULATION=ON

(I would avoid 3)

In any case, it seems I kind of failed my initial goal of fitting within the existing CI boundaries ;-)

BTW, is this actually doing some magic instead of relying on the Cmake command line options? If yes, I think this is a terrible idea...

I don't know exactly what you consider magic, but there is indeed a piece of adapter cmake code, to make the thing works with the current o2.sh, as I did not want to have to change the recipe to get this one working (but eventually the o2 recipe will have to be changed a bit)

That adapter's working fine on my tests under CentOS, macOS and Ubuntu. The macOS CI seems to agree as well. I would wait for the O2/o2 to pick the right QC so see if the issue is limited to o2checkcode before jumping to conclusions.

There is also something strange in the Travis CI, which complains about files which are no longer in the code (e.g. Examples/ExampleModule1/src/Foo.cxx)

Ah, ok, I missed it's something temporary. As long as nothing overrides -D options in the final version, I am fine (of course, dropping the options or changing them is also fine).

I would make BUILD_SIMULATION=OFF if pythia and co are not found, and BUILD_SIMULATION=ON if they are found. If people specify on the command line -DBUILD_SIMULATION=ON, then you complain. I.e. never have an implicit flag which requires explicit configuration to work correctly.

well, it's not an implicit flag, it's an option (granted, should document the few options we have somewhere, but all are defined in https://github.com/aphecetche/AliceO2/blob/cmake-migration/cmake/O2DefineOptions.cmake)

I get the impression that computing the option value (from the presence or not of some packages) is the implicit think that we should not do (i.e. we disagree on this one ;-) )

Anyway, before changing anything, I am running a bit blind on the CI side : I think I've seen the build/O2/o2 ran again but fail again (as the QC version it's picking is still the old one). Is there a way to see when it was attempted last and/or to know when it will be attempted again ?

It's implicit in the sense that you do not need to pass it on command line. What happens in general with well behaved configure / cmake using software is the following:

• Things work if you just say "./configure" or "cmake ." with some configuration which is compatible with your system.
• If you specify an option explicitly, boundary conditions for that option are checked and you get an error if they are not satisfied.

IMHO, diverging from this behavior, like you are doing here, might be debatably better but it's for sure not what happens in general, so I would avoid doing so.

... or if you prefer: "if something is optional, it's off by default" is also a viable solution.

indeed, I first tried the optional=off approach (which I like) but with it the current CI would behave differently as of now, i.e. it would not build the simulation dependent parts (as the BUILD_SIMULATION is not part of the o2.sh recipe) ...

Ok, but then I would keep the original behavior (which is what in general configure / cmake do for implicitly on options). Switching to "implicitly off" will then be just a matter of changing ON to OFF in the default value.

Now I'm confused... (will have to go to lunch and get some sugar in my brain ;-) ) Do you propose that BUILD_SIMULATION is set to ON by default (which it is, btw) and set to OFF if simulation parts are not found, i.e. we gracefully build whatever the case may be ? (instead of bailing out as I do now ?)

In general the behaviour is:

• Implicit ON => Autodetect and silently pass if not found
• Implicit OFF => Disable
• Explicit ON => Detect and fail if not found
• Explicit OFF => Disable

Then you decide whether you want the default to be ON or OFF, but the behaviour described above is always the same.

ok, will try to change the logic.

here's a try with a re-worked BUILD_SIMULATION logic.

(concerning the commits : once everything is green I'll get back to the 3 commits initial idea : the cmake* stuff itself, the documentation, and the actual source C++ code that had to be touched)

@kft the build/O2/o2 is in "expected" mode since a long time, is there any issue ?

Never mind, there's now a conflict ...

I have one more bugfix and one more proposal for add_root_dictionary:

Bugfix: Absolute -rml path argument leads to hardcoded build paths in rootmap files which is not what we want.

diff --git a/cmake/AddRootDictionary.cmake b/cmake/AddRootDictionary.cmake
index d4428a8ac..50f996fbb 100644
--- a/cmake/AddRootDictionary.cmake
+++ b/cmake/AddRootDictionary.cmake
@@ -131,7 +131,7 @@ function(add_root_dictionary target)
       ${dictionaryFile}
       -inlineInputHeader
       -rmf ${rootmapFile}
-      -rml $<TARGET_FILE:${target}>
+      -rml $<TARGET_FILE_NAME:${target}>
       $<GENEX_EVAL:-I$<JOIN:$<TARGET_PROPERTY:${target},INCLUDE_DIRECTORIES>,\;-I>>
       # the generator expression above gets the list of all include
       # directories that might be required using the transitive dependencies

Proposal: Rename G__<LIB>_rdict.pcm to lib<LIB>_rdict.pcm (This is following the convention how ROOT6 names its pcm files):

diff --git a/cmake/AddRootDictionary.cmake b/cmake/AddRootDictionary.cmake
index d4428a8ac..7d45579ec 100644
--- a/cmake/AddRootDictionary.cmake
+++ b/cmake/AddRootDictionary.cmake
@@ -101,7 +101,8 @@ function(add_root_dictionary target)
   endif()
   set(dictionary G__${basename})
   set(dictionaryFile ${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.cxx)
-  set(pcmBase ${dictionary}_rdict.pcm)
+  set(pcmTarget lib${basename})
+  set(pcmBase ${pcmTarget}_rdict.pcm)
   set(pcmFile ${lib_output_dir}/${pcmBase})
   set(rootmapFile ${lib_output_dir}/lib${basename}.rootmap)

@@ -132,6 +133,7 @@ function(add_root_dictionary target)
       -inlineInputHeader
       -rmf ${rootmapFile}
       -rml $<TARGET_FILE:${target}>
+      -s ${pcmTarget}
       $<GENEX_EVAL:-I$<JOIN:$<TARGET_PROPERTY:${target},INCLUDE_DIRECTORIES>,\;-I>>
       # the generator expression above gets the list of all include
       # directories that might be required using the transitive dependencies

Now:

└── lib64
    ├── G__testlib_rdict.pcm
    ├── libtestlib.rootmap
    └── libtestlib.so

vs proposed:

└── lib64
    ├── libtestlib_rdict.pcm
    ├── libtestlib.rootmap
    └── libtestlib.so

edit: In addition to the above honor the PREFIX target property.

diff --git a/cmake/AddRootDictionary.cmake b/cmake/AddRootDictionary.cmake
index d4428a8ac..658d662da 100644
--- a/cmake/AddRootDictionary.cmake
+++ b/cmake/AddRootDictionary.cmake
@@ -99,11 +99,16 @@ function(add_root_dictionary target)
   if(NOT basename)
     set(basename ${target})
   endif()
+  get_property(prefix TARGET ${target} PROPERTY PREFIX)
+  if(NOT prefix)
+    set(prefix lib)
+  endif()
   set(dictionary G__${basename})
   set(dictionaryFile ${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.cxx)
-  set(pcmBase ${dictionary}_rdict.pcm)
+  set(pcmTarget ${prefix}${basename})
+  set(pcmBase ${pcmTarget}_rdict.pcm)
   set(pcmFile ${lib_output_dir}/${pcmBase})
-  set(rootmapFile ${lib_output_dir}/lib${basename}.rootmap)
+  set(rootmapFile ${lib_output_dir}/${prefix}${basename}.rootmap)

   # get the list of compile_definitions and split it into -Dxxx pieces but only
   # if non empty
@@ -131,7 +136,8 @@ function(add_root_dictionary target)
       ${dictionaryFile}
       -inlineInputHeader
       -rmf ${rootmapFile}
-      -rml $<TARGET_FILE:${target}>
+      -rml $<TARGET_FILE_NAME:${target}>
+      -s ${pcmTarget}
       $<GENEX_EVAL:-I$<JOIN:$<TARGET_PROPERTY:${target},INCLUDE_DIRECTORIES>,\;-I>>
       # the generator expression above gets the list of all include
       # directories that might be required using the transitive dependencies

Maybe offtopic, not sure: Can someone enlighten me why the build paths are baked into these pcm files?

e.g.

╭─dklein@gamma ~/projects/AliceO2/test ‹ruby-2.6.0› ‹cmake-migration*›
╰─➤  strings ~/alice/sw/fedora30_x86-64/FairRoot/latest/lib/G__BaseDict_rdict.pcm | grep BUILD
TFileh/home/dklein/alice/sw/BUILD/6645368508eba6377cfd84d95691480eb41f5e4d/FairRoot/base/G__BaseDict_rdict.pcm
h/home/dklein/alice/sw/BUILD/6645368508eba6377cfd84d95691480eb41f5e4d/FairRoot/base/G__BaseDict_rdict.pcm
TFileh/home/dklein/alice/sw/BUILD/6645368508eba6377cfd84d95691480eb41f5e4d/FairRoot/base/G__BaseDict_rdict.pcm
TFileh/home/dklein/alice/sw/BUILD/6645368508eba6377cfd84d95691480eb41f5e4d/FairRoot/base/G__BaseDict_rdict.pcm
╭─dklein@gamma ~/projects/AliceO2/test ‹ruby-2.6.0› ‹cmake-migration*›
╰─➤  strings ~/alice/sw/fedora30_x86-64/ROOT/latest/lib/libEve_rdict.pcm | grep BUILD
TFile^/home/dklein/alice/sw/BUILD/17fe86512615f50b92686cb41c298606a5967038/ROOT/lib/libEve_rdict.pcm
^/home/dklein/alice/sw/BUILD/17fe86512615f50b92686cb41c298606a5967038/ROOT/lib/libEve_rdict.pcm
TFile^/home/dklein/alice/sw/BUILD/17fe86512615f50b92686cb41c298606a5967038/ROOT/lib/libEve_rdict.pcm
TFile^/home/dklein/alice/sw/BUILD/17fe86512615f50b92686cb41c298606a5967038/ROOT/lib/libEve_rdict.pcm

It seems this is already the case for ROOT's own pcm files. Makes me wonder what is the purpose of these files? I thought they were some kind of ROOT-style precompiled headers?

@dennisklein I've applied your first fix (the relative build path for root map). The other one can wait a bit I guess.

Well, removal of BASENAME parameter of add_root_dictionary at this stage was not such a good idea after all ;-) as QC uses it... My bad. Added a protection.

One (last?) fix removing one test macro when pythia8 is missing (of o2checkcode).

@aphecetche You could also run git-clang-format once and fix the codacy warnings, should only take 1 minute

But actually she has a dependency to Vdt::Vdt
So this should not come from ROOT, since root just puts in the lib directly (vdt, or vdt with path), or could this have been different with the older ROOT version.

Perhaps another package pulls in the Vdt requirement?

ok, it is actually the ROOT version, 6-14-06 was detecting Vdt differently and pulled in Vdt::Vdt. I guess it will compile for @ihrivnac now with 6-16-00, but let's see.

@ihrivnac yes the older Root version might explain this. There have been some changes around the treatment of the Vdt dependency in the Root repo (see e.g. https://github.com/root-project/root/pull/2863/files).
So let's wait for your build, but I'm confident it will be ok.

That also means this is something to watch out in the next Root version, in case it's changed again or fixed.

The build has finished ok, as you expected :)

I am ok with merging (but can't do it); I may have some suggestions afterwards.

I've rebased my work area but now I get:

402 CMake Error at /nix/store/kxfkny9867qzndaskvvbk14h3i1p5hpz-clang-7.1.0/lib/cmake/clang/ClangConfig.cmake:10 (find_package):
403   Could not find a package configuration file provided by "LLVM" with any of
404   the following names:
405
406     LLVMConfig.cmake
407     llvm-config.cmake
408
409   Add the installation prefix of "LLVM" to CMAKE_PREFIX_PATH or set
410   "LLVM_DIR" to a directory containing one of the above files.  If "LLVM"
411   provides a separate development package or SDK, be sure it has been
412   installed.
413 Call Stack (most recent call first):
414   dependencies/O2Dependencies.cmake:133 (find_package)
415   dependencies/CMakeLists.txt:11 (include)
416   CMakeLists.txt:44 (include)
417
418
419 -- Configuring incomplete, errors occurred!

ideas?

I see:

 find_package(LLVM)
137 set_package_properties(LLVM PROPERTIES TYPE OPTIONAL)

when did we add LLVM to our requirements?

It is optional, as it is needed for the OpenGL2 GPU build.
I wonder why you get this error, since it is explicitly marked as optional.

but vdt is nowhere defined as a target.

Why does it need to be a target?

edit: nvm, I see, ROOT_LIBRARY_DIR was probably not searched by the linker.

#2183 fixes some of the problems. I also had to disable the doc folder and I am still fighting vs vdt...

@ktf @davidrohr indeed in O2 LLVM is marked as optional, but the error you see is coming from the Clang config ... which is not finding its companion LLVM. At least this is why I see from the alibuild installed Clang (can only assume it's similar for the nix one ?)

[ cmake ]~/alice/cmake/sw/osx_x86-64/Clang/latest/lib/cmake/clang$ more ClangConfig.cmake
# This file allows users to call find_package(Clang) and pick up our targets.

# Compute the installation prefix from this LLVMConfig.cmake file location.
get_filename_component(CLANG_INSTALL_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)

find_package(LLVM REQUIRED CONFIG
             HINTS "${CLANG_INSTALL_PREFIX}/lib/cmake/llvm")

@ktf, OK, you already solved it, I was too slow ;-)

Do I really need to bump root to 6.16?

For vdt, you mean ? I'm afraid yes. But 6.16 is the default version for O2, isn't it ?

yeah, but I've some developments in 6.14.8 which I need to forward port... We should change the required version as well, BTW...

Indeed, should I open a PR for that or do you want to add it to your #2183 ?

Root version fixed in #2183 as well.