Repository: pipxproject/pipx (python)

Issue:
Installation not pulling in optional extras-requires packages. 
(Full disclosure, this issue is occurring after an OS upgrade, though I've uninstalled and re-installed pipx) -- great tool by the way!!

I have a project on Bitbucket (private). I have historically installed locally with pipx; 
`pipx install "my-project[cli] @ git+ssh://git@bitbucket.org/my-company/myproject.git"`

The setup.cfg for the project has an install-requires of
`[options.extras_require]`
```
cli =
    begins
    docker
```

Installing goes smoothly, but the `begins` and `docker` packages do not install. I verify by looking within `/home/steve/.local/pipx/venvs/my-project/lib/python/site-packages`

[pipx-verbose.txt](https://github.com/pipxproject/pipx/files/5330613/pipx-verbose.txt)



**Expected behavior**
Historically, this would have correctly detected the [cli] designation and installed the appropriate packages. I'm not sure if this has to do with a recent upgrade or something is broken on my end. 

If it is an issue on my part, can you recommend troubleshooting tips? 

Thank you, Sincerely, Steve Jackson


Pull request description:
Fix pep508
<!-- add an 'x' in the brackets below -->
* [x] I have added an entry to `docs/changelog.md`

## Summary of changes
Fixes bug in handling of PEP508 package specifications including URLs and extras

Closes #515 

I was too aggressive in the original code for "cleaning" PEP508 package specifications for pipx.  I originally removed the package name if there was a URL present, on the idea that the user-supplied name might not match the URL and we should just use the URL only to avoid confusion.  Unfortunately, this also removed the only way to supply package extras with a URL.

The new code is more surgical.  The basic idea: convert package specification string to Requirement object, null out anything we don't want as pipx, convert Requirement object back to string.  We then use that string for `package_or_url`.

To guard against the user supplying a mismatched package name to the URL in their PEP508 package specification, I modified `valid_pypi_name` to return `None` if a URL is present, signifying to the calling code that it needs to do a trial install of the URL to determine package name, even if it's present in the PEP508 package specifier.   I also, introduced a new function `fix_package_name` which fixes the package_name part of a spec if necessary.

## Test plan
<!-- provide evidence of testing, preferably with command(s) that can be copy+pasted by others -->
`pipx install <package>[<extra>] @ URL`

Does anybody have a good suggestion for a test package that uses a URL and has at least one possible extra?  I couldn't easily find one.


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 8287291..b48989c 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -3,8 +3,9 @@ dev
 - [docs] Update license
 - [bugfix] Fixed regression in list, inject, upgrade, reinstall-all commands when suffixed packages are used.
 - [bugfix] Do not reset package url during upgrade when main package is `pipx`
-- Update help text to show description for `ensurepath` and `completions` help
-- Add support for user-defined default python interpreter via new PIPX_DEFAULT_PYTHON.  Helpful for use with pyenv among other uses.
+- Updated help text to show description for `ensurepath` and `completions` help
+- Added support for user-defined default python interpreter via new PIPX_DEFAULT_PYTHON.  Helpful for use with pyenv among other uses.
+- [bugfix] Fixed bug where extras were ignored with a PEP 508 package specification with a URL.
 
 
 0.15.5.1
diff --git a/src/pipx/package_specifier.py b/src/pipx/package_specifier.py
index 8c66346..1fd25af 100644
--- a/src/pipx/package_specifier.py
+++ b/src/pipx/package_specifier.py
@@ -13,6 +13,7 @@ from pathlib import Path
 from typing import List, NamedTuple, Optional, Set, Tuple
 
 from packaging.requirements import InvalidRequirement, Requirement
+from packaging.specifiers import SpecifierSet
 from packaging.utils import canonicalize_name
 
 from pipx.emojies import hazard
@@ -97,44 +98,57 @@ def _extras_to_str(extras: Set):
         return ""
 
 
-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
+def package_or_url_from_pep508(
+    requirement: Requirement, remove_version_specifiers=False
+) -> str:
+    requirement.marker = None
+    requirement.name = canonicalize_name(requirement.name)
+    if remove_version_specifiers:
+        requirement.specifier = SpecifierSet("")
+    return str(requirement)
 
-    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)
+
+def _parsed_package_to_package_or_url(
+    parsed_package: ParsedPackage, remove_version_specifiers: bool
+) -> str:
     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="    ",
-                    )
+        if parsed_package.valid_pep508.marker is not None:
+            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="    ",
                 )
+            )
+        package_or_url = package_or_url_from_pep508(
+            parsed_package.valid_pep508,
+            remove_version_specifiers=remove_version_specifiers,
+        )
     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
+
 
