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

Replace genreflex in favor of rootcling #379

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Replace genreflex in favor of rootcling #379

wants to merge 13 commits into from

Conversation

imorlxs
Copy link
Member

@imorlxs imorlxs commented Jul 11, 2024

@imorlxs imorlxs added WIP Work in progress DoNotMerge Not ready for merge labels Jul 11, 2024
@imorlxs imorlxs changed the title Replacing genreflex in favor of rootcling Replace genreflex in favor of rootcling Jul 17, 2024
cmake/FindROOT.cmake Outdated Show resolved Hide resolved
@imorlxs imorlxs added ready-for-review PR is ready for review and removed WIP Work in progress DoNotMerge Not ready for merge labels Jul 27, 2024
@imorlxs imorlxs marked this pull request as ready for review July 27, 2024 15:32
@TobiasDuswald
Copy link
Contributor

here we'd probably need you to check if this is all fine @vgvassilev .

Thanks @imorlxs 🚀

@vgvassilev
Copy link

This looks reasonable to me. Maybe we should remove the whitespace changes and configure the editor to not do them automatically.

That change is a small but important step towards getting C++ modules support for BioDynaMo.

@imorlxs
Copy link
Member Author

imorlxs commented Aug 5, 2024

This looks reasonable to me. Maybe we should remove the whitespace changes and configure the editor to not do them automatically.

That change is a small but important step towards getting C++ modules support for BioDynaMo.

I just fixed the whitespace changes. Is this PR ready to merge?

cmake/FindROOT.cmake Outdated Show resolved Hide resolved
@vgvassilev
Copy link

Looks good to me!

@TobiasDuswald
Copy link
Contributor

@imorlxs @vgvassilev can you comment if and how that affects the codebase or it's compilation? Does it compile identically, e.g., is genreflex a deprecated command that we now replace with cling or what's the difference now? Would be great if you could elaborate on that such that we can understand that contribution better and have it documented here.

Also @imorlxs, the call to cling is once

COMMAND ${ROOTCLING_EXECUTABLE} -reflex -f ${gensrcdict} ${rootmapopts} ${headerfiles} ${selectionfile} -noIncludePaths -inlineInputHeader

and once

rootcling -reflex -f ${DICT} ${HEADERS} --noIncludePaths -inlineInputHeader ${SELECTIONFILE} ${CXX_DEFINES} ${CXX_INCLUDES} $ADDITIONAL_CXX_FLAGS

I noticed that the -inlineInputHeader flag occurs at two different position. I'm not sure if It treats what follows as argument or not? I had issues with the selection files in the past and they gave me a headache. I want to be sure that we treat them the right way. Same goes for --nonIncludePaths and -nonIncludePaths, respectively. If the order makes no difference, I think it would still be good to change the commands such that the arguments occur in the same order; I have a slight preference for the former if they are identical.

@vgvassilev
Copy link

@imorlxs @vgvassilev can you comment if and how that affects the codebase or it's compilation? Does it compile identically, e.g., is genreflex a deprecated command that we now replace with cling or what's the difference now? Would be great if you could elaborate on that such that we can understand that contribution better and have it documented here.

genreflex is a hard link to rootcling but with less capabilities. That is, genreflex is a driver for rootcling but does not support all flags that we need to enable C++ modules for BDM. That's the motivation to move, moreover, genreflex is in practice deprecated :)

@imorlxs
Copy link
Member Author

imorlxs commented Aug 7, 2024

@imorlxs @vgvassilev can you comment if and how that affects the codebase or it's compilation? Does it compile identically, e.g., is genreflex a deprecated command that we now replace with cling or what's the difference now? Would be great if you could elaborate on that such that we can understand that contribution better and have it documented here.

Also @imorlxs, the call to cling is once

COMMAND ${ROOTCLING_EXECUTABLE} -reflex -f ${gensrcdict} ${rootmapopts} ${headerfiles} ${selectionfile} -noIncludePaths -inlineInputHeader

and once

rootcling -reflex -f ${DICT} ${HEADERS} --noIncludePaths -inlineInputHeader ${SELECTIONFILE} ${CXX_DEFINES} ${CXX_INCLUDES} $ADDITIONAL_CXX_FLAGS

I noticed that the -inlineInputHeader flag occurs at two different position. I'm not sure if It treats what follows as argument or not? I had issues with the selection files in the past and they gave me a headache. I want to be sure that we treat them the right way. Same goes for --nonIncludePaths and -nonIncludePaths, respectively. If the order makes no difference, I think it would still be good to change the commands such that the arguments occur in the same order; I have a slight preference for the former if they are identical.

As @vgvassilev said, since ROOTv6. genreflex is nothing but a wrapper around rootcling 1. Genreflex internally uses rootcling, and the rootcling call can be showed with genreflex -debug or the new --print-rootcling-invocation 2 That's how I figured out the arguments that we need to migrate to rootcling. It should compile basically the same, and don't affect the codebase.

The flags can be in any position because the way the ROOT parser works, and can also be with one minus or two. I made a commit fixing it and putting the two call the same way.

Thank you for the feedback!

@imorlxs imorlxs marked this pull request as draft August 19, 2024 17:23
@imorlxs imorlxs marked this pull request as ready for review August 19, 2024 17:23
.editorconfig Outdated Show resolved Hide resolved
Copy link

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

Successfully merging this pull request may close these issues.

3 participants