[Python] Don't import cppyy in ROOT facade module #21091
Merged
+138
−160
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The setup of the ROOT module roughly works like this: 1. The ROOT module imports `cppyy` modules 2. It then creates the ROOT Facade, which keeps a handle to the original ROOT module 3. It replaces itself with the facade The facade module also import cppyy. In the future, we want to have `cppyy` as an internal submodule of `ROOT` to separate the ROOT package cleanly from other cppyy packages. That means, the facade module will have to import something like `ROOT._cppyy`, which implies the import of the ROOT module, which imports the facade, and that's infinite recursion. So one prerequisite for moving the `cppyy` module inside `ROOT` is that the facade module doesn't import `cppyy` in its global scope, which is implemented in this commit. Fortunately, this can easily be avoided by re-using the handle to the cppyy module that `ROOT/__init__.py` has originally imported, which is set as an attribute of the facade. This commit also takes the opportunity to fixup one design flaw in e9fe075: the handle to the original ROOT module in the facade should not be public, because this is an implementation detail of the was the ROOT module is constructed. So essentially, the `ROOT.module` attribute is replaced with a new `ROOT._cppyy` attribute.
In the future, we don't want to rely on ROOT also shipping the `cppyy` package. It should be a ROOT implementation detail instead.
guitargeek
force-pushed
the
root_facade
branch
3 times, most recently
from
4ccd44d to
1fa26bd
Compare
guitargeek
force-pushed
the
root_facade
branch
from
1fa26bd to
d8c87c5
Compare
Test Results 22 files 22 suites 3d 7h 53m 51s ⏱️ Results for commit d8c87c5. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The setup of the ROOT module roughly works like this:
cppyymodulesROOT module
The facade module also import cppyy.
In the future, we want to have
cppyyas an internal submodule ofROOTto separate the ROOT package cleanly from other cppyy packages.That means, the facade module will have to import something like
ROOT._cppyy, which implies the import of the ROOT module, whichimports the facade, and that's infinite recursion.
So one prerequisite for moving the
cppyymodule insideROOTis thatthe facade module doesn't import
cppyyin its global scope, which isimplemented in this commit.
Fortunately, this can easily be avoided by re-using the handle to the
cppyy module that
ROOT/__init__.pyhas originally imported, which isset as an attribute of the facade.
This commit also takes the opportunity to fixup one design flaw in
e9fe075: the handle to the original ROOT module in the facade
should not be public, because this is an implementation detail of the
was the ROOT module is constructed.
So essentially, the
ROOT.moduleattribute is replaced with a newROOT._cppyyattribute.