R3BRootGroup / R3BRoot

Framework for Simulations and Data Analysis of R3B Experiment
https://github.com/R3BRootGroup/R3BRoot/wiki
GNU General Public License v3.0
18 stars 104 forks source link

[FairRoot 19] Apply Modern CMake and clear dependencies #911

Closed YanzhaoW closed 5 months ago

YanzhaoW commented 1 year ago
  1. Use cmake targets to specify the include search path for C++ source files.
  2. Eliminate circular dependency between r3bbase and r3bsource introduced from #892 .
  3. Naming: change R3Bsource to R3BSource

Motivation

  1. PR #910 fails when building with Sofia dependencies. This is because the current method to build root dictionary relies on cmake include function which takes directories of all needed header files. This is very inflexible when new folders are created in base directories such as r3bbase. To make the building successfull, every other CMakefiles that utilize the base functionality has to include the new folder as well. This happens in #910 where Sofia has its own lmd source reader in a different folder but it relies on R3BReader.h in r3bsource/base. To fix this, Sofia project has to include r3bsource/base/utils as well even though nothing was changed in the Sofia repository.

  2. Starting from FairRoot 19, it's required to load ROOT package using CONFIG mode. Loading ROOT using old MODULE mode would fail due to the breaking changes from ROOT side (see here). Therefore, to make R3BRoot compatible with old FairRoot versions, it's also better to switch to cmake targets.

This PR could be merged after switching to FairRoot 19.0.

Any comments are welcomed.


Checklist:

YanzhaoW commented 6 months ago

Hi, @jose-luis-rs

Is it possible to merge this pull request even before we go to FairRoot 19? The changes will only be made in the CMakeFiles and nothing will be changed in the source file. Therefore, this PR will not alter any algorithms.

jose-luis-rs commented 6 months ago

Hello @YanzhaoW

The problem is that the code fails with the tests for simulation and unpacking, so I cannot accept it at the moment because this will block other developments to the dev branch and also the current analysis if our collaborators do a rebase of this version.

Next week we will try to have available FairSoft-jan24p1 and FairRoot-v18.8.2 in /cvmfs/fairsoft.gsi.de, I will also ask for the installation of FairRoot-v19.0.0 since it is now available at https://github.com/FairRootGroup/FairRoot/releases/tag/v19.0.0

If everything goes fine, we could accept this PR by the end of May.

YanzhaoW commented 6 months ago

Hi, @jose-luis-rs

The problem is that the code fails with the tests for simulation and unpacking, so I cannot accept it at the moment because this will block other developments to the dev branch and also the current analysis if our collaborators do a rebase of this version.

I didn't mean to merge it now :D. I will fix every problem in the CI before merging to the dev branch.

YanzhaoW commented 6 months ago

Hey @jose-luis-rs , CI tests for R3BRoot and glad-tpc have been fixed. Could you merge the pull request in the sofia repo and restart the CI test here?

jose-luis-rs commented 6 months ago

Hello @YanzhaoW

I will check the modifications, thanks.

Another question related to this upgrade, I am using the FairSoft version Jan24 with Root 6.30.02 and when I save a figure as a ".C" then I cannot execute it with root because the definitions for TCanvas and other objects are wrong, for example:

error: no matching constructor for initialization of 'TCanvas' TCanvas c_projection = new TCanvas("c_projection", "c1",1,920,37,1,920,1,043); ^ ~~~~~~~~~~~ /jan24/buildjan24/Build/root/include/TCanvas.h:103:4: note: candidate constructor not viable: requires 6 arguments, but 9 were provided TCanvas(const char name, const char *title, Int_t wtopx, Int_t wtopy, ...

Did you find similar problems?

YanzhaoW commented 6 months ago

@jose-luis-rs

No, I didn't have such an issue.

TCanvas *c_projection = new TCanvas("c_projection", "c1", 1, 920, 37, 1, 920, 1, 043);

I think this is a wrong constructor. TCanvas can only take 6 parameters instead of 9. Here you can check the available constructors. Maybe there is a typo? It should be 1920 instead of 1,920?

YanzhaoW commented 6 months ago

Additionally, this PR also clean the dependency tree a little bit.

The old dependency tree is like this: r3bbase_mess

And after cleaning up, it becomes better with this: r3bbase

YanzhaoW commented 5 months ago

Hi @jose-luis-rs. Have you checked the modifications?

Since there are cyclic dependencies between R3BRoot and the sub projects (sofia, glad, etc), the errors in the CI cannot be fixed only with this PR. Therefore, I would suggest we could merge this PR first and then push further PRs in the sub projects to fix the errors.

jose-luis-rs commented 5 months ago

Thanks @YanzhaoW

I would like to take a look at your modifications, but due to other tasks I didn't have time yet. Sorry for that!

Additionally, the FairSoft team is still working on the jan24p1 release, and I would like to have this new version available before merging this PR.

YanzhaoW commented 5 months ago

Thanks @jose-luis-rs

But I don't think jan24p1 of FairSoft matters for this PR, as the PR should be backward-compat towards older versions of FairSoft & FairRoot. As you can also see, this PR has lots of CMakeLists changes and it would be really nice to have it merged before the other PRs to avoid inconvenient merge conflicts.

jose-luis-rs commented 5 months ago

Many thanks @YanzhaoW

YanzhaoW commented 5 months ago

@jose-luis-rs Thanks. If there are some linking error issues, I will make further quick fixes ASAP.