macOS build configuration for pyinstaller with associated helper assets #111
Conversation
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
==========================================
- Coverage 86.5% 86.48% -0.03%
==========================================
Files 16 16
Lines 1223 1221 -2
Branches 138 138
==========================================
- Hits 1058 1056 -2
Misses 136 136
Partials 29 29
Continue to review full report at Codecov.
|
|
I give up. Either appveyor works and codecov fails or vice-versa. Too much time wasted for a simple commit. |
|
@michaelgale I sometimes ignore codecov if I think it's being too picky. I think your PR is fine. We can merge it even with codecov complaining. |
|
@jmwright Cool. To be honest, this is my first encounter with code coverage and I must say it was annoying! There is something psychologically annoying about the last red X due to a 0.03% difference in coverage! FYI, I did check both building and running the macOS app bundle in macOS 10.14 Mojave and 10.15 Catalina and they both work. One last Q: is there any value in adding a macOS CI auto build and test to the project? |
|
I think so, if it can be done without a large amount of effort. However, @adam-urbanczyk is the primary maintainer of this project. I'd want to make sure he's ok with it. |
|
@michaelgale thanks for all the work! Codecov is not a problem for merging this PR. I will have some review comments though. I'd skip for now automatic building (can be always added later, but preferably as an azure pipeline). I'm trying to setup CI for Mac here: #101 - help will be welcome. |
| oce_dir = Path(sys.prefix) / "Library" / "share" / "oce" | ||
|
|
||
|
|
||
| a = Analysis( |
There was a problem hiding this comment.
What's with the formatting change? I think only one line is actually edited.
There was a problem hiding this comment.
The entire pyinstaller.spec file was accidentally reformatted with black. Therefore, spacing and single-quote substitution was applied globally. Since it had no semantic impact, I left it. In the line above, I simply added the check for sys.platform=="darwin"
| exe, a.binaries, a.zipfiles, a.datas, strip=False, upx=True, name="CQ-Editor" | ||
| ) | ||
|
|
||
| app = BUNDLE( |
There was a problem hiding this comment.
I did build on Win and Linux without this section. Shouldn't it be behind an if?
There was a problem hiding this comment.
From what I understand, the "BUNDLE" component will not be processed on linux/windows anyway. Guarding with an "if" is likely not necessary, but wouldn't hurt--everything still builds the same in macOS with or without the if guard.
There was a problem hiding this comment.
Indeed, but still it will be more readable.
There was a problem hiding this comment.
No problem, I'll put a platform 'if' guard for macOS to enhance readability.
| # It introspects this program's absolute path and appends the | ||
| # relative path discovered from the environment. | ||
|
|
||
| if sys.platform == 'darwin': |
There was a problem hiding this comment.
this should probably go to the runtime hook , also do use Path i.s.o. manual additions.
There was a problem hiding this comment.
What is the context of the runtime hook (i.e. where is it injected at runtime?) The code in run.py is able to introspect its path and then apply the substitution since it knows the relative location of CASROOT etc. I'm not sure if this introspection will work in pyi_rth_occ?
There was a problem hiding this comment.
Here is some info regarding runtime hooks in pyinstaller: https://pythonhosted.org/PyInstaller/when-things-go-wrong.html#changing-runtime-behavior
Note that the environment variables you are manipulating are set here
https://github.com/CadQuery/CQ-editor/blob/1cee3f4ff90b13e1dbc54bfa22e9286c3e05b10e/pyinstaller/pyi_rth_occ.py
So for me it is quite logical to handle such things in the original file. I also noticed that you are deleting CASROOT in an else branch. I'd prefer not to change any behavior regarding other platforms.
There was a problem hiding this comment.
Thanks for the references, I will check them out and work out some changes using rth.
Deleting CASROOT was simply retaining the existing behaviour before my PR. I wasn't sure what the purpose of the statement was, but I left the logic in place so that it would work the same way.
There was a problem hiding this comment.
Indeed, my bad. CASROOT delete is related to this #9
There was a problem hiding this comment.
Regarding RTH: you could just expand the relative to absolute paths here:
CQ-editor/pyinstaller/pyi_rth_occ.py
Line 5 in 1cee3f4
e.g. use something like (after doing from path import Path)
env['CSF_ShadersDirectory'] = Path('oce/src/Shaders' ).abspath()
env['CSF_UnitsLexicon'] = Path('oce/src/UnitsAPI/Lexi_Expr.dat').abspath()
env['CSF_UnitsDefinition'] = Path('oce/src/UnitsAPI/Units.dat').abspath()There was a problem hiding this comment.
I have already implemented roughly the same in the rth and it works well. My next commit will include this change.
| @@ -46,8 +46,11 @@ def __init__(self,parent=None): | |||
|
|
|||
| self.prepare_statusbar() | |||
| self.prepare_actions() | |||
|
|
|||
| self.components['object_tree'].addLines() | |||
|
|
|||
There was a problem hiding this comment.
The application (without bundling) did work on Mac (according to some users). So I'd rather not remove this functionality but rather fix the graphics initialization process
There was a problem hiding this comment.
If I leave this statement, the resulting app binary is guaranteed to crash (verified in macOS 10.14 and 10.15). We can either defer this pull request until a root cause is found or leave the 'if platform' guard in place until a root cause is found for future releases. Unless there is some urgency to make a new release including macOS, we can always defer the PR and wait until the root cause / remedy to this race condition crash at startup.
There was a problem hiding this comment.
Actually if you are willing to help, I have some ideas regarding debugging this.
There was a problem hiding this comment.
Sounds good. I did spend a few hours yesterday trying to get more clues. One issue is the timing when “InitDriver” is called relative to MainWindow.show(). The rest of the issue is related to abi trap from AIS interactive viewer. I think it tries to throw an exception and it doesn’t get handled properly.
There was a problem hiding this comment.
@adam-urbanczyk let me know if you want to proceed with some macOS debugging of this issue--I would like to help if I can. BTW, good luck with the FOSDEM talk--I just checked out your slide deck!
|
In the meantime, I'll have a look at your CI build pipeline to see if I can help with the CI. |
New macOS platform specific assets and configuration are contained in a new folder "macos". The PyInstaller.spec file was accidentally reformatted with "black"--don't panic, despite the sea of red diff's only a handful of changes were made: i) new app=BUNDLE section at the end and ii) additional platform check at the beginning for the oce library path.