[WIP] Cmake migration - #2170

@aphecetche : I just tried your branch. It compiles, but I have some comments below.
However, o2-sim gets stuck, when I build O2 with your branch, while it works with the origin/dev branch.

• you forgot to merge the additional optional dependencies I had added:

find_package(GLEW)
find_package(OpenGL)
find_package(Clang)
find_package(LLVM)

Could you add this to dependencies/O2Dependencies.cmake?

• Your branch fails to build if one has libfmt installed as a system package, since the framework picks up fmt.h, but the CMake does not link v.s. libfmt. @ktf: there is already an fmt.sh in alidist, why don't we just add it as O2 dependency now?

• There is a problem with the includes of Framework/DebugGUI:
  I think you do not see it since you don't have OpenGL in.
  Now, if one puts FrameworkDebugGUI as dependency but not FrameworkCore, the compilation will fail, since Framework/DebugGUI needs headers from FrameworkCore, but it does not have FrameworkCore as dependency and also not Core/include as PUBLIC_INCLUDE_DIRECTORY.
  Unfortunately, adding FrameworkCore as dependency will break all CUDA builds that depend on DebugGUI (at least for me), since the settings imposed by the ROOT dependency and the CUDA dependency are incompatible. I am not sure what is the right thing to do here. In principle, DebugGUI does not depend on Core, it just needs the DebugGUI.h header, which is fore some reason in Core. Perhaps one should just move this header to DebugGUI @ktf. If this does not work, I'd suggest to set Core/include as PUBLIC_INCLUDE_DIRECTORY for DebugGUI, this works for me.

AFAICT libfmt is a dependency already:

https://github.com/alisw/alidist/pull/1701/files

As you may have noticed, this branch is not yet fully functional. There is indeed a problem with o2-sim which I'm currently trying (and failing so far ... ) to understand.

For the missing deps, sorry I missed those, will add them back.

thx @ktf, then I'd also add fmt as required package in the new cmake.

o2-sim issue is now fixed (a very stupid but very long to find typo in the name of one of the 3 executables used by o2-sim ...). Missing deps were added. Let's see how the CI goes.

More cleanings tomorrow.

@aphecetche : I have another general question. I think you explicitly add tests for all macros, while before we were testing all macros with some explicit exceptions. I actually liked before that all macros are tested. Otherwise, people can check in plenty of "private" macros that bread over time, since people will rather not add the macro as test instead of fixing it I guess.

@dberzano @ktf : It seems GLEW is not available on the CI build servers. Could we install it, since it is needed for the GPU tracking visualization. @ktf uses gl3w, but the gl3w we have seems to be not at OpenGL version 4.5, and only provides the core profile which is currently insufficient for me.

@davidrohr regarding the macros. I indeed disliked the fact that all the macros testing is stuffed in the top CMakeLists.txt. And together with a long list of exclusions (which, IMO, doesn't seem to shrink at a reasonable pace, or at even all). (And I think we have way too many macros that takes too much time to test, but that's maybe just me)

Besides, my original intent was to be able to do a ctest without setting the full alienv env. i.e. once you've make the cmake configure stage, all the generated files should have all the information they need to be able to run. That is doable, but "requires" to be explicit in the dependencies of each target, including the macros (instead of, like now, assuming all macros need all the libraries and all the includes). I would argue that is anyway good practice (and already pointed a couple of issues with circular deps). That ctest-without-env is mostly working for non-macro tests. But not for the macros currently. To be able to launch a root process to test a macro, one needs a correct LD_LIBRARY_PATH and a correct ROOT_INCLUDE_PATH. While the ROOT_INCLUDE_PATH can be derived from the dependencies quite easily, I've so far failed to do the same for LD_LIBRARY_PATH (see O2AddTestRootMacro.cmake . So I've currently abandoned the idea of "env. free" dev. mode, for now. (that would anyway be a departure from current practices and as such would required discussions).

Granted, with the explicit declaration mode of macro tests, there is the risk of getting more macros that are introduced and not tested. You have a point here. But that is something that can be detected (by humans) at PR review time. I can also add a script or something that make a list of the macros which are present in the repository and not tested (which could help the reviewers and/or be turned into an automatic test to see we are not regressing). I guess that would be preferable to returning to globbing the macros in the top CMakeLists.txt and manually listing the exclusions (and separately for compiled and loading modes, in addition...).

@davidrohr I really appreciate the time you take to review this PR. But there might be one piece of information I forgot to mention. I'll disappear for 4 weeks next Thursday. So the question now is whether we target to merge this before I leave (at the expense of postponing a few things for later on) or we postpone it altogether until mid/end- august.
I would of course be in favour of the first option ( ;-) ) but that's of course open for discussion.