+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)
+    package_or_url = _parsed_package_to_package_or_url(
+        parsed_package, remove_version_specifiers=False
+    )
     if "--editable" in pip_args and not parsed_package.valid_local_path:
         logging.warning(
             textwrap.fill(
@@ -158,19 +172,9 @@ def parse_specifier_for_metadata(package_spec: str) -> str:
     * 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}")
-
+    package_or_url = _parsed_package_to_package_or_url(
+        parsed_package, remove_version_specifiers=True
+    )
     return package_or_url
 
 
@@ -181,4 +185,30 @@ def valid_pypi_name(package_spec: str) -> Optional[str]:
         # not a valid PEP508 package specification
         return None
 
-    return package_req.name
+    if package_req.url:
+        # package name supplied by user might not match package found in URL,
+        #   so force package name determination the long way
+        return None
+
+    return canonicalize_name(package_req.name)
+
+
+def fix_package_name(package_or_url: str, package: str) -> str:
+    try:
+        package_req = Requirement(package_or_url)
+    except InvalidRequirement:
+        # not a valid PEP508 package specification
+        return package_or_url
+
+    if canonicalize_name(package_req.name) != canonicalize_name(package):
+        logging.warning(
+            textwrap.fill(
+                f"{hazard}  Name supplied in package specifier was "
+                f"{package_req.name!r} but package found has name "
+                f"{package!r}.  Using {package!r}.",
+                subsequent_indent="    ",
+            )
+        )
+    package_req.name = package
+
+    return str(package_req)
diff --git a/src/pipx/venv.py b/src/pipx/venv.py
index 3bf1e6f..c2a4ed7 100644
--- a/src/pipx/venv.py
+++ b/src/pipx/venv.py
@@ -8,6 +8,7 @@ from pipx.animate import animate
 from pipx.constants import PIPX_SHARED_PTH
 from pipx.interpreter import DEFAULT_PYTHON
 from pipx.package_specifier import (
+    fix_package_name,
     parse_specifier_for_install,
     parse_specifier_for_metadata,
 )
@@ -183,6 +184,9 @@ class Venv:
         if pip_args is None:
             pip_args = []
 
+        # package name in package specifier can mismatch URL due to user error
+        package_or_url = fix_package_name(package_or_url, package)
+
         # check syntax and clean up spec and pip_args
         (package_or_url, pip_args) = parse_specifier_for_install(
             package_or_url, pip_args


Test patch:
diff --git a/tests/test_package_specifier.py b/tests/test_package_specifier.py
index 1c79644..b11c8b9 100644
--- a/tests/test_package_specifier.py
+++ b/tests/test_package_specifier.py
@@ -3,14 +3,52 @@ from pathlib import Path
 import pytest  # type: ignore
 
 from pipx.package_specifier import (
+    fix_package_name,
     parse_specifier_for_install,
     parse_specifier_for_metadata,
+    valid_pypi_name,
 )
 from pipx.util import PipxError
 
 TEST_DATA_PATH = "./testdata/test_package_specifier"
 
 
+@pytest.mark.parametrize(
+    "package_spec_in,package_name_out",
+    [
+        ("Black", "black"),
+        ("https://github.com/ambv/black/archive/18.9b0.zip", None),
+        ("black @ https://github.com/ambv/black/archive/18.9b0.zip", None),
+    ],
+)
+def test_valid_pypi_name(package_spec_in, package_name_out):
+    assert valid_pypi_name(package_spec_in) == package_name_out
+
+
+@pytest.mark.parametrize(
+    "package_spec_in,package_name,package_spec_out",
+    [
+        (
+            "https://github.com/ambv/black/archive/18.9b0.zip",
+            "black",
+            "https://github.com/ambv/black/archive/18.9b0.zip",
+        ),
+        (
+            "nox@https://github.com/ambv/black/archive/18.9b0.zip",
+            "black",
+            "black@ https://github.com/ambv/black/archive/18.9b0.zip",
+        ),
+        (
+            "nox[extra]@https://github.com/ambv/black/archive/18.9b0.zip",
+            "black",
+            "black[extra]@ https://github.com/ambv/black/archive/18.9b0.zip",
+        ),
+    ],
+)
+def test_fix_package_name(package_spec_in, package_name, package_spec_out):
+    assert fix_package_name(package_spec_in, package_name) == package_spec_out
+
+
 # TODO: Make sure git+ works with tests, correct in test_install as well
 @pytest.mark.parametrize(
     "package_spec_in,package_or_url_correct,valid_spec",
@@ -32,7 +70,7 @@ TEST_DATA_PATH = "./testdata/test_package_specifier"
         ),
         (
             "nox@git+https://github.com/cs01/nox.git@5ea70723e9e6",
-            "git+https://github.com/cs01/nox.git@5ea70723e9e6",
+            "nox@ git+https://github.com/cs01/nox.git@5ea70723e9e6",
             True,
         ),
         (
@@ -42,12 +80,22 @@ TEST_DATA_PATH = "./testdata/test_package_specifier"
         ),
         (
             "black@https://github.com/ambv/black/archive/18.9b0.zip",
-            "https://github.com/ambv/black/archive/18.9b0.zip",
+            "black@ https://github.com/ambv/black/archive/18.9b0.zip",
             True,
         ),
         (
             "black @ https://github.com/ambv/black/archive/18.9b0.zip",
-            "https://github.com/ambv/black/archive/18.9b0.zip",
+            "black@ https://github.com/ambv/black/archive/18.9b0.zip",
+            True,
+        ),
+        (
+            "black[extra] @ https://github.com/ambv/black/archive/18.9b0.zip",
+            "black[extra]@ https://github.com/ambv/black/archive/18.9b0.zip",
+            True,
+        ),
+        (
+            'my-project[cli] @ git+ssh://git@bitbucket.org/my-company/myproject.git ; python_version<"3.7"',
+            "my-project[cli]@ git+ssh://git@bitbucket.org/my-company/myproject.git",
             True,
         ),
         ("path/doesnt/exist", "non-existent-path", False,),