Repository: pipxproject/pipx (python)

Issue:
Installing VCS URL should not use `--editable` option
<!-- Thank you for filing a bug! Please feel free to answer as much or as little of this template as you can. -->

**Describe the bug**
<!-- Please be as detailed as possible! -->
Our package name detection scheme depends on installing a package first with pip and querying its name.  pip seems to allow installation with a bare VCS url **ONLY** if the `--editable` flag is not present.  If the `--editable` flag is present, then pip seems to require the suffix `#egg=<package_name>`.

**How to reproduce**
<!-- If possible, include output of `pipx --verbose ...` -->

Using a git package from another issue I was reading (#148) and trying to reproduce:

```
birchtree:pipx pipx install --verbose --editable git+https://github.com/raxod502/diary-manager
pipx > (run_subprocess:112): running /usr/local/opt/python@3.8/bin/python3.8 -m venv --without-pip /var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli
pipx > (run_subprocess:112): running /var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli/bin/python -c import sysconfig; print(sysconfig.get_path('purelib'))
pipx > (run_subprocess:112): running /Users/mclapp/.local/pipx/shared/bin/python -c import sysconfig; print(sysconfig.get_path('purelib'))
pipx > (run_subprocess:112): running /var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli/bin/python --version
pipx > (run_subprocess:112): running /var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli/bin/python -m pip list --format=json
pipx > (run_subprocess:112): running /var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli/bin/python -m pip install --no-dependencies --editable git+https://github.com/raxod502/diary-manager
ERROR: Could not detect requirement name for 'git+https://github.com/raxod502/diary-manager', please specify one with #egg=your_package_name
pipx > (install_package_no_deps:221): '/var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/tmp_5cntfli/bin/python -m pip install --no-dependencies --editable git+https://github.com/raxod502/diary-manager' failed
Cannot determine package name from spec 'git+https://github.com/raxod502/diary-manager'. Check package spec for errors.
```

Just with pip itself, `--editable` + VCS URL needs `#egg`:
```
birchtree:pipx pip install --no-dependencies --editable git+https://github.com/raxod502/diary-manager
ERROR: Could not detect requirement name for 'git+https://github.com/raxod502/diary-manager', please specify one with #egg=your_package_name
```
Just with pip itself, bare VCS URL does NOT need `#egg`:
```
birchtree:pipx pip install --no-dependencies git+https://github.com/raxod502/diary-manager 
Collecting git+https://github.com/raxod502/diary-manager
  Cloning https://github.com/raxod502/diary-manager to /private/var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/pip-req-build-5kt0mjb7
  Running command git clone -q https://github.com/raxod502/diary-manager /private/var/folders/gp/7z0mcw717y1b0kt0swc1lfp80000gn/T/pip-req-build-5kt0mjb7
Using legacy setup.py install for diary-manager, since package 'wheel' is not installed.
Installing collected packages: diary-manager
    Running setup.py install for diary-manager ... done
Successfully installed diary-manager-2.0.2
```

**Expected behavior**
<!-- What should have happened? -->
I expected pip could install the VCS URL whether it was --editable or not.

Pull request:
Parse package specifier before install
<!-- add an 'x' in the brackets below -->
[x] I have added an entry to `docs/changelog.md`

## Summary of changes
Fixes #438 .

This parses a package specifier before install.

It removes the `--editable` option for any package specification that is not a local path, and gives a warning that it is doing so.
It removes any environment markers from the package specification and gives a warning that it is doing so.
I also refactored `package_specifier.py` a bit.
And a bunch of tests were added for the new functionality.

I started this PR as a fix for a pip error within pipx that was complicated enough that there might be user confusion.  (See #438).  Once I was parsing the package specifier, I also decided to remove the environment markers that cause errors in pipx operation, and anyway seemed to me not a fit for pipx's use-case.

## Test plan
<!-- provide evidence of testing, preferably with command(s) that can be copy+pasted by others -->
### Test 1
```
pipx install -e pylint
```
This PR gives a warning that pipx is removing the `--editable` option.  The current dev version of pipx would just pass-through a pip error.

### Test 2
```
pipx install --editable git+https://github.com/raxod502/diary-manager
```
This PR gives a warning that pipx is removing the `--editable` option.  The current dev version of pipx would just pass-through an even more complicated pip error.

### Test 3
```
pipx install 'pylint;python_version=="3.6"'
```
Current dev pipx gives cryptic error:
```
Internal error with venv metadata inspection.

Unable to install pylint from spec 'pylint;python_version=="3.6"'.
Check the name or spec for errors, and verify that it can be installed with pip.
```
This PR pipx gives warning it is ignoring markers and proceeds normally (without error!) to install pylint.

Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 8055cfe..c0d0b12 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,4 +1,5 @@
 dev
+- pipx now parses package specification before install.   It removes (with warning) the `--editable` install option for any package specification that is not a local path.   It also removes (with warning) any environment markers.
 - Disabled animation when we cannot determine terminal size or if the number of columns is too small. (Fixes #444)
 - Version of each injected package is now listed after name for `pipx list --include-injected`
 - Change metadata recorded from version-specified install to allow upgrades in future.  Adds pipx dependency on `packaging` package.
diff --git a/src/pipx/commands/common.py b/src/pipx/commands/common.py
index b881944..b5c5d2a 100644
--- a/src/pipx/commands/common.py
+++ b/src/pipx/commands/common.py
@@ -14,10 +14,10 @@ import userpath  # type: ignore
 from pipx import constants
 from pipx.colors import bold, red
 from pipx.emojies import hazard, stars
-from pipx.package_specifier import valid_pypi_name
+from pipx.package_specifier import parse_specifier_for_install, valid_pypi_name
+from pipx.pipx_metadata_file import PackageInfo
 from pipx.util import WINDOWS, PipxError, mkdir, rmdir
 from pipx.venv import Venv
-from pipx.pipx_metadata_file import PackageInfo
 
 
 def expose_apps_globally(
@@ -244,9 +244,9 @@ def package_name_from_spec(
 ) -> str:
     start_time = time.time()
 
-    # shortcut if valid PyPI name and not a local path
+    # shortcut if valid PyPI name
     pypi_name = valid_pypi_name(package_spec)
-    if pypi_name is not None and not Path(package_spec).exists():
+    if pypi_name is not None:
         # NOTE: if pypi name and installed package name differ, this means pipx
         #       will use the pypi name
         package_name = pypi_name
@@ -254,6 +254,9 @@ def package_name_from_spec(
         logging.info(f"Package name determined in {time.time()-start_time:.1f}s")
         return package_name
 
+    # check syntax and clean up spec and pip_args
+    (package_spec, pip_args) = parse_specifier_for_install(package_spec, pip_args)
+
     with tempfile.TemporaryDirectory() as temp_venv_dir:
         venv = Venv(Path(temp_venv_dir), python=python, verbose=verbose)
         venv.create_venv(venv_args=[], pip_args=[])
diff --git a/src/pipx/commands/run.py b/src/pipx/commands/run.py
index 8aacbc2..a9b812e 100644
--- a/src/pipx/commands/run.py
+++ b/src/pipx/commands/run.py
@@ -10,6 +10,7 @@ from shutil import which
 from typing import List
 
 from pipx import constants
+from pipx.commands.common import package_name_from_spec
 from pipx.constants import TEMP_VENV_EXPIRATION_THRESHOLD_DAYS
 from pipx.emojies import hazard
 from pipx.util import (
@@ -115,12 +116,15 @@ def _download_and_run(
     venv = Venv(venv_dir, python=python, verbose=verbose)
     venv.create_venv(venv_args, pip_args)
 
-    # venv.pipx_metadata.main_package.package contains package name if it is
-    #   pre-existing, otherwise is None to instruct venv.install_package to
-    #   determine package name.
+    if venv.pipx_metadata.main_package.package is not None:
+        package = venv.pipx_metadata.main_package.package
+    else:
+        package = package_name_from_spec(
+            package_or_url, python, pip_args=pip_args, verbose=verbose
+        )
 
     venv.install_package(
-        package=venv.pipx_metadata.main_package.package,
+        package=package,
         package_or_url=package_or_url,
         pip_args=pip_args,
         include_dependencies=False,
diff --git a/src/pipx/package_specifier.py b/src/pipx/package_specifier.py
index 500a1e4..1329830 100644
--- a/src/pipx/package_specifier.py
+++ b/src/pipx/package_specifier.py
@@ -6,52 +6,44 @@
 #   <pypi_package_name><version_specifier>
 
 import logging
+import textwrap
 from pathlib import Path
-from typing import Optional
+from typing import List, NamedTuple, Optional, Set, Tuple
 
 from packaging.requirements import InvalidRequirement, Requirement
 from packaging.utils import canonicalize_name
 
+from pipx.emojies import hazard
 from pipx.util import PipxError
 
 
-def parse_specifier_for_metadata(package_spec: str) -> str:
-    """Return package_or_url suitable for pipx metadata
+class ParsedPackage(NamedTuple):
+    valid_pep508: Optional[Requirement]
+    valid_url: Optional[str]
+    valid_local_path: Optional[str]
 
-    Specifically:
-    * Strip any version specifiers (e.g. package == 1.5.4)
-    * Strip any markers (e.g. python_version > 3.4)
-    * Convert local paths to absolute paths
+
+def _parse_specifier(package_spec: str) -> ParsedPackage:
+    """Parse package_spec as would be given to pipx
     """
     # NOTE: If package_spec is valid pypi name, pip will always treat it as a
     #       pypi package, not checking for local path.
     #       We replicate pypi precedence here (only non-valid-pypi names
     #       initiate check for local path, e.g. './package-name')
-
-    valid_pep508 = False
-    valid_url = False
-    valid_local_path = False
-    package_or_url = ""
+    valid_pep508 = None
+    valid_url = None
+    valid_local_path = None
 
     try:
         package_req = Requirement(package_spec)
     except InvalidRequirement:
         # not a valid PEP508 package specification
-        valid_pep508 = False
+        pass
     else:
         # valid PEP508 package specification
-        valid_pep508 = True
-        if package_req.url:
-            package_or_url = package_req.url
-        else:
-            if package_req.extras:
-                package_or_url = canonicalize_name(
-                    package_req.name + "[" + ",".join(sorted(package_req.extras)) + "]"
-                )
-            else:
-                package_or_url = canonicalize_name(package_req.name)
+        valid_pep508 = package_req
 
-    # NOTE: packaging currently (2020-07-19) does basic syntax checks on the URL.
+    # NOTE: packaging currently (2020-07-19) only does basic syntax checks on URL.
     #   Some examples of what it will not catch:
     #       - invalid RCS string (e.g. "gat+https://...")
     #       - non-existent scheme (e.g. "zzzzz://...")
@@ -59,10 +51,10 @@ def parse_specifier_for_metadata(package_spec: str) -> str:
         try:
             package_req = Requirement("notapackagename @ " + package_spec)
         except InvalidRequirement:
-            valid_url = False
+            # not a valid url
+            pass
         else:
-            valid_url = True
-            package_or_url = package_spec
+            valid_url = package_spec
 
     if not valid_pep508 and not valid_url:
         package_path = Path(package_spec)
@@ -72,12 +64,101 @@ def parse_specifier_for_metadata(package_spec: str) -> str:
             package_path_exists = False
 
         if package_path_exists:
-            valid_local_path = True
-            package_or_url = str(package_path.resolve())
+            valid_local_path = str(package_path.resolve())
 
     if not valid_pep508 and not valid_url and not valid_local_path:
         raise PipxError(f"Unable to parse package spec: {package_spec}")
 
+    return ParsedPackage(
+        valid_pep508=valid_pep508,
+        valid_url=valid_url,
+        valid_local_path=valid_local_path,
+    )
+
+
+def _extras_to_str(extras: Set):
+    if extras:
+        return "[" + ",".join(sorted(extras)) + "]"
+    else:
+        return ""
+
+
+def package_is_local_path(package_spec: str) -> bool:
+    return _parse_specifier(package_spec).valid_local_path is not None
+
+
+def parse_specifier_for_install(
+    package_spec: str, pip_args: List[str]
+) -> Tuple[str, List[str]]:
+    """Return package_or_url and pip_args suitable for pip install
+
+    Specifically:
+    * Strip any markers (e.g. python_version > 3.4)
+    * Ensure --editable is removed for any package_spec not a local path
+    * Convert local paths to absolute paths
+    """
+    parsed_package = _parse_specifier(package_spec)
+    if parsed_package.valid_pep508 is not None:
+        if parsed_package.valid_pep508.url:
+            package_or_url = parsed_package.valid_pep508.url
+        else:
+            package_or_url = canonicalize_name(parsed_package.valid_pep508.name)
+            package_or_url += _extras_to_str(parsed_package.valid_pep508.extras)
+            if parsed_package.valid_pep508.specifier:
+                package_or_url = package_or_url + str(
+                    parsed_package.valid_pep508.specifier
+                )
+            if parsed_package.valid_pep508.marker:
+                logging.warning(
+                    textwrap.fill(
+                        f"{hazard}  Ignoring environment markers "
+                        f"({parsed_package.valid_pep508.marker}) in package "
+                        "specification. Use pipx options to specify this type of "
+                        "information.",
+                        subsequent_indent="    ",
+                    )
+                )
+    elif parsed_package.valid_url is not None:
+        package_or_url = parsed_package.valid_url
+    elif parsed_package.valid_local_path is not None:
+        package_or_url = parsed_package.valid_local_path
+
+    logging.info(f"cleaned package spec: {package_or_url}")
+
+    if "--editable" in pip_args and not parsed_package.valid_local_path:
+        logging.warning(
+            textwrap.fill(
+                f"{hazard}  Ignoring --editable install option. pipx disallows it "
+                "for anything but a local path, to avoid having to create a new "
+                "src/ directory.",
+                subsequent_indent="    ",
+            )
+        )
+        pip_args.remove("--editable")
+
+    return (package_or_url, pip_args)
+
+
+def parse_specifier_for_metadata(package_spec: str) -> str:
+    """Return package_or_url suitable for pipx metadata
+
+    Specifically:
+    * Strip any version specifiers (e.g. package == 1.5.4)
+    * Strip any markers (e.g. python_version > 3.4)
+    * Convert local paths to absolute paths
+    """
+    parsed_package = _parse_specifier(package_spec)
+    if parsed_package.valid_pep508 is not None:
+        if parsed_package.valid_pep508.url:
+            package_or_url = parsed_package.valid_pep508.url
+        else:
+            package_or_url = canonicalize_name(parsed_package.valid_pep508.name)
+            package_or_url += _extras_to_str(parsed_package.valid_pep508.extras)
+    elif parsed_package.valid_url is not None:
+        package_or_url = parsed_package.valid_url
+    elif parsed_package.valid_local_path is not None:
+        package_or_url = parsed_package.valid_local_path
+
     logging.info(f"cleaned package spec: {package_or_url}")
 
     return package_or_url
diff --git a/src/pipx/venv.py b/src/pipx/venv.py
index d0c67e9..92d8821 100644
--- a/src/pipx/venv.py
+++ b/src/pipx/venv.py
@@ -2,11 +2,14 @@ import json
 import logging
 import pkgutil
 from pathlib import Path
-from typing import Dict, Generator, List, NamedTuple, Optional, Set
+from typing import Dict, Generator, List, NamedTuple, Set
 
 from pipx.animate import animate
 from pipx.constants import DEFAULT_PYTHON, PIPX_SHARED_PTH
-from pipx.package_specifier import parse_specifier_for_metadata
+from pipx.package_specifier import (
+    parse_specifier_for_install,
+    parse_specifier_for_metadata,
+)
 from pipx.pipx_metadata_file import PackageInfo, PipxMetadata
 from pipx.shared_libs import shared_libs
 from pipx.util import (
@@ -159,20 +162,20 @@ class Venv:
 
     def install_package(
         self,
-        package: Optional[str],  # if None, will be determined in this function
+        package: str,
         package_or_url: str,
         pip_args: List[str],
         include_dependencies: bool,
         include_apps: bool,
         is_main_package: bool,
     ) -> None:
-
         if pip_args is None:
             pip_args = []
-        if package is None:
-            # If no package name is supplied, install only main package
-            #   first in order to see what its name is
-            package = self.install_package_no_deps(package_or_url, pip_args)
+
+        # check syntax and clean up spec and pip_args
+        (package_or_url, pip_args) = parse_specifier_for_install(
+            package_or_url, pip_args
+        )
 
         try:
             with animate(
@@ -224,7 +227,7 @@ class Venv:
         installed_packages = self.list_installed_packages() - old_package_set
         if len(installed_packages) == 1:
             package = installed_packages.pop()
-            logging.info(f"Determined package name: '{package}'")
+            logging.info(f"Determined package name: {package}")
         else:
             logging.info(f"old_package_set = {old_package_set}")
             logging.info(f"install_packages = {installed_packages}")


Test patch:
diff --git a/tests/test_package_specifier.py b/tests/test_package_specifier.py
index 60acc9b..679970b 100644
--- a/tests/test_package_specifier.py
+++ b/tests/test_package_specifier.py
@@ -2,7 +2,10 @@ from pathlib import Path
 
 import pytest  # type: ignore
 
-from pipx.package_specifier import parse_specifier_for_metadata
+from pipx.package_specifier import (
+    parse_specifier_for_install,
+    parse_specifier_for_metadata,
+)
 from pipx.util import PipxError
 
 
@@ -64,3 +67,60 @@ def test_parse_specifier_for_metadata(
         print(f"package_spec_in = {package_spec_in}")
         with pytest.raises(PipxError, match=r"^Unable to parse package spec"):
             package_or_url = parse_specifier_for_metadata(package_spec_in)
+
+
+@pytest.mark.parametrize(
+    "package_spec_in,pip_args_in,package_spec_expected,pip_args_expected,warning_str",
+    [
+        ('pipx==0.15.0;python_version>="3.6"', [], "pipx==0.15.0", [], None),
+        ("pipx==0.15.0", ["--editable"], "pipx==0.15.0", [], "Ignoring --editable",),
+        (
+            'pipx==0.15.0;python_version>="3.6"',
+            [],
+            "pipx==0.15.0",
+            [],
+            'Ignoring environment markers (python_version >= "3.6") in package',
+        ),
+        (
+            "pipx==0.15.0",
+            ["--no-cache-dir", "--editable"],
+            "pipx==0.15.0",
+            ["--no-cache-dir"],
+            "Ignoring --editable",
+        ),
+        (
+            "git+https://github.com/cs01/nox.git@5ea70723e9e6",
+            ["--editable"],
+            "git+https://github.com/cs01/nox.git@5ea70723e9e6",
+            [],
+            "Ignoring --editable",
+        ),
+        (
+            "https://github.com/ambv/black/archive/18.9b0.zip",
+            ["--editable"],
+            "https://github.com/ambv/black/archive/18.9b0.zip",
+            [],
+            "Ignoring --editable",
+        ),
+        (
+            "src/pipx",
+            ["--editable"],
+            str(Path("src/pipx").resolve()),
+            ["--editable"],
+            None,
+        ),
+    ],
+)
+def test_parse_specifier_for_install(
+    caplog,
+    package_spec_in,
+    pip_args_in,
+    package_spec_expected,
+    pip_args_expected,
+    warning_str,
+):
+    [package_or_url_out, pip_args_out] = parse_specifier_for_install(
+        package_spec_in, pip_args_in
+    )
+    if warning_str is not None:
+        assert warning_str in caplog.text
diff --git a/tests/test_run.py b/tests/test_run.py
index 1601a3f..513b25e 100644
--- a/tests/test_run.py
+++ b/tests/test_run.py
@@ -1,12 +1,11 @@
 #!/usr/bin/env python3
-import os
 import logging
-import sys
+import os
 import subprocess
+import sys
 from unittest import mock
 
 import pytest  # type: ignore
-
 from helpers import run_pipx_cli
 
 import pipx.main
@@ -123,4 +122,4 @@ def test_package_determination(
     run_pipx_cli(["run", "--verbose", "--spec", package_or_url, "--"] + app_appargs)
 
     assert "Cannot determine package name" not in caplog.text
-    assert f"Determined package name: '{package}'" in caplog.text
+    assert f"Determined package name: {package}" in caplog.text