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

Bootloader build: Add traceback to debug pyiboot01 #4592

Merged
merged 1 commit into from May 2, 2020

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Dec 28, 2019

Adds the Python traceback information to debug errors during pyiboot01_bootstrap when running in Windows in windowed mode. This will allow debugging errors like in Issue #4213.

In order to get the full traceback, you need to use --debug=noarchive. I have tried to test this as much as I can for Python 2.7 and < Python 3.7 compatibility using a minimal example, but I don't have a full test app to duplicate pyiboot01_bootloader errors with.

Signed-off-by: Dan Yeaw dyeaw@ford.com

@htgoebel htgoebel added the area:bootloader Caused be or effecting the bootloader label Jan 4, 2020
@Legorooj Legorooj requested review from htgoebel and removed request for htgoebel April 18, 2020 06:57
@Legorooj
Copy link
Member

@danyeaw what's the status on this PR?

@danyeaw
Copy link
Contributor Author

danyeaw commented Apr 18, 2020

@Legorooj It is ready to be reviewed. I successfully used this to debug the referenced issue. It adds traceback information to the windowed error messages instead of just telling the user there is an error.

@Legorooj
Copy link
Member

@danyeaw this looks good. Could any of this be made better if you don't need py2 support becuase we have, after all dropped it? If not, I'll get a review from Hartmut; I'm not merging my weak spot (The C language) without another pair of eyes checking this out.

@htgoebel htgoebel force-pushed the modify-bootloader-clean branch 2 times, most recently from 61d98d4 to cd014cf Compare April 19, 2020 08:18
Copy link
Member

@htgoebel htgoebel left a comment

Choose a reason for hiding this comment

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

@danyeaw Thanks for this pull-request.I now found time for a review. I already removed the Python 2 stuff myself to speed up merging.
Anyway, I'm curious:

  • What only for Windows?
  • Why only for --no-console?
    Please also see the inline comment.

bootloader/src/pyi_launch.c Outdated Show resolved Hide resolved
@htgoebel
Copy link
Member

@Legorooj Please note: our policy is no never ever accept bootloaders executable build by somebody, as we can not verify the build. Bootloaders are always build be a core developer.

@htgoebel htgoebel self-assigned this Apr 19, 2020
@htgoebel htgoebel added this to the PyInstaller 4.0 milestone Apr 19, 2020
@Legorooj
Copy link
Member

Legorooj commented Apr 19, 2020

@htgoebel this needed a manual merge when I looked; I was going to ignore the new bootloader build.

So I shouldn't include a build in the py2-cleanup PR? I was going to for windows. I can verify it works - I have an env which builds the bootloader on change (with MSVC) for my issue replication environment.

@htgoebel
Copy link
Member

@Legorooj I already cleaned this up to get it merged soon. I'm jsut wondering why only Windows and only --no-console. This does not make any sense for me.

@Legorooj
Copy link
Member

Legorooj commented Apr 19, 2020

@htgoebel that is a little odd. @danyeaw should have a reason?

@danyeaw
Copy link
Contributor Author

danyeaw commented Apr 19, 2020

@htgoebel Thanks for reviewing the PR and for your questions 🎁

There are times when everything works great when there is a console, but fails in windowed mode with only a window that pops up to say there is an error. For example in your .spec file have options = [ ('v', None, 'OPTION') ] and set noconsole=True.

Normally if you have a console available, you do have the debug information you need because it can be displayed in the console.

@htgoebel
Copy link
Member

@danyeaw I now understood that we need to code to to get the traceback in --windowed mode. The reason is that in --windowed mode sys.stderr is NullWriter, which will will inhibit printing the traceback.

I still don't understand, this shall only be done on windows, and not on OSX, to.Is there a reason or is it just that you did not had OS X in mind here?

@htgoebel
Copy link
Member

htgoebel commented Apr 19, 2020

@danyeaw Regarding rebasing: I discovered magit (for emacs) a few month ago - very, very helpful. Maybe there is such a tool for vi too. (Or you give spacemacs or emacs doom a try, which offer vi-mode)

@danyeaw
Copy link
Contributor Author

danyeaw commented Apr 19, 2020

@htgoebel You are right, I didn't have macOS in mind. I think I was incorrectly assuming macOS would work more similar to Linux in windowed mode, sorry about that. Unfortunately, I don't have a mac to test this on.

Did you want me to rebase the PR, is that why you are discussing editors?

@danyeaw
Copy link
Contributor Author

danyeaw commented Apr 19, 2020

@htgoebel @Legorooj Thanks for your feedback. I removed the guard for Windows and rebased the PR.

@Legorooj
Copy link
Member

@danyeaw @htgoebel Travis is failing - on a exitcode of 0?

When running in Windows in windowed mode, the debug bootloader will now show
the Python traceback information. This help debuging errors during
pyiboot01_bootstrap. This will allow debugging errors like in Issue pyinstaller#4213.
For showing the source lines, the application needs to be frozen with ``--noarchive``.

Signed-off-by: Dan Yeaw <dyeaw@ford.com>
@htgoebel htgoebel merged commit 70709cd into pyinstaller:develop May 2, 2020
@htgoebel
Copy link
Member

htgoebel commented May 2, 2020

Thanks for the update. I only added some more details to the comments and merges teh pull-request. Thanks again for the pull-request and for helping to improve PyInstaller.

@danyeaw
Copy link
Contributor Author

danyeaw commented May 2, 2020

@htgoebel Thanks for merging this! 🎁

Legorooj added a commit to Legorooj/pyinstaller that referenced this pull request May 15, 2020
Legorooj added a commit to Legorooj/pyinstaller that referenced this pull request May 15, 2020
Legorooj added a commit that referenced this pull request May 16, 2020
… on macOS

* tests: unskip QtQml tests for macOS.
This reverts commit 6aebb5c.

* bootloader: Don't use the traceback added in #4592 when running on macOS
See #4860.
Legorooj added a commit to Legorooj/pyinstaller that referenced this pull request May 17, 2020
…QtQml tests on macOS

* tests: unskip QtQml tests for macOS.
This reverts commit 6aebb5c.

* bootloader: Don't use the traceback added in pyinstaller#4592 when running on macOS
See pyinstaller#4860.
cool-RR pushed a commit to cool-RR/pyinstaller that referenced this pull request Jun 20, 2020
…QtQml tests on macOS

* tests: unskip QtQml tests for macOS.
This reverts commit 6aebb5c.

* bootloader: Don't use the traceback added in pyinstaller#4592 when running on macOS
See pyinstaller#4860.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:bootloader Caused be or effecting the bootloader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants