Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

YanzhaoW
Copy link
Contributor

@YanzhaoW YanzhaoW commented Nov 28, 2023

  1. Use cmake targets to specify the include search path for C++ source files.
  2. Eliminate circular dependency between r3bbase and r3bsource introduced from Implementation of the Tprev/Tnext task. #892 .
  3. Naming: change R3Bsource to R3BSource

Motivation

  1. PR R3BUcesbSource2 and add runtime struct info checking #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 R3BUcesbSource2 and add runtime struct info checking #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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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 YanzhaoW force-pushed the edwin_cmake branch 6 times, most recently from 01ddd43 to 31519f0 Compare May 21, 2024 16:10
@YanzhaoW
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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 YanzhaoW marked this pull request as ready for review May 23, 2024 11:02
@YanzhaoW
Copy link
Contributor Author

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 YanzhaoW force-pushed the edwin_cmake branch 2 times, most recently from cbf609e to 3513a6c Compare May 27, 2024 14:23
@YanzhaoW
Copy link
Contributor Author

YanzhaoW commented Jun 3, 2024

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.

CMake targets imported from ROOT using config mode is suggested both
from ROOT and FairRoot. Libraries generated with different detectors are
also represented by proper CMake targets for easier dependency
management.
@jose-luis-rs
Copy link
Contributor

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
Copy link
Contributor Author

YanzhaoW commented Jun 6, 2024

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
Copy link
Contributor

Many thanks @YanzhaoW

@jose-luis-rs jose-luis-rs merged commit e4be10c into R3BRootGroup:dev Jun 10, 2024
3 of 5 checks passed
@YanzhaoW
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants