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.

Sign up for GitHub

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

Jump to bottom

main: Add informative message what do to if RecurrsionError occurs. #5156

Merged
merged 1 commit into from
Oct 18, 2020
Merged

main: Add informative message what do to if RecurrsionError occurs. #5156

merged 1 commit into from
Oct 18, 2020

Conversation

htgoebel
Copy link
Member

See #4406

Legorooj
Legorooj approved these changes Sep 11, 2020
View reviewed changes
Copy link
Member

@Legorooj Legorooj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work! EDIT: see below

@Legorooj
Copy link
Member

However, the new test fails - can you fix this? Please also fix the lint error as well.

@htgoebel
Copy link
Member Author

Interesting to see the recursion on Python 3.6 is deeper than on he other ones.

@htgoebel htgoebel force-pushed the issue-4406-recursion-message branch 2 times, most recently from 1b0aeb4 to 1bffafb Compare September 11, 2020 13:51
@Legorooj
Copy link
Member

Legorooj commented Sep 12, 2020 •
edited
Loading

@htgoebel can you rebase off develop? That should fix the test failures. And yes, it is interesting.

@htgoebel
Copy link
Member Author

@Legorooj Will do, but I'm curious: Current develop head only adds "hooks: Stop hooks collecting bytecode files". What is the reason why you expect this to stop the worker-crashes?

@Legorooj
Copy link
Member

Current development head also reverts your accidental commit AFAIK - it worked for other PRs with the same issue (I think it's the same issue anyway)

Legorooj
Legorooj requested changes Sep 13, 2020
View reviewed changes
Copy link
Member

@Legorooj Legorooj left a comment

Choose a reason for hiding this comment

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

Just one little thing: can you please fix the following filename:

+ _recursion_too_deep_message.py
- _recursion_to_deep_message.py

@htgoebel htgoebel force-pushed the issue-4406-recursion-message branch 2 times, most recently from 971907c to 29b9861 Compare September 13, 2020 11:27
@htgoebel
Copy link
Member Author

The tests for this case are inconsistently failing on Win-64 Py 3.7 and 3.8 only: The respective worker is crashing. But this does not happen if only running these tests, see https://ci.appveyor.com/project/htgoebel/pyinstaller/builds/35176564. Since I have not time for debugging CI platform annoyances, I'll just remove the tests. @Legorooj Maybe you want to keep them as a base for tests when moving to modulegraph2.

@Legorooj
Copy link
Member

@htgoebel I did some digging and it looks like this is an issue with pytest-xdist's memory handling. I've applied the same fix that SciPy did so we'll merge if that works, and if not, keep digging.

@htgoebel htgoebel added this to the PyInstaller 4.1 milestone Sep 17, 2020
@htgoebel
Copy link
Member Author

Since the pinning pytest-xdist did not help, I'm going to remove these test.

@htgoebel
Copy link
Member Author

Or better: I'm xfailing them for Wondoze.

Legorooj
Legorooj requested changes Oct 12, 2020
View reviewed changes
tests/unit/test_recursion_limit.py
Comment on lines +82 to +83
with pytest.raises(SystemExit) as execinfo:
pyi_main.run(pyi_args, PYI_CONFIG)
Copy link
Member

Choose a reason for hiding this comment

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

This clause isn't catching the RecursionError for some reason...

@htgoebel htgoebel merged commit c83edbf into pyinstaller:develop Oct 18, 2020
@htgoebel htgoebel deleted the issue-4406-recursion-message branch October 18, 2020 19:41
@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.
Reviewers

@Legorooj Legorooj Legorooj requested changes

@bjones1 bjones1 bjones1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
PyInstaller 4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@htgoebel @Legorooj @bjones1