Skip to content

capture: recover from closed files #2633

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

Closed
wants to merge 6 commits into from
Closed

capture: recover from closed files #2633

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 30, 2017

works around pytest-dev/pytest-xdist#167. This is not however a "fix" but simply allows the tests to run.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 91.992% when pulling 36d1d8c on xoviat:capture into 5e0e038 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 91.995% when pulling d0a032e on xoviat:capture into 5e0e038 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 91.975% when pulling bbab72b on xoviat:capture into 5e0e038 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.962% when pulling c007b2b on xoviat:capture into 5e0e038 on pytest-dev:master.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks!

Please add a new changelog entry and a test

@ghost
Copy link
Author

ghost commented Jul 30, 2017

Unfortunately the only place I can reproduce this is on the pyinstaller test suite on py36-win_amd64. For some reason stderr capturing fails resulting in verbose test output and invalid sys.stderr.fileno().

@nicoddemus
Copy link
Member

I see, thanks.

Perhaps you can forcibly close the captured file handle inside a test and ensure it doesn't blow up?

@nicoddemus
Copy link
Member

Took the liberty of refactoring the test into a subtest. 😁

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.963% when pulling e8ef67a on xoviat:capture into 5e0e038 on pytest-dev:master.

@@ -388,17 +396,23 @@ def done(self):
seeked to position zero. """
targetfd_save = self.__dict__.pop("targetfd_save")
os.dup2(targetfd_save, self.targetfd)
os.close(targetfd_save)
# os.close(targetfd_save)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this code commented out?

@@ -1159,3 +1159,6 @@ def test_pickling_and_unpickling_enocded_file():
ef = capture.EncodedFile(None, None)
ef_as_str = pickle.dumps(ef)
pickle.loads(ef_as_str)

def test_closed_handle():
Copy link
Member

Choose a reason for hiding this comment

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

I see you overwrote my update to this test, was that deliberate?

Copy link
Author

Choose a reason for hiding this comment

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

@nicoddemus I am sorry about this; I thought that this had already been merged so I could trash the branch.

@ghost
Copy link
Author

ghost commented Jul 31, 2017

@nicoddemus I have addressed the root cause of these problems with PyInstaller so I do not think that it is worth the time to try to fix this PR. I am sorry for any wasted time.

@ghost ghost closed this Jul 31, 2017
@ghost ghost deleted the capture branch July 31, 2017 19:43
@nicoddemus
Copy link
Member

@xoviat OK then, much thanks anyway for the contribution! 👍

@RonnyPfannschmidt
Copy link
Member

@xoviat for documentation purposes, can you please link the solution, im curious

@ghost
Copy link
Author

ghost commented Aug 1, 2017

See here: https://github.com/pyinstaller/pyinstaller/pull/2713/files#diff-f1efaa62975ecd8783846ec9799512a7R348

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants