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

With Git bash, cannot create virtualenv with a nested path. #1582

Closed
vphilippon opened this issue Feb 11, 2020 · 9 comments · Fixed by #1598
Closed

With Git bash, cannot create virtualenv with a nested path. #1582

vphilippon opened this issue Feb 11, 2020 · 9 comments · Fixed by #1598

Comments

@vphilippon
Copy link
Member

@vphilippon vphilippon commented Feb 11, 2020

With Git Bash, virtualenv refuses to create a virtualenv with a nested directory (ex: foo/bar), reporting that / is a path seperator and shouldn't be in the destination path.

I have no idea why / is considered a path separator (usually ; or :, depending on OS, right?)

  • Minimal reproducible example or detailed descriptions
  • OS and pip list output

Environment: Windows 10, with Git Bash, using Python 3.7.

$ py -3.7 -m pip list
Package            Version
------------------ -------
appdirs            1.4.3
distlib            0.3.0
filelock           3.0.12
importlib-metadata 1.5.0
pip                20.0.2
setuptools         45.2.0
six                1.14.0
virtualenv         20.0.2
zipp               2.2.0

Example:

$ py -3.7 -m virtualenv foo/bar
usage: virtualenv [--version] [--with-traceback] [-v | -q] [--discovery {builtin}] [-p py] [--creator {builtin,cpython3-win,venv}] [--seeder {app-data,pip}] [--no-seed] [--activators comma_separated_list] [--clear] [--system-site-packages]
                  [--symlinks | --copies] [--download | --no-download] [--extra-search-dir d [d ...]] [--pip version] [--setuptools version] [--wheel version] [--no-pip] [--no-setuptools] [--no-wheel] [--clear-app-data] [--symlink-app-data]

                  [--prompt prompt] [-h]
                  dest
virtualenv: error: argument dest: destination 'foo/bar' must not contain the path separator (/) as this would break the activation scripts

And FYI:

$ py -3.7
Python 3.7.6 (tags/v3.7.6:43364a7ae0, Dec 19 2019, 00:42:30) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.pathsep
';'
>>>
@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Feb 11, 2020

Does it work if you pass the windows path separator instead of the UNIX one you do in your example? I feel like the error message is bad, but seems you're trying to use Unix paths on windows.

@vphilippon

This comment has been minimized.

Copy link
Member Author

@vphilippon vphilippon commented Feb 11, 2020

Yes, it'll work:

$ py -3.7 -m virtualenv foo\\bar
created virtual environment CPython3Windows(dest=F:\rendezvous_local\foo\bar, clear=False, global=False) with seeder FromAppData pip=latest setuptools=latest wheel=latest app_data_dir=C:\Users\vphilippon\AppData\Local\pypa\virtualenv\seed-v1 via=copy

Although, "I didn't have issues before" ™️ , but also usually don't have issues doing so using Git Bash with other python application.
Ultra-simple example:

$ py -3.7
Python 3.7.6 (tags/v3.7.6:43364a7ae0, Dec 19 2019, 00:42:30) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> with open('my_dir/my_file.txt') as f:
...     print(f.readlines())
...
['Hello gaborbernat!\n']

I do understand that virtualenv is likely doing way more stuff with that path, with millions of ways for things to go haywire. Yet, that's a regression in virtualenv IMO.
I'll try to dig in to see if I can give more details on what happens internally, and if I can submit something.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Feb 11, 2020

If you don't make headway on this I'll look into it tomorrow why this fails, and if it would be possible to make it work 🤞if you can come up with a pr that would be epic though 👍

@vphilippon

This comment has been minimized.

Copy link
Member Author

@vphilippon vphilippon commented Feb 12, 2020

@gaborbernat Do you remember why you decided to explicitly dissalow os.altsep in the path?

for char in (i for i in (os.pathsep, os.altsep) if i is not None):

524d95e

Did you hit some specific issues with it?
If I undertand correctly, it should be a valid path component.
https://docs.python.org/3.8/library/os.html#os.altsep

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Feb 12, 2020

Some tools support altsep so using them might break things. To be fair this would only be an issue if the paths we write into the activation scripts are used in the altsep form. If you can add tests/validate that we normalize altsep to pathsep during the env creation I think we can remove this constraint.

@vphilippon

This comment has been minimized.

Copy link
Member Author

@vphilippon vphilippon commented Feb 12, 2020

Some tools support altsep so using them might break things.

I'm not sure I understand: Wouldn't it be the opposite: those tools support / just like \ on Windows, so having virtualenv support it wouldn't be an issue, right?

If you can add tests/validate that we normalize altsep to pathsep [...]

(emphasis mine)

Could you be interpreting that os.altsep as something like os.altpathsep, i.e. that / acts like ; on Windows? If so, I think that's incorrect, / is an alternative to \ (once again, if my experience, and doc reading, is correct).

Edit:
Might be clearer like this:
os.sep - os.altsep: pathname separator (ex: /, \)
os.pathsep path separator (ex: :, ;)

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Feb 12, 2020

Yeah I think you're right should be altpathsep

@vphilippon

This comment has been minimized.

Copy link
Member Author

@vphilippon vphilippon commented Feb 12, 2020

Alright, glad this was cleared up :)

Although, don't go searching for os.altpathsep, it doesn't exist :P.
It was an example name to try and point the concept difference. Sorry about that.

I can submit and MR, which essentially is a revert of 524d95e

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Feb 14, 2020

Hello, a fix for this issue has been released via virtualenv 20.0.4; see https://pypi.org/project/virtualenv/20.0.4/ (https://virtualenv.pypa.io/en/latest/changelog.html#v20-0-4-2020-02-14). 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.