Make wx dependency optional for CLI usage #532
Conversation
meawoppl
force-pushed
the
meawoppl/optional-wx-dep
branch
2 times, most recently
from
64d3670 to
2d0397a
Compare
There was a problem hiding this comment.
I am honestly a bit confused on how this line of code works now that I look at it. I am not sure how this imports resolves... regardless, it triggers import of the __init__ here I think:
https://github.com/openscopeproject/InteractiveHtmlBom/blob/master/InteractiveHtmlBom/__init__.py
Would you prefer I replace the imports there with the get_wx() singleton or move the import?
There was a problem hiding this comment.
This file is not part of the package, it exists solely for convenience where you can just download the repo and drop it as a whole in kicad's plugin dir. KiCad's plugin system will run this init file and the import initializes kicad plugin, that's all it does. This file is irrelevant for cli use case so just leave it unchanged.
There was a problem hiding this comment.
This makes sense wrt. to the existance of the plugin import, that said, that import will still trigger the __init__.py one folder up so would you prefer I replace the imports there with the get_wx() singleton we add here?
There was a problem hiding this comment.
It doesn't matter what it triggers, this code does not run when you use ibom in cli mode. Leave it unchanged.
one folder up
This is the topmost level init, there is no one folder up. When you run cli it only uses inner folder as a package, this top level folder may as well not exist.
meawoppl
force-pushed
the
meawoppl/optional-wx-dep
branch
from
2d0397a to
7c796fe
Compare
| app = wx.App() | ||
| if hasattr(wx, "APP_ASSERT_SUPPRESS"): | ||
| app.SetAssertMode(wx.APP_ASSERT_SUPPRESS) | ||
| elif hasattr(wx, "DisableAsserts"): |
There was a problem hiding this comment.
This is a bit confusing as this would nominally only run if not creating a wx app, correct?
I feel like we shouldn't have to disable asserts if not even importing wx...
I can move it into the if create_wx_app block, but I think that would be a behavior change. What is this line of code really about?
There was a problem hiding this comment.
Kicad's code sometimes throws GUI asserts requiring user input to close even when wx app is not created. This line suppresses that so that the app does not crash or get blocked without a desktop environment.
Add get_wx() in compat.py to gate wx imports behind INTERACTIVE_HTML_BOM_NO_DISPLAY environment variable.
meawoppl
force-pushed
the
meawoppl/optional-wx-dep
branch
from
7c796fe to
db509db
Compare
|
I think I got everything, lmk if you want anything else changed/tweaked. |
qu1ck
left a comment
qu1ck
left a comment
There was a problem hiding this comment.
Tested it and it works fine except when wx is not available the error is cryptic.
| from .compat import get_wx, should_create_wx_app | ||
| wx = get_wx() | ||
| create_wx_app = should_create_wx_app() | ||
|
|
There was a problem hiding this comment.
Add a check here if wx is None and create_wx_app is true then show an error "wxpython is required unless INTERACTIVE_HTML_BOM_NO_DISPLAY environment variable is set" and exit early.
2 participants
wxPython is kinda annoying/needy to install, and the way the imports are currently make it tough to use headless w/o installing a bunch of extra stuff. This PR uses an existing environment variable to gate the import of
wxto areas of the code that actually use GUI elements.Quick summary of changes:
get_wx()incompat.pyto gate wx imports behindINTERACTIVE_HTML_BOM_NO_DISPLAYenvironment variable--show-dialogstill require wxUsage