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

Fix the error that ROOT header files can't be found #1466

Closed
wants to merge 1 commit into from

Conversation

YanzhaoW
Copy link

@YanzhaoW YanzhaoW commented Nov 20, 2023

ROOT header files can't be found because of an typo in FairMacros.cmake.

Fixed by changing ROOT_INCLUDE_DIRS to ROOT_INCLUDE_DIR

Edit:
It seems that this was changed deliberately by PR #1119.


Checklist:

@YanzhaoW YanzhaoW marked this pull request as draft November 20, 2023 16:40
@YanzhaoW
Copy link
Author

YanzhaoW commented Nov 20, 2023

Hi, @ChristianTackeGSI

I'm trying to use the dev version of FairRoot in R3BRoot. We are still using the MODULE mode to find ROOT, which runs through FindRoot.cmake. In this case, the ${ROOT_INCLUDE_DIR} is defined properly but ${ROOT_INCLUDE_DIRS} is empty.

What is the suggested way to include ROOT package?

@ChristianTackeGSI
Copy link
Member

FairRoot dev has a major CMake rewrite. See the Chabgelog.

Handling this in R3BRoot in a way that is compatible with FairRoot 18.x and 19.x at the same time is not trivial!
I once did this for PandaRoot, but I don't have a recommended way of doing it nicely / right now. Not to mention 19.x is still a moving target (also I would hope we'll release sooner than later).

The idea is that FairRoot 18.8 has most C++ changes and 19.0 has only CMake changes. I don't know, how close we are to this idea still. But once it was like this.

@YanzhaoW
Copy link
Author

Great to hear this change.

If using CONFIG mode to find ROOT is preferred, should we just use cmake targets without global variable ${SYSTEM_INCLUDE_LIBRARY} or ${BASE_INCLUDE_LIBRARY} that include root include dir? This is exactly what ROOT suggests.

In this case, we should keep it as ${ROOT_INCLUDE_DIR} for the MODULE mode and do not use global ${ROOT_INCLUDE_DIRS} if users choose to use CONFIG mode.

@ChristianTackeGSI
Copy link
Member

Right, the idea is to use exported targets from ROOT with their include paths. So one should only need to properly declare dependencies on ROOT::Core/etc and be done without needing to take care about include paths.

@YanzhaoW
Copy link
Author

YanzhaoW commented Nov 24, 2023

Hi

Sorry for the late reply.

Right, the idea is to use exported targets from ROOT with their include paths. So one should only need to properly declare dependencies on ROOT::Core/etc and be done without needing to take care about include paths.

It's great the hear that again. But I always have a problem with ROOTCINT when generating dictionary. The problem is that it seems ROOTCINT can't use the included path from the CMake targets. Every time I try to compile a library, with some cmake targets as its dependencies, rootcint seems to always fail to find the header files:

# at R3BBase cmake file:

target_include_directories(R3BBase PUBLIC folder)

# at another cmake file
set(DEPENDENCIES R3BBase)
GENERATE_LIBRARY()

even though the header file does exist inside an include folder of R3BBase cmake target. Normally this has to go round with this:

include_directories(folder)

which sets the global include_directories for all targets (including those who even don't need this folder). This works but would fail if R3BBase needs another new folder. The fix requires all libraries which are depending on R3BBase to have this new folder included in include_directories as well. All of these efforts is just to make rootcint generate dictionaries. The generation of the library actually doesn't need this because cmake targets can automatically propagate the new properties to all depended targets.

Is there any way to make rootcint work with cmake targets? Or even is there actually a plan to implement it in FairMacros?

A possible solution.

@YanzhaoW
Copy link
Author

YanzhaoW commented Nov 28, 2023

So I tried the solution mentioned above. It works (it has actually been documented in ROOT manual)

Please find the example in this PR proposed in R3BRoot. The boilerplate part is:

add_library(R3BSource SHARED ${SRCS})
target_include_directories(R3BSource PUBLIC ${DIRS} ${CMAKE_CURRENT_SOURCE_DIR})
target_link_libraries(R3BSource PUBLIC R3BData MbsAPI Ucesb)
root_generate_dictionary(R3BSource_dict ${HEADERS} MODULE R3BSource LINKDEF SourceLinkDef.h)

With this ROOT can automatically find those needed include dirs without using include().

@YanzhaoW
Copy link
Author

YanzhaoW commented Jun 3, 2024

This PR has been covered with the release of FairRoot 19.

@YanzhaoW YanzhaoW closed this Jun 3, 2024
@YanzhaoW YanzhaoW deleted the edwin_fix_fairMacro branch June 3, 2024 09:40
@ChristianTackeGSI
Copy link
Member

Thanks for taking care and cleaning up! :-)

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