BioDynaMo
Replace genreflex in favor of rootcling
here we'd probably need you to check if this is all fine @vgvassilev .
Thanks @imorlxs 🚀
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.
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?
@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.
@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 @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 -inlineInputHeaderand once
rootcling -reflex -f ${DICT} ${HEADERS} --noIncludePaths -inlineInputHeader ${SELECTIONFILE} ${CXX_DEFINES} ${CXX_INCLUDES} $ADDITIONAL_CXX_FLAGSI noticed that the
-inlineInputHeaderflag 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--nonIncludePathsand-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!
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
PR appears to affect [ FAILED ] SchedulerTest.Backup on macOS.
Cannot reproduce failure on MacBook Pro, M3 pro, Sequoia 15.3.1, Xcode 15.4 .
➡️ won't merge until conflict is resolved
@imorlxs, I am sorry this comment comes a bit late from my side; please clarify things about this PR (purpose, aim, what it solves, etc.). Thanks
Hi @vasvav. The purpose of this PR is to replace genreflex. As @vgvassilev said, since ROOTv6. genreflex is nothing but a wrapper around rootcling 1
Genreflex doesn't have the options to create a module based dictionary, and is essentially deprecated 2
This PR should not affect the codebase, as it only replace the hardlink to the actually rootcling call. The aim is to be able to use C++ Modules in BDM (#385 is a GSoC project that I would love to resume this summer, but as an independent developer)
Previously advised not to merge because of stochastic failure of [ FAILED ] SchedulerTest.Backup. Saw this in another PR now so this issue is (as expected) not related to this PR and can be merged!
Thanks @imorlxs