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

Fix setting PYTHONPATH on Python 2 not working #1673

Merged
merged 6 commits into from
Mar 3, 2020
Merged

Fix setting PYTHONPATH on Python 2 not working #1673

merged 6 commits into from
Mar 3, 2020

Conversation

jd
Copy link
Contributor

@jd jd commented Feb 26, 2020 •
edited by gaborbernat
Loading

  • ensure we do not rewrite the PYTHONPATH
  • ensure we respect the -E flag.

@jd jd force-pushed the skip-pythonpath branch 2 times, most recently from 5781f4f to 997d515 Compare February 26, 2020 16:30
@jd jd mentioned this pull request Feb 26, 2020
Closed
@jd jd force-pushed the skip-pythonpath branch 2 times, most recently from 08db125 to 56fc536 Compare February 27, 2020 10:56
@jd
Copy link
Contributor Author

jd commented Feb 27, 2020

Is there anything left that needs fixing?

gaborbernat
gaborbernat requested changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Still needs tests and changelog entry 👍

src/virtualenv/create/via_global_ref/builtin/python2/site.py Outdated Show resolved Hide resolved
src/virtualenv/create/via_global_ref/builtin/python2/site.py Outdated Show resolved Hide resolved
@jd
Copy link
Contributor Author

jd commented Feb 28, 2020

Could you give me some hints about where to add some tests?
I took a look at #1641 but it's unclear to me how you added/changed the tests.

@gaborbernat
Copy link
Contributor

In that pr the test were adding brew Python to the ci matrix. Here you want a test inside test_create 👍set Python path, run create, check sys path of the debug property 👍

@jd
Copy link
Contributor Author

jd commented Mar 2, 2020

I'm blocked by #1683 to try locally to add tests :(

@jd jd force-pushed the skip-pythonpath branch 5 times, most recently from ab7c12a to 9268426 Compare March 2, 2020 14:07
@jd
Copy link
Contributor Author

jd commented Mar 2, 2020

Any idea how you set UTF-8 strings in a Windows env? 🤔

@pfmoore
Copy link
Member

pfmoore commented Mar 2, 2020

Environment variables are Unicode in Windows, so I'm not quite sure the question makes sense. Having said that, if this is Python 2, os.environ probably uses the ANSI APIs to read the environment, so the practical answer would likely be "you can't - you always get the default encoding".

@jd
Copy link
Contributor Author

jd commented Mar 2, 2020

My question is related to the CI failure. IIUC the dir contains Unicode chars and setting that as PYTHONPATH is a problem… :(

@pfmoore
Copy link
Member

pfmoore commented Mar 2, 2020

Ah, OK. So passing a general Unicode value through to the subprocess' environment. I don't think you can, in Python 2 (IIRC, you can only use characters that are valid in the system ANSI codepage) 🙁 (I may be wrong, it's a long while since I used Python 2).

Sorry I can't be more help.

gaborbernat
gaborbernat reviewed Mar 3, 2020
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
@gaborbernat gaborbernat force-pushed the skip-pythonpath branch 2 times, most recently from 1f48d16 to 90bf357 Compare March 3, 2020 12:40
gaborbernat
gaborbernat approved these changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@jd needed to do quite a few changes here, but I think now things should be OK. Can you validate with your system?

src/virtualenv/create/via_global_ref/builtin/python2/site.py Show resolved Hide resolved
src/virtualenv/create/via_global_ref/builtin/python2/site.py Show resolved Hide resolved
tests/unit/create/test_creator.py Show resolved Hide resolved
tests/unit/create/test_creator.py Show resolved Hide resolved
tests/unit/create/test_creator.py Show resolved Hide resolved
@gaborbernat gaborbernat changed the title Fix PYTHONPATH rewrite on Python 2 Fix setting PYTHONPATH on Python 2 not working Mar 3, 2020
@gaborbernat
Copy link
Contributor

@jd it's just how pythonpath is parsed at the interpreter level.

jd added 2 commits March 3, 2020 16:31
@jd @gaborbernat
feat(tests): add override_env_var fixture
8736dfb
@jd @gaborbernat
fix(python2): enhance prefix detection for paths
6e5eb84 
str.startswith() is a little bit too naive to check if a directories are
related.
jd and others added 4 commits March 3, 2020 16:31
@jd @gaborbernat
refactor: do not set sys.path if no rewrite happened
350f479
@jd @gaborbernat
refactor(python2/site): do not override PYTHONPATH
d34af4b
@gaborbernat
fix
e4be2ef 
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat
fix
763bbe7 
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat merged commit c310a95 into pypa:master Mar 3, 2020
@gaborbernat
Copy link
Contributor

Hello, a fix for this issue has been released via virtualenv 20.0.8; see https://pypi.org/project/virtualenv/20.0.8/ (https://virtualenv.pypa.io/en/latest/changelog.html#v20-0-8-2020-03-04). Please give a try and report back if your issue has not been addressed; if not, please comment here, and we'll reopen the ticket. We want to apologize for the inconvenience this has caused you and say thanks for having patience while we resolve the unexpected bugs with this new major release.
thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaborbernat gaborbernat gaborbernat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jd @gaborbernat @pfmoore