-
Notifications
You must be signed in to change notification settings - Fork 998
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
Fix github action for building macos app #1117
Conversation
…SX. Hopefully fixes #1113.
@a2k-hanlon can you test the builds that get generated here and see if this resolves the problems you reported in #1113 ? |
Ok, I tried running the build for this PR. I also tried implementing what you just added in my local build process. The app generated from Github still throws a "The application “pronterface.app” can’t be opened." The binary file throws this error instead now (a wxpython thing perhaps?):
When I tried building locally with your changes, I can start the app, though it seems iffy about starting up - the app icon in the dock bounces, then disappears, then it comes back and the program window appears. Plus, the app defaults to the 2D view; the 3D view doesn't work. I noticed in the Github log that |
@a2k-hanlon thanks for checking, this is super useful! The hidden import is only present of specific versions of setuptools - you have one of those versions, the github builder had one when 2.0.0rc7 was built but has since been upgraded to one that no longer has it. Your local setup has an affected version, so the warning goes away on your machine but is still there on github. This is normal and expected. The missing windows stuff is also as expected. I'll add a commit that hopefully fixes the framework build thing. |
Ok, interesting. The github builder has setuptools 50.3.0, while I have 46.1.3. So you're saying if I upgraded my version of setuptools, I would have this issue too? If so, is there a workaround? By missing windows, do you mean the missing 3D view? |
No I mean the windows-specific dlls that are missing. And yes, setuptools versions between 45 and 49 have this incorrectly specified import. All other versions will show a warning (but still work). Affected versions will not show a warning - it's a version-specific workaround to a now-fixed bug in setuptools |
…ork build of python on macos
Ah, ok I follow. Thank you for clarifying! |
I'm still getting
after you added |
Well damn. I am out of my depth here - you're saying it works on your local build though? |
Oh, I think I know what's happening - it's this the github actions python is a useless non-framework build. I'll see what I can do |
If this works we will somehow need to apply the same fix to the wheel builder - it's not ideal because it downloads an explicit version of python |
@a2k-hanlon can you try the latest .app build here? It should be using a framework build |
Hm, still getting the "The application “pronterface.app” can’t be opened." with the github build app. The binary file throws this now... ugh.
|
@a2k-hanlon Sigh - I don't know what to do. Sorry. |
@a2k-hanlon in desperation I built a framework build without a venv - I don't expect it to make any difference but can you please try it? |
Ok. I GOT IT TO WORK. But, not your last commit. I will explain. I stumbled across a different issue with these github builds. I think this was the actual issue here, not a framework build, nor those pyinstaller warnings (though it may have been good to fix those warnings anyways). The issue is, the binary file WITHIN the generated application bundle does not have execute permissions! I ran Now the interesting part: I had to go back to the generated app from commit 1c4a0dd to find a working app. After I made this change to the generated apps from your more recent commits here, none of the generated apps from your commits AFTER 1c4a0dd ran. We still have some issues to sort out though. I tried connecting to my printer and starting a print with the generated app, and got this:
so it seems my suggestion earlier of tweaking the ctypes call to load IOKit was not a successful suggestion. In fact, when I run pronterface from source with that code change, I have the same issue. I didn't have that IOKit issue running from source before that change, so if we revert it, there won't be this problem running from source but we still need to sort it out for the precompiled app. |
Something else I learned about these generated macOS apps - in order for macOS to render everything at full resolution on a Retina display, you have to add a few lines to the |
Trying the generated app from 3d72c9a which is one commit back from 1c4a0dd (ie. before the change to the IOKit stuff), I did not get the IOKit-related warnings on print start, but I would still be suspicious about the computer power control working if pyinstaller threw a warning about not finding IOKit during that build. I need to try letting a print run for longer than a minute to see if that's really an issue. |
Revert "Explicit path to makespec and pyinstaller on macos app build" This reverts commit 98edf13. Revert "Try building with python.org python framework build but without venv" This reverts commit bd3fc54. Revert "Use venv for all actions for framework python" This reverts commit 5dfd622. Revert "Don't use github's broken non-framework python when building macos app" This reverts commit db8531d. Revert "pass --windowed to pyinstaller to see if that makes it use the framework build of python on macos" This reverts commit 6cc9309.
@a2k-hanlon can you try this one? |
It works, with no error about IOKit (haven't checked that sleep prevention is actually working; will need to do that when I have something to print). I still had to run |
I see you added the |
Good idea, I'll try that. Can also try zipping it myself rather than letting gh do it |
Success! Now the app can be run without too much trouble. Thanks for all your effort here @kliment! I'm sorry I sent you on such a journey with the framework python build and all that, only to find out that it was just github garbling the file permissions. If it's not too much to ask, while we're on the subject of this macOS binary it would be great if you could modify the build process to enable Retina resolution. All it takes is adding a couple lines to the generated pyinstaller spec file, in the BUNDLE properties (add the info_plist part):
Also, there's still the non-working 3D view, but I'm guessing that's a whole other issue. |
@a2k-hanlon No problem, and thank you so much for testing and helping! The spec file is currently automatically generated, so editing it is a bit involved - I guess I should just put the generated file in the repo and edit it there, which is exactly what I was trying to avoid. I'll look into it. The 3d view not working is probably also a build problem - can you try to track down which commit broke it? Does it work on your local build? |
Hi @kliment, I'm back. I'm so sorry I went MIA for so long... school became a headache last semester and I neglected to tie up the loose end here. I tinkered with the build based on this pull request, as well as based on the up to date master and I have been able to package a working app with a working 3D view. I have a few changes related to the macOS build that I could open a PR for, plus some info that's probably worth discussing in a new issue. As for this PR, I would say it can be merged at this point and I can work off of it with the things I've learned more recently. |
@a2k-hanlon Okay, thank you so much! I'm entirely out at sea when it comes of macos and you have been incredibly helpful. I'll merge this and wait for your PR. |
Hopefully fixes #1113.