I assume this is a matter of taste, but to me the old scheme was shorter (in terms of CMake lines) and simpler (less things to check for the reviewers, less code to add for the developer).

I also prefer to grant access to the full O2 environment to macros, otherwise plenty of detector macros will probably have to be moved to the global macro folder to avoid circular dependencies. I guess there is a use case for detector specific macros having access to other libraries, which are built later in the built process.

Finally, I think there will definitely be macros that can only be tested in compiled or only in interpreted mode. The interpreted mode will fail with external (or GPU) dependencies which do not have a root dictionary, but they can still run in compiled mode.

@ktf @dberzano @Barthelemy the build/O2/o2 is failing (at least) because it is testing an old version (0.14.2) of QualityControl instead of one of the latests (>=0.15.0). I guess that's because the QC recipe has not been bumped to one of those versions ? Assuming a bump is all it takes, I've opened an alidist PR for that.

@aphecetche : I am definitely in favor of getting it merged before! Having it lying around for 4 weeks will require to catch up afterwards again. Additional changes can certainly be implemented later.

(closing temporarily so I can force push without triggering the CI checks)

Well, don't seem to be able to reopen this one, so opened a new one : #2172

@aphecetche : I just had a discussion with @shahor02 and also he mentioned he'd prefer to have all macros tested if not explicitly excluded. Otherwise people will not add tests for their macros, and reviewers will overlook this, and we'll have plenty of macros failing when the code is updated.

How about we do it the following way:
We leave all the explicit tests like they are now.
At the end of the CMake, you already print a list of untested macros.
We just add a manual list of explicitly disabled macros, and then we throw a fatal_error for every untested macro that is not in this list?

@davidrohr @shahor02 @ihrivnac this is something to discuss more widely maybe, as I guess I still disagree ;-)
I believe instead of making it easy to get added macros tested, we should make it easy to reduce the number of macros...

Disabling (if any) should not be in the top level CMakeLists.txt IMO. At the very least must have a separate cmake file list for that. But to disable a test macro, why not just comment out the relevant o2_add_test_root_macro in the corresponding CMakeLists.txt in the relevant sub directory ?

Also, are our 130 macros really useful / used ? What's the purpose of those macros (as they take a long time to test, so they should bring some real value) ? If some are meant to check the functionality of some piece of code, I would argue those ones should be converted to proper tests. If some pre-date workflows, are they still relevant ? etc...

All in all I believe the macros situation warrants a review.

@aphecetche I agree that we have some obsolete macros and periodic cleanup is needed. But most of them are used (e.g. it is easier to debug the code steered by a macro than by the DPL).

By not running compilation tests for all macros (except some justified cases) we risk to accumulate lot of broken code. In opposite, having them in compilation test will allow to spot when it becomes obsolete and trigger either its fix or removal.

@shahor02 @davidrohr Maybe I misunderstood David's earlier comment completely, sorry about that, so let me rephrase.
What you propose is to ensure that the list of currently not tested macro is empty (instead of containing 30 macros right now), i.e. we explicitly add tests for those 30, or we add them to an exclusion list.
Then, if a (new) macro is not in the exclusion list or in the list of tested macro, that's a FATAL_ERROR at configure time.
If that's the plan, then I agree and can prepare a PR for that (assuming all the 30 macros go in the exclusion list for now, for instance)
I thought you were proposing to come back to automatically add tests for all macros (which btw, is not technically sound as we need to get the dependencies correct to add a test).

@aphecetche I would expect that whatever is not in the exclusion list (for a justified reason) should be tested.
But if you say that to get the dependencies right we need to have o2_add_test_root_macro for every macro and macros which are in neither list will lead to FATAL, then it should be OK, provided we reduce exclusions as much as possible.

yes, calling o2_add_test_root_macro is required.

See #2188 as a possible implementation of the logic outlined in the comments above.