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

Hide include directive from ROOT5 interpreter #456

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

klendathu2k
Copy link
Contributor

@klendathu2k klendathu2k commented Dec 5, 2022

Proposed fix for Issue #455

It is not clear to me from the tests I've done on issue #455 why StBFChain/StBFChain.h is not located in the include path by root5 / cint. Simplest fix is to hide the include directive from CINT.

@klendathu2k klendathu2k requested review from plexoos and genevb December 5, 2022 18:51
@plexoos
Copy link
Member

plexoos commented Dec 5, 2022

There is no reason to hide the header from ROOT5/CINT and the CI tests confirm just that. The reason why ROOT5/cint can't find the header is because ROOT5 is not given the paths to search for it in interpreter mode. It is done by using gInterpreter->AddIncludePath() as in #441 . I don't really understand the resistance to update the rootlogon.C macro to include those lines... I think the same effect can be achieved if one adds these calls directly to the bfc.C macro before calling gSystem->ProcessLine()...

My point is that we should minimize the branching of the code for ROOT5 and ROOT6 and use #ifdefs only if nothing else works.

@klendathu2k
Copy link
Contributor Author

Agreed. I had not appreciated that the gInterpreter -> AddIncludePath(...) function only accepted one path at a time. Will close this PR.

@plexoos plexoos reopened this Dec 20, 2022
@plexoos plexoos force-pushed the star-fix-for-issue445 branch from 13050ee to 91c8ed7 Compare December 20, 2022 15:31
@plexoos plexoos changed the title Proposed fix for Issue #455 Hide include statement from ROOT5 interpreter Dec 20, 2022
Copy link
Member

@plexoos plexoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having no control over the system-wide root config files, this seems like a viable solution (at least for now)

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK to me.

@plexoos plexoos changed the title Hide include statement from ROOT5 interpreter Hide include directive from ROOT5 interpreter Dec 20, 2022
@plexoos plexoos merged commit e35ae1d into star-bnl:main Dec 20, 2022
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.

3 participants