Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/5934.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Eveb better handling of vcs branch references that contain special characters.
23 changes: 15 additions & 8 deletions pipenv/utils/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,20 @@ def _file_path_from_pipfile(path_obj, pipfile_entry):
return req_str


def normalize_vcs_url(vcs_url):
"""Return vcs_url and possible vcs_ref from a given vcs_url."""
# We have to handle the fact that some vcs urls have a ref in them
# and some have a netloc with a username and password in them, and some have both
vcs_ref = ""
if "@" in vcs_url:
parsed_url = urlparse(vcs_url)
if "@" in parsed_url.path:
url_parts = vcs_url.rsplit("@", 1)
vcs_url = url_parts[0]
vcs_ref = url_parts[1]
return vcs_url, vcs_ref


def install_req_from_pipfile(name, pipfile):
"""Creates an InstallRequirement from a name and a pipfile entry.
Handles VCS, local & remote paths, and regular named requirements.
Expand All @@ -1051,14 +1065,7 @@ def install_req_from_pipfile(name, pipfile):
subdirectory = _pipfile.get("subdirectory", "")
if subdirectory:
subdirectory = f"#subdirectory={subdirectory}"
fallback_ref = ""
if ("ssh://" in vcs_url and vcs_url.count("@") >= 2) or (
"ssh://" not in vcs_url and "@" in vcs_url
):
vcs_url_parts = vcs_url.rsplit("@", 1)
if re.match(r"^[\w\.]+$", vcs_url_parts[1]):
vcs_url = vcs_url_parts[0]
fallback_ref = vcs_url_parts[1]
vcs_url, fallback_ref = normalize_vcs_url(vcs_url)
req_str = f"{vcs_url}@{_pipfile.get('ref', fallback_ref)}{extras_str}"
if not req_str.startswith(f"{vcs}+"):
req_str = f"{vcs}+{req_str}"
Expand Down
39 changes: 17 additions & 22 deletions pipenv/utils/requirements.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import os
import re
import urllib.parse
from typing import Tuple
from urllib.parse import quote, unquote

from pipenv.patched.pip._internal.network.session import PipSession
from pipenv.patched.pip._internal.req import parse_requirements
Expand Down Expand Up @@ -41,7 +41,7 @@ def redact_netloc(netloc: str) -> Tuple[str]:
else:
# If it is, leave it as is
password = ":" + password
user = urllib.parse.quote(user)
user = quote(user)
return (f"{user}{password}@{netloc}",)


Expand Down Expand Up @@ -86,7 +86,7 @@ def import_requirements(project, r=None, dev=False, categories=None):
if package.editable:
package_string = f"-e {package.link}"
else:
package_string = urllib.parse.unquote(
package_string = unquote(
redact_auth_from_url(package.original_link.url)
)

Expand Down Expand Up @@ -143,7 +143,7 @@ def add_index_to_pipfile(project, index, trusted_hosts=None):
def requirement_from_lockfile(
package_name, package_info, include_hashes=True, include_markers=True
):
from pipenv.utils.dependencies import is_editable_path, is_star
from pipenv.utils.dependencies import is_editable_path, is_star, normalize_vcs_url

# Handle string requirements
if isinstance(package_info, str):
Expand All @@ -166,36 +166,31 @@ def requirement_from_lockfile(
# Handling vcs repositories
for vcs in VCS_LIST:
if vcs in package_info:
url = package_info[vcs]
vcs_url = package_info[vcs]
ref = package_info.get("ref", "")
if ("ssh://" in url and url.count("@") >= 2) or (
"ssh://" not in url and "@" in url
):
url_parts = url.rsplit("@", 1)
# Check if the second part matches the criteria to be a ref (vcs URLs would likely have a /)
if re.match(r"^[\w\.]+$", url_parts[1]):
url = url_parts[0]
if not ref:
ref = url_parts[1]

# We have to handle the fact that some vcs urls have a ref in them
# and some have a netloc with a username and password in them, and some have both
vcs_url, fallback_ref = normalize_vcs_url(vcs_url)
if not ref:
ref = fallback_ref
extras = (
"[{}]".format(",".join(package_info.get("extras", [])))
if "extras" in package_info
else ""
)
subdirectory = package_info.get("subdirectory", "")
include_vcs = "" if f"{vcs}+" in url else f"{vcs}+"
egg_fragment = "" if "#egg=" in url else f"#egg={package_name}"
ref_str = "" if not ref or f"@{ref}" in url else f"@{ref}"
include_vcs = "" if f"{vcs}+" in vcs_url else f"{vcs}+"
egg_fragment = "" if "#egg=" in vcs_url else f"#egg={package_name}"
ref_str = "" if not ref or f"@{ref}" in vcs_url else f"@{ref}"
if (
is_editable_path(url)
or "file://" in url
is_editable_path(vcs_url)
or "file://" in vcs_url
or package_info.get("editable", False)
):
pip_line = f"-e {include_vcs}{url}{ref_str}{egg_fragment}{extras}"
pip_line = f"-e {include_vcs}{vcs_url}{ref_str}{egg_fragment}{extras}"
pip_line += f"&subdirectory={subdirectory}" if subdirectory else ""
else:
pip_line = f"{package_name}{extras}@ {include_vcs}{url}{ref_str}"
pip_line = f"{package_name}{extras}@ {include_vcs}{vcs_url}{ref_str}"
pip_line += f"#subdirectory={subdirectory}" if subdirectory else ""
return pip_line
# Handling file-sourced packages
Expand Down
63 changes: 63 additions & 0 deletions tests/integration/test_lockfile.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
from collections import defaultdict
import json
import pytest

from pipenv.project import Project
from pipenv.utils import requirements


@pytest.fixture
def pypi_lockfile():
lockfile = defaultdict(dict)
lockfile["_meta"] = {
"sources": [
{
"name": "pypi",
"url": "https://pypi.org/simple",
"verify_ssl": True,
}
]
}
yield lockfile


def test_git_branch_contains_slashes(
pipenv_instance_pypi, pypi_lockfile
):
pypi_lockfile["default"]["google-api-python-client"] = {
"git": "https://github.com/thehesiod/google-api-python-client.git@thehesiod/batch-retries2",
"markers": "python_version >= '3.7'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this test break if the branch is removed?
We should have an isolated test that does not depend on other people's repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are unit level tests but I had to do them as integration to leverage the pipfile/lockfile stuff we get from those fixtures, but I am starting to write tests that are more unit level in nature -- so we are really just testing the string conversions that happen through these constructs -- no actual lock resolution or installation happening in these tests. More like is it giving intelligible things to pip resolver or installer based on the inputs. For example, that commit hash was from a different test and not actually part of that repo.

Choose a reason for hiding this comment

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

you can use your own repo with a fake branch too right?

Choose a reason for hiding this comment

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

I don't care using mine, however I'm guessing you'll want to keep your own long term ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

These are unit level tests but I had to do them as integration to leverage the pipfile/lockfile stuff we get from those fixtures, but I am starting to write tests that are more unit level in nature -- so we are really just testing the string conversions that happen through these constructs -- no actual lock resolution or installation happening in these tests. More like is it giving intelligible things to pip resolver or installer based on the inputs. For example, that commit hash was from a different test and not actually part of that repo.

You also submit a PR with the example files to plette repo. I started a collection of test cases there.

"ref": "03803c21fc13a345e978f32775b2f2fa23c8e706"
}

with pipenv_instance_pypi() as p:
with open(p.lockfile_path, "w") as f:
json.dump(pypi_lockfile, f)

project = Project()
lockfile = project.load_lockfile(expand_env_vars=False)
deps = lockfile["default"]
pip_installable_lines = requirements.requirements_from_lockfile(
deps, include_hashes=False, include_markers=True
)
assert pip_installable_lines == ["google-api-python-client@ git+https://github.com/thehesiod/google-api-python-client.git@03803c21fc13a345e978f32775b2f2fa23c8e706"]


def test_git_branch_contains_subdirectory_fragment(pipenv_instance_pypi, pypi_lockfile):
pypi_lockfile["default"]["pep508_package"] = {
"git": "https://github.com/techalchemy/test-project.git@master",
"subdirectory": "parent_folder/pep508-package",
"ref": "03803c21fc13a345e978f32775b2f2fa23c8e706"
}

with pipenv_instance_pypi() as p:
with open(p.lockfile_path, "w") as f:
json.dump(pypi_lockfile, f)

project = Project()
lockfile = project.load_lockfile(expand_env_vars=False)
deps = lockfile["default"]
pip_installable_lines = requirements.requirements_from_lockfile(
deps, include_hashes=False, include_markers=True
)
assert pip_installable_lines == ['pep508_package@ git+https://github.com/techalchemy/test-project.git@03803c21fc13a345e978f32775b2f2fa23c8e706#subdirectory=parent_folder/pep508-package']