Skip to content

fix test_typed_pkg test failure on Mac #4888

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

Merged
merged 2 commits into from
Apr 11, 2018

Conversation

JelleZijlstra
Copy link
Member

Fixes #4883

The check for whether we're in a venv turns out to be unreliable. See https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv for background.

Note that if you're testing this on a Mac that previously ran into the bug, you'll have to manually remove any global typedpkg installs for the test to work.

if hasattr(sys, 'real_prefix'):
return True
else:
# https://github.com/python/typeshed/pull/2047
Copy link
Member

Choose a reason for hiding this comment

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

I merged that typeshed PR, but yet another typeshed sync is also needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was expecting that we'd just merge this to get mypy master back in a good state. I can send another small PR to remove the type ignore later.

Copy link
Member

Choose a reason for hiding this comment

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

(Actually I propose not to cp that onto the release branch, since the # type: ignore below silences the error. We can fix things on the master branch.)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I'll let @ethanhs review and merge onto master. Once merged I'll cherry-pick this PR into the release-0.590 branch.

if hasattr(sys, 'real_prefix'):
return True
else:
# https://github.com/python/typeshed/pull/2047
Copy link
Member

Choose a reason for hiding this comment

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

(Actually I propose not to cp that onto the release branch, since the # type: ignore below silences the error. We can fix things on the master branch.)

Copy link
Member

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix.

@emmatyping emmatyping merged commit 88ee411 into python:master Apr 11, 2018
@gvanrossum
Copy link
Member

gvanrossum commented Apr 11, 2018 via email

@emmatyping
Copy link
Member

So apparently the repro was to run the test twice...

Well, run the test twice in an environment where the global/user site packages directory was available from a virtualenv. In all of my testing environments, the tests could be run twice without failure.

I'm a little sad this test uses and modifies the environment.

Yes, it is less than ideal. I am working on a version that creates, uses, then destroys a virtualenv of its own.

@JelleZijlstra JelleZijlstra deleted the venvcheck branch April 11, 2018 23:10
@JelleZijlstra
Copy link
Member Author

I am working on a version that creates, uses, then destroys a virtualenv of its own.

That might also be tricky, because on Python 2 you need to install the third-party virtualenv library to even create a venv, so your test might still need to do things to the global env in order to even make a venv.

@emmatyping
Copy link
Member

That might also be tricky, because on Python 2 you need to install the third-party virtualenv library to even create a venv, so your test might still need to do things to the global env in order to even make a venv.

I mean our Python 2 tests already depend on it having typing installed. We can add virtualenv as a requirement on top of that.

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Apr 11, 2018
@gvanrossum
Copy link
Member

I cherry-picked this into release-0.590, but not #4884 (which you created to debug this). And I'm not going to cp the typeshed change for sys.base*prefix, nor hurry with another typeshed sync.

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.

3 participants