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

Support options in requirements.txt in pip-sync #824

Merged
merged 5 commits into from
Mar 4, 2020
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
106 changes: 84 additions & 22 deletions piptools/scripts/sync.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
# coding: utf-8
from __future__ import absolute_import, division, print_function, unicode_literals

import itertools
import os
import sys

from .. import click, sync
from .._compat import get_installed_distributions, parse_requirements
from ..exceptions import PipToolsError
from ..logging import log
from ..utils import flat_map
from ..repositories import PyPIRepository
from ..utils import create_install_command, flat_map, get_trusted_hosts

DEFAULT_REQUIREMENTS_FILE = "requirements.txt"

Expand Down Expand Up @@ -104,8 +106,15 @@ def cli(
log.error("ERROR: " + msg)
sys.exit(2)

install_command = create_install_command()
options, _ = install_command.parse_args([])
session = install_command._build_session(options)
finder = install_command._build_package_finder(options=options, session=session)

# Parse requirements file. Note, all options inside requirements file
# will be collected by the finder.
requirements = flat_map(
lambda src: parse_requirements(src, session=True), src_files
lambda src: parse_requirements(src, finder=finder, session=session), src_files
)

try:
Expand All @@ -117,26 +126,17 @@ def cli(
installed_dists = get_installed_distributions(skip=[], user_only=user_only)
to_install, to_uninstall = sync.diff(requirements, installed_dists)

install_flags = []
for link in find_links or []:
install_flags.extend(["-f", link])
if no_index:
install_flags.append("--no-index")
if index_url:
install_flags.extend(["-i", index_url])
if extra_index_url:
for extra_index in extra_index_url:
install_flags.extend(["--extra-index-url", extra_index])
if trusted_host:
for host in trusted_host:
install_flags.extend(["--trusted-host", host])
if user_only:
install_flags.append("--user")
if cert:
install_flags.extend(["--cert", cert])
if client_cert:
install_flags.extend(["--client-cert", client_cert])

install_flags = _compose_install_flags(
finder,
no_index=no_index,
index_url=index_url,
extra_index_url=extra_index_url,
trusted_host=trusted_host,
find_links=find_links,
user_only=user_only,
cert=cert,
client_cert=client_cert,
)
sys.exit(
sync.sync(
to_install,
Expand All @@ -147,3 +147,65 @@ def cli(
ask=ask,
)
)


def _compose_install_flags(
finder,
no_index=False,
index_url=None,
extra_index_url=None,
trusted_host=None,
find_links=None,
user_only=False,
cert=None,
client_cert=None,
):
"""
Compose install flags with the given finder and CLI options.
"""
result = []

# Build --index-url/--extra-index-url/--no-index
if no_index:
result.append("--no-index")
elif index_url:
result.extend(["--index-url", index_url])
elif finder.index_urls:
finder_index_url = finder.index_urls[0]
if finder_index_url != PyPIRepository.DEFAULT_INDEX_URL:
result.extend(["--index-url", finder_index_url])
for extra_index in finder.index_urls[1:]:
result.extend(["--extra-index-url", extra_index])
else:
result.append("--no-index")

for extra_index in extra_index_url or []:
result.extend(["--extra-index-url", extra_index])

# Build --trusted-hosts
for host in itertools.chain(trusted_host or [], get_trusted_hosts(finder)):
result.extend(["--trusted-host", host])

# Build --find-links
for link in itertools.chain(find_links or [], finder.find_links):
result.extend(["--find-links", link])

# Build format controls --no-binary/--only-binary
for format_control in ("no_binary", "only_binary"):
formats = getattr(finder.format_control, format_control)
if not formats:
continue
result.extend(
["--" + format_control.replace("_", "-"), ",".join(sorted(formats))]
)

if user_only:
result.append("--user")

if cert:
result.extend(["--cert", cert])

if client_cert:
result.extend(["--client-cert", client_cert])

return result
73 changes: 48 additions & 25 deletions tests/test_cli_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from .utils import invoke

from piptools.scripts.sync import cli
from piptools.scripts.sync import DEFAULT_REQUIREMENTS_FILE, cli


def test_run_as_module_sync():
Expand Down Expand Up @@ -111,42 +111,65 @@ def test_merge_error(runner):


@pytest.mark.parametrize(
("cli_flags", "expected_install_flags"),
"install_flags",
[
(["--find-links", "./libs"], ["-f", "./libs"]),
(["--no-index"], ["--no-index"]),
(["--index-url", "https://example.com"], ["-i", "https://example.com"]),
(
["--extra-index-url", "https://foo", "--extra-index-url", "https://bar"],
["--extra-index-url", "https://foo", "--extra-index-url", "https://bar"],
),
(
["--trusted-host", "https://foo", "--trusted-host", "https://bar"],
["--trusted-host", "https://foo", "--trusted-host", "https://bar"],
),
(
["--extra-index-url", "https://foo", "--trusted-host", "https://bar"],
["--extra-index-url", "https://foo", "--trusted-host", "https://bar"],
),
(["--user"], ["--user"]),
(["--cert", "foo.crt"], ["--cert", "foo.crt"]),
(["--client-cert", "foo.pem"], ["--client-cert", "foo.pem"]),
["--find-links", "./libs1", "--find-links", "./libs2"],
["--no-index"],
["--index-url", "https://example.com"],
["--extra-index-url", "https://foo", "--extra-index-url", "https://bar"],
["--trusted-host", "foo", "--trusted-host", "bar"],
["--user"],
["--cert", "foo.crt"],
["--client-cert", "foo.pem"],
],
)
@mock.patch("piptools.sync.check_call")
def test_pip_install_flags(check_call, cli_flags, expected_install_flags, runner):
def test_pip_install_flags(check_call, install_flags, runner):
"""
Test the cli flags have to be passed to the pip install command.
"""
with open("requirements.txt", "w") as req_in:
req_in.write("six==1.10.0")

runner.invoke(cli, cli_flags)
runner.invoke(cli, install_flags)

call_args = [call[0][0] for call in check_call.call_args_list]
assert [args[6:] for args in call_args if args[3] == "install"] == [
expected_install_flags
]
called_install_options = [args[6:] for args in call_args if args[3] == "install"]
assert called_install_options == [install_flags], "Called args: {}".format(
call_args
)


@pytest.mark.parametrize(
"install_flags",
[
["--no-index"],
["--index-url", "https://example.com"],
["--extra-index-url", "https://example.com"],
["--find-links", "./libs1"],
["--trusted-host", "example.com"],
["--no-binary", ":all:"],
["--only-binary", ":all:"],
],
)
@mock.patch("piptools.sync.check_call")
Copy link
Member

Choose a reason for hiding this comment

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

FTR the preferred way of patching things in pytest is to use a built-in fixture called monkeypatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there any links to the discussion about monkeypatch vs mock? I see only pytest-dev/pytest#4576 and a few others where arguments are mostly based on personal preferences. Personally, I do like @mock.patch decorator.

Would you like to open an issue so that we can discuss it and decide whether it's worth to refactor codebase form mock to monkeypatch?

Copy link
Member

Choose a reason for hiding this comment

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

I just think that it's cleaner because it's natively integrated + fewer deps.
Also, it automatically, unpatches things on exit.

Copy link
Member Author

@atugushev atugushev Oct 21, 2019

Choose a reason for hiding this comment

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

fewer deps

MagicMock would require pytest-mock btw.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's vice versa

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.. but that's not monkeypatch.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, the patching method may still be monkeypatch while the value would be MagicMock().

Copy link
Member Author

@atugushev atugushev Oct 27, 2019

Choose a reason for hiding this comment

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

@webknjaz

I just think that it's cleaner because it's natively integrated + fewer deps.

You are right, it could be the mock.MagicMock. Instead of using mock and patch things with mock, the suggestion is to use monkeypatch with mock.MagicMock. I don't get it, how it can be "fewer deps"?

Copy link
Member

Choose a reason for hiding this comment

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

It's fewer deps under py3. Also, I'm not sure I'd actually endorse magic mock, better define a proper fake object which would fit expectations instead of implicitly hiding possibly malicious behaviors.

Copy link
Member Author

@atugushev atugushev Oct 27, 2019

Choose a reason for hiding this comment

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

I see. Pretend might be an alternative to the MagickMock, btw.

def test_pip_install_flags_in_requirements_file(check_call, runner, install_flags):
"""
Test the options from requirements.txt file pass to the pip install command.
"""
with open(DEFAULT_REQUIREMENTS_FILE, "w") as reqs:
reqs.write(" ".join(install_flags) + "\n")
reqs.write("six==1.10.0")

out = runner.invoke(cli)
assert out.exit_code == 0, out

# Make sure pip install command has expected options
call_args = [call[0][0] for call in check_call.call_args_list]
called_install_options = [args[6:] for args in call_args if args[3] == "install"]
assert called_install_options == [install_flags], "Called args: {}".format(
call_args
)


@mock.patch("piptools.sync.check_call")
Expand Down