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

Fix crash when sys.stdin is None #7118

Merged
merged 8 commits into from Oct 2, 2019
Merged

Fix crash when sys.stdin is None #7118

merged 8 commits into from Oct 2, 2019

Conversation

emilburzo
Copy link
Contributor

@emilburzo emilburzo commented Sep 30, 2019

Fixes #7119


This happens especially on AWS Lambda.

(I did not add a news entry because it feels like a trivial change)

@chrahunt
Copy link
Member

@chrahunt chrahunt commented Sep 30, 2019

Hi @emilburzo!

Can you please create an issue instead, providing the relevant details and ideally a way to easily reproduce this? As it stands it's very difficult to evaluate this change, and without understanding or tests it would be even harder to prevent a regression in the future.

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 30, 2019

This was created in #7119.

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 30, 2019

(I did not add a news entry because it feels like a trivial change)

Could you please add news/7119.bugfix and news/7118.bugfix, both with the same contents:

Fix a crash when ``sys.stdin`` is set to ``None``, such as on AWS Lambda.

@chrahunt
Copy link
Member

@chrahunt chrahunt commented Sep 30, 2019

Can we take the approach mentioned here, pulling the interactivity check into a separate function that can be tested in isolation?

@emilburzo
Copy link
Contributor Author

@emilburzo emilburzo commented Oct 1, 2019

Can we take the approach mentioned here, pulling the interactivity check into a separate function that can be tested in isolation?

I'm new to the project code layout, but I can give it a go. :)

I have one uncertainty though: do we want to have the new interactivity check function in the Subversion class?

Or, because it's so generic, would it be better placed somewhere in a "higher" class? (where?)

@chrahunt
Copy link
Member

@chrahunt chrahunt commented Oct 1, 2019

Thanks. :)

I would put it in pip._internal.utils.misc.

@emilburzo
Copy link
Contributor Author

@emilburzo emilburzo commented Oct 1, 2019

I extracted the function, feedback on naming things is very welcomed :)

Is testing this feasible?

I guess we could forcefully set sys.stdin to None, but I'm not sure about the impact.

Any thoughts?

@chrahunt
Copy link
Member

@chrahunt chrahunt commented Oct 1, 2019

Hi @emilburzo!

Thanks for the update. is_console_interactive seems like a fine name to me. I think this is definitely testable.

I would add a helper function and two tests in tests/unit/test_utils.py (probably at the bottom).

The function would take a monkeypatch argument and do something like monkeypatch.setattr(sys.stdin, 'isatty', Mock(return_value=True)), this ensures that the behavior is consistent no matter if we're running locally or in CI.

The first test would call the helper function then make sure that is_console_interactive returns True.

The second (maybe test_is_console_interactive_when_stdin_is_None?) would call the helper function, then monkeypatch.setattr(sys, 'stdin', None), then check that is_console_interactive returns False.

That ensures that we're always testing the situation that would succeed except for the other changes we make for the failure test cases - this will help a lot with the other change mentioned in #7042 later.

tests/unit/test_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@chrahunt chrahunt left a comment

Assuming everything passes, this looks good to me. Thanks for your work on this @emilburzo!

@emilburzo
Copy link
Contributor Author

@emilburzo emilburzo commented Oct 2, 2019

and thank you @chrahunt for the hand-holding, it was very helpful!

@pradyunsg pradyunsg changed the title fix crash when sys.stdin is None Fix crash when sys.stdin is None Oct 2, 2019
@pradyunsg pradyunsg merged commit e6f69fa into pypa:master Oct 2, 2019
24 checks passed
@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Oct 2, 2019

Hurrah!

Thanks @emilburzo for the PR and @chrahunt for all the help and reviews. :)

@emilburzo -- do let us know if you're interested in contributing further. There's a lot of places in pip where we could use some help! ^>^

@emilburzo emilburzo deleted the patch-1 branch Oct 2, 2019
@emilburzo
Copy link
Contributor Author

@emilburzo emilburzo commented Oct 2, 2019

@pradyunsg no promises, but I'd at least be interested to see what other things you need help with, maybe there's something I can help with.

@lock lock bot added the auto-locked label Nov 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants