Repository: pypa/virtualenv
Pull request #1360 (2019-05-23 18:46:09)

Check for os.pathsep for path.  If we use a directory with os.pathsep…
…, the virtualenv can't be added to the PATH.

## Thanks for contributing a pull request, see checklist all is good!

- [x] wrote descriptive pull request text
- [x ] added/updated test(s)
- [ ] updated/extended the documentation

Do we want to document this limitation somewhere?  Or just let it be an error?

- [x ] added news fragment in ``docs/changelog`` folder


rrauenza: Hmm -- I think os.path.exists(path) on windows checks for ;?
gaborbernat: maybe does multi path check , no idea, can you find it out?
rrauenza: So it does actually check for the ; ... according to my interactive test.

```
>>> os.path.exists('c:\\temp')
True
>>> os.path.exists('c:\\temp;1')
False
>>> os.path.exists('c:\\temp;')
False
>>> os.path.exists('c:\\temp\;')
False
>>> os.path.exists('c:\\temp;')
False
>>> os.path.exists('c:\\temp')
True
>>> os.mkdir('c:\\temp\\x;')
>>> 
```
rrauenza: It's as if the directory actually exists.  I've modified my test slightly.  I've rebased and force pushed ...  did that trigger a new CI?  I wouldn't think it would since it's a new SHA.
rrauenza: ok, latest test shows that the exists fails before my test even begins -- the directory already exists.  Maybe from another test?

gaborbernat: you should definitely changedir to a temp folder to perform the operation
rrauenza: Can we force the CI again? Looks like an infrastructure problem this time.
gaborbernat: just force push ammend your last commit.

Patch:
diff --git a/docs/changelog/395.bugfix.rst b/docs/changelog/395.bugfix.rst
new file mode 100644
index 00000000..a5436e0a
--- /dev/null
+++ b/docs/changelog/395.bugfix.rst
@@ -0,0 +1,1 @@
+Raise an error if the target path contains the operating systems path separator (using this would break our activation scripts) - by @rrauenza.
diff --git a/virtualenv.py b/virtualenv.py
index c9a2fca3..cd76bf6f 100755
--- a/virtualenv.py
+++ b/virtualenv.py
@@ -804,6 +804,11 @@ def main():
         logger.fatal("Please provide a different path or delete the file.")
         sys.exit(3)
 
+    if os.pathsep in home_dir:
+        logger.fatal("ERROR: target path contains the operating system path separator '{}'".format(os.pathsep))
+        logger.fatal("This is not allowed as would make the activation scripts unusable.".format(os.pathsep))
+        sys.exit(3)
+
     if os.environ.get("WORKING_ENV"):
         logger.fatal("ERROR: you cannot run virtualenv while in a working env")
         logger.fatal("Please deactivate your working env, then re-run this script")


Test patch:
diff --git a/tests/test_cmdline.py b/tests/test_cmdline.py
index 814fd526..62738844 100644
--- a/tests/test_cmdline.py
+++ b/tests/test_cmdline.py
@@ -9,18 +9,10 @@ import pytest
 import virtualenv
 
 
-def get_src(path):
-    base, _ = os.path.splitext(path)
-    return "{}.py".format(base)
-
-
-VIRTUALENV_SCRIPT = get_src(virtualenv.__file__)
-
-
 def test_commandline_basic(tmpdir):
     """Simple command line usage should work and files should be generated"""
     home_dir, lib_dir, inc_dir, bin_dir = virtualenv.path_locations(str(tmpdir.join("venv")))
-    subprocess.check_call([sys.executable, VIRTUALENV_SCRIPT, home_dir])
+    subprocess.check_call([sys.executable, "-m", "virtualenv", home_dir])
 
     assert os.path.exists(home_dir)
     assert os.path.exists(bin_dir)
@@ -38,9 +30,31 @@ def test_commandline_basic(tmpdir):
     _check_no_warnings("distutils")
 
 
+def test_commandline_os_path_sep(tmp_path):
+    path = tmp_path / "bad{}0".format(os.pathsep)
+    assert not path.exists()
+    process = subprocess.Popen(
+        [sys.executable, "-m", "virtualenv", str(path)],
+        cwd=str(tmp_path),
+        universal_newlines=True,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        stdin=subprocess.PIPE,
+    )
+    out, err = process.communicate()
+    assert process.returncode == 3
+    msg = (
+        "ERROR: target path contains the operating system path separator '{}'\n"
+        "This is not allowed as would make the activation scripts unusable.\n".format(os.pathsep)
+    )
+    assert not err
+    assert out == msg
+    assert not path.exists()
+
+
 def test_commandline_explicit_interp(tmpdir):
     """Specifying the Python interpreter should work"""
-    subprocess.check_call([sys.executable, VIRTUALENV_SCRIPT, "-p", sys.executable, str(tmpdir.join("venv"))])
+    subprocess.check_call([sys.executable, "-m", "virtualenv", "-p", sys.executable, str(tmpdir.join("venv"))])
 
 
 # The registry lookups to support the abbreviated "-p 3.5" form of specifying
@@ -52,4 +66,4 @@ def test_commandline_explicit_interp(tmpdir):
 def test_commandline_abbrev_interp(tmpdir):
     """Specifying abbreviated forms of the Python interpreter should work"""
     abbrev = "{}{}.{}".format("" if sys.platform == "win32" else "python", *sys.version_info[0:2])
-    subprocess.check_call([sys.executable, VIRTUALENV_SCRIPT, "-p", abbrev, str(tmpdir.join("venv"))])
+    subprocess.check_call([sys.executable, "-m", "virtualenv", "-p", abbrev, str(tmpdir.join("venv"))])