R3BRoot icon indicating copy to clipboard operation
R3BRoot copied to clipboard

[FairRoot 19] Apply Modern CMake and clear dependencies

Open YanzhaoW opened this issue 2 years ago • 7 comments

  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 avatar Nov 28 '23 22:11 YanzhaoW

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.

YanzhaoW avatar May 17 '24 11:05 YanzhaoW

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.

jose-luis-rs avatar May 17 '24 16:05 jose-luis-rs

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 avatar May 17 '24 16:05 YanzhaoW

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?

YanzhaoW avatar May 22 '24 19:05 YanzhaoW

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?

jose-luis-rs avatar May 23 '24 09:05 jose-luis-rs

@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 avatar May 23 '24 10:05 YanzhaoW

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 avatar May 23 '24 11:05 YanzhaoW

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.

YanzhaoW avatar Jun 03 '24 09:06 YanzhaoW

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.

jose-luis-rs avatar Jun 06 '24 12:06 jose-luis-rs

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.

YanzhaoW avatar Jun 06 '24 13:06 YanzhaoW

Many thanks @YanzhaoW

jose-luis-rs avatar Jun 10 '24 20:06 jose-luis-rs

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

YanzhaoW avatar Jun 11 '24 12:06 YanzhaoW