Skip to content

PEP 440 direct reference tweak unit tests #1453

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

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
abb07fc
add PEP 440 support and tests
FlorentJeannot Apr 24, 2021
210fa12
add github url dependency in fake_with_deps
FlorentJeannot Apr 25, 2021
4999357
no direct reference if egg in URL
FlorentJeannot Apr 25, 2021
dee9ddf
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Apr 27, 2021
4ac34f1
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jun 20, 2021
3ec83ce
Merge remote-tracking branch 'origin/pep440-direct-reference' into pe…
FlorentJeannot Jun 20, 2021
7f8685f
update logic for file: and update tests
FlorentJeannot Jun 21, 2021
e850781
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jun 21, 2021
d45422d
update logic to parse URL in ireq and add more tests
FlorentJeannot Jun 22, 2021
10cfb27
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jun 22, 2021
54ab1ea
add more tests in test_cli_compile.py
FlorentJeannot Jun 30, 2021
3f50330
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jun 30, 2021
cad473b
Merge branch 'master' into pep440-direct-reference
ssbarnea Jul 1, 2021
6c0d539
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jul 7, 2021
fe10214
tweak logic in format_requirement
FlorentJeannot Jul 7, 2021
c75572b
Merge remote-tracking branch 'origin/pep440-direct-reference' into pe…
FlorentJeannot Jul 7, 2021
d7aa047
Merge branch 'master' into pep440-direct-reference
FlorentJeannot Jul 16, 2021
aa50f02
tweak tests, add test for extras
FlorentJeannot Jul 17, 2021
b52eafe
fix test to work with Windows
FlorentJeannot Jul 17, 2021
5ec56ec
Merge branch 'master' into pep440-direct-reference-tweak-tests
FlorentJeannot Jul 17, 2021
aa3049f
tweak project name of small_fake_with_subdir
FlorentJeannot Jul 17, 2021
14ce893
revert .gitignore
FlorentJeannot Jul 17, 2021
4413b41
Merge branch 'master' into pep440-direct-reference-tweak-tests
atugushev Aug 2, 2021
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ coverage.xml
requirements.in
requirements.txt
.eggs/

# OS
.DS_Store
113 changes: 62 additions & 51 deletions tests/test_cli_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import shutil
import subprocess
import sys
from pathlib import Path
from textwrap import dedent
from unittest import mock

Expand Down Expand Up @@ -513,40 +512,34 @@ def test_locally_available_editable_package_is_not_archived_in_cache_dir(
@pytest.mark.parametrize(
("line", "dependency"),
(
# zip URL
# use pip-tools version prior to its use of setuptools_scm,
# which is incompatible with https: install
(
pytest.param(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added pytest.param to add an id to the tests, I think it's much nicer and easier to follow when we need to debug.

"https://github.com/jazzband/pip-tools/archive/"
"7d86c8d3ecd1faa6be11c7ddc6b29a30ffd1dae3.zip",
"\nclick==",
id="Zip URL",
),
# scm URL
(
pytest.param(
"git+git://github.com/jazzband/pip-tools@"
"7d86c8d3ecd1faa6be11c7ddc6b29a30ffd1dae3",
"\nclick==",
id="VCS URL",
),
# wheel URL
(
pytest.param(
"https://files.pythonhosted.org/packages/06/96/"
"89872db07ae70770fba97205b0737c17ef013d0d1c790"
"899c16bb8bac419/pip_tools-3.6.1-py2.py3-none-any.whl",
"\nclick==",
id="Wheel URL",
),
(
pytest.param(
"pytest-django @ git+git://github.com/pytest-dev/pytest-django"
"@21492afc88a19d4ca01cd0ac392a5325b14f95c7"
"#egg=pytest-django",
"git+git://github.com/pytest-dev/pytest-django"
"@21492afc88a19d4ca01cd0ac392a5325b14f95c7#egg=pytest-django",
),
(
"git+git://github.com/open-telemetry/opentelemetry-python.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this test and created a local one with a subdirectory inside test_data, here: https://github.com/jazzband/pip-tools/pull/1453/files#diff-99b06d953a9978ee564df2e67547b8a76f3b1ab116ab7c89a9005b9239f2a904R608

I did this to decrease the time it takes to run this test.

"@v1.3.0#subdirectory=opentelemetry-api"
"&egg=opentelemetry-api",
"git+git://github.com/open-telemetry/opentelemetry-python.git"
"@v1.3.0#subdirectory=opentelemetry-api",
id="VCS with direct reference and egg",
),
),
)
Expand All @@ -565,25 +558,23 @@ def test_url_package(runner, line, dependency, generate_hashes):
@pytest.mark.parametrize(
("line", "dependency", "rewritten_line"),
(
# file:// wheel URL
(
pytest.param(
path_to_url(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
)
),
"\nsmall-fake-a==0.1",
None,
id="Wheel URI",
),
# file:// directory
(
pytest.param(
path_to_url(os.path.join(PACKAGES_PATH, "small_fake_with_deps")),
"\nsmall-fake-a==0.1",
None,
id="Local project URI",
),
# bare path
# will be rewritten to file:// URL
(
pytest.param(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
),
Expand All @@ -593,11 +584,38 @@ def test_url_package(runner, line, dependency, generate_hashes):
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
)
),
id="Bare path to file URI",
),
pytest.param(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
),
"\nsmall-fake-with-deps @ "
+ path_to_url(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
)
),
"\nsmall-fake-with-deps @ "
+ path_to_url(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
)
),
id="Local project with absolute URI",
),
pytest.param(
path_to_url(os.path.join(PACKAGES_PATH, "small_fake_with_subdir"))
+ "#subdirectory=subdir&egg=small_fake_a",
path_to_url(os.path.join(PACKAGES_PATH, "small_fake_with_subdir"))
+ "#subdirectory=subdir&egg=small_fake_a",
None,
id="Local project with subdirectory",
),
),
)
@pytest.mark.parametrize("generate_hashes", ((True,), (False,)))
def test_local_url_package(
def test_local_file_uri_package(
pip_conf, runner, line, dependency, rewritten_line, generate_hashes
):
if rewritten_line is None:
Expand All @@ -612,38 +630,31 @@ def test_local_url_package(
assert dependency in out.stderr


@pytest.mark.parametrize(
("line", "dependency", "rewritten_line"),
(
pytest.param(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
),
"\nfile:small_fake_with_deps-0.1-py2.py3-none-any.whl",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is now a standard unit test called test_relative_file_uri_package (just below).

"file:small_fake_with_deps-0.1-py2.py3-none-any.whl",
id="Relative URL",
def test_relative_file_uri_package(pip_conf, runner):
# Copy wheel into temp dir
shutil.copy(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
),
pytest.param(
os.path.join(
MINIMAL_WHEELS_PATH, "small_fake_with_deps-0.1-py2.py3-none-any.whl"
),
"\nsmall-fake-with-deps"
" @ file://{absolute_path}/small_fake_with_deps-0.1-py2.py3-none-any.whl",
"small-fake-with-deps"
" @ file://{absolute_path}/small_fake_with_deps-0.1-py2.py3-none-any.whl",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

id="Relative URL",
),
),
)
def test_relative_url_package(pip_conf, runner, line, dependency, rewritten_line):
dependency = dependency.format(absolute_path=Path(".").absolute())
rewritten_line = rewritten_line.format(absolute_path=Path(".").absolute())
shutil.copy(line, ".")
".",
)
with open("requirements.in", "w") as req_in:
req_in.write(dependency)
req_in.write("file:small_fake_with_deps-0.1-py2.py3-none-any.whl")
out = runner.invoke(cli, ["-n", "--rebuild"])
assert out.exit_code == 0
assert rewritten_line in out.stderr
assert "file:small_fake_with_deps-0.1-py2.py3-none-any.whl" in out.stderr


def test_direct_reference_with_extras(runner):
Copy link
Contributor Author

@FlorentJeannot FlorentJeannot Jul 17, 2021

Choose a reason for hiding this comment

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

@atugushev this is the new unit test to confirm extras with direct reference is working and we don't need to add any logic to the existing codebase (I also double-checked manually by installing the version of pip-tools that contains my changes).

with open("requirements.in", "w") as req_in:
req_in.write(
"piptools[testing,coverage] @ git+https://github.com/jazzband/[email protected]"
)
out = runner.invoke(cli, ["-n", "--rebuild"])
assert out.exit_code == 0
assert "pip-tools @ git+https://github.com/jazzband/[email protected]" in out.stderr
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be extras included in the package name?

Suggested change
assert "pip-tools @ git+https://github.com/jazzband/[email protected]" in out.stderr
assert "pip-tools[testing,coverage] @ git+https://github.com/jazzband/[email protected]" in out.stderr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, basically at some point it takes into account the extras that is passed in the .in like so:

❯ cat requirements.in
uvicorn[standard] @ file:///Users/florent/Desktop/fastapi/uvicorn-0.14.0-py3-none-any.whl

And so it produces the .txt with the extras:

❯ cat requirements.txt
#
# This file is autogenerated by pip-compile with python 3.9
# To update, run:
#
#    pip-compile
#
asgiref==3.4.1
    # via uvicorn
click==8.0.1
    # via uvicorn
h11==0.12.0
    # via uvicorn
httptools==0.2.0
    # via uvicorn
python-dotenv==0.18.0
    # via uvicorn
pyyaml==5.4.1
    # via uvicorn
uvicorn @ file:///Users/florent/Desktop/fastapi/uvicorn-0.14.0-py3-none-any.whl
    # via -r requirements.in
uvloop==0.15.3
    # via uvicorn
watchgod==0.7
    # via uvicorn
websockets==9.1
    # via uvicorn

If I remove the extras from the .in, it gives this:

❯ pip-compile
#
# This file is autogenerated by pip-compile with python 3.9
# To update, run:
#
#    pip-compile
#
asgiref==3.4.1
    # via uvicorn
click==8.0.1
    # via uvicorn
h11==0.12.0
    # via uvicorn
uvicorn @ file:///Users/florent/Desktop/fastapi/uvicorn-0.14.0-py3-none-any.whl
    # via -r requirements.in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another example is the ouput of poetry:

from:

pip-tools = {git = "https://github.com/FlorentJeannot/pip-tools.git", rev = "master", extras=["testing"]}

the requirements.txt contains:

pip-tools @ git+https://github.com/FlorentJeannot/pip-tools.git@master ; python_version >= "3.6"

not:

pip-tools[testing] @ git+https://github.com/FlorentJeannot/pip-tools.git@master ; python_version >= "3.6"

So the current behavior of pip-tools seems fine to me. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And from pip:

❯ pip install 'git+https://github.com/encode/[email protected]#egg=uvicorn[standard]'

pip freeze:

asgiref==3.4.1
click==8.0.1
h11==0.12.0
httptools==0.2.0
python-dotenv==0.18.0
PyYAML==5.4.1
uvicorn @ git+https://github.com/encode/uvicorn.git@87da6cf4082cd28306bdb78d7d42552d131cc179
uvloop==0.15.3
watchgod==0.7
websockets==9.1

So unless the specification was not implemented correctly in the other tools, I think we're fine?

Copy link
Contributor Author

@FlorentJeannot FlorentJeannot Jul 17, 2021

Choose a reason for hiding this comment

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

Also if we specify the extras in the requirements.txt and we have the extras of a package listed in the file as well, I think it's redundant?

Like for uvicorn if you add [standard] it's going to install uvloop and a bunch of other packages.

So if you have:

uvicorn[standard] @ git+https://github.com/encode/uvicorn.git@87da6cf4082cd28306bdb78d7d42552d131cc179
uvloop==0.15.3

I think it would mean when it installs uvicorn, then it also installs uvloop. And then when it wants to install uvloop==0.15.3 I think it's going to say it's already installed.

Copy link
Contributor Author

@FlorentJeannot FlorentJeannot Jul 17, 2021

Choose a reason for hiding this comment

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

In case you have specified coverage[toml] in requirements.in and you have already generated the requirements.txt (which outputted toml as dependency), I think as long as you don't do pip-compile -U, it's going to keep the version that has been pinned in the requirements.txt.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sure, but I'm talking about a mixed workflow. Does pip actually store the extras you select anywhere?

Copy link
Contributor Author

@FlorentJeannot FlorentJeannot Jul 17, 2021

Choose a reason for hiding this comment

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

I am not sure to understand your workflow so I'll try to answer as best I can.

If I install coverage with pip it does the following:

❯ pip install 'coverage[toml] @ git+https://github.com/nedbat/[email protected]'
[...]

❯ pip freeze
coverage @ git+https://github.com/nedbat/coveragepy.git@b854e6103efd49a2c72438db55e99273ef5fc52e
toml==0.10.2

Then if I upgrade:

❯ pip install -U 'coverage[toml] @ git+https://github.com/nedbat/coveragepy.git@master'
[...]

❯ pip freeze
coverage @ git+https://github.com/nedbat/coveragepy.git@c0da97eb03d4ffe8be8854ad6ff1a2736f169003
toml==0.10.2
tomli==1.0.4

By the way you can see that pip does not display the extras for coverage which makes me think we should not do this in the requirements.txt when we run pip-compile. Also, same if the input is just coverage[toml]:

❯ pip install 'coverage[toml]'
[...]

❯ pip freeze
coverage==5.5
toml==0.10.2

Please let me know if I answered your question, otherwise could you give me more details about your workflow?
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's what I feared. The fact that pip-compile left the extras in made me think they were important

Copy link
Contributor Author

@FlorentJeannot FlorentJeannot Jul 17, 2021

Choose a reason for hiding this comment

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

Yep, that's what I feared. The fact that pip-compile left the extras in made me think they were important

Yeah I think pip-compile is doing it wrong actually. pip, pipenv and poetry are not showing the extras in the .txt file when you generate it. I really think we should fix this in pip-tools and do it like the other tools.

EDIT:
@graingert Sorry actually pipenv shows the extras when you specify coverage[toml] 🤔

#
# These requirements were autogenerated by pipenv
# To regenerate from the project's Pipfile, run:
#
#    pipenv lock --requirements
#

-i https://pypi.org/simple
coverage[toml]==5.5
toml==0.10.2; python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2'

But it's broken when you do pipenv install 'coverage[toml] @ git+https://github.com/nedbat/coveragepy.git@master'.

pip freeze only contains this after installation:

coverage @ git+https://github.com/nedbat/coveragepy.git@c0da97eb03d4ffe8be8854ad6ff1a2736f169003

and the requirements.txt is missing toml:

#
# These requirements were autogenerated by pipenv
# To regenerate from the project's Pipfile, run:
#
#    pipenv lock --requirements
#

-i https://pypi.org/simple
git+https://github.com/nedbat/coveragepy.git@c0da97eb03d4ffe8be8854ad6ff1a2736f169003#egg=coverage[toml]

The Pipfile is right though:

coverage = {extras = ["toml"], ref = "master", git = "https://github.com/nedbat/coveragepy.git"}

Well... I am seeing so many issues with pipenv that I don't know why I even mentioned it in this thead.
I think we should follow how pip is doing things anyway.

assert "pytest==" in out.stderr
assert "pytest-cov==" in out.stderr


def test_input_file_without_extension(pip_conf, runner):
Expand Down
1 change: 0 additions & 1 deletion tests/test_data/packages/fake_with_deps/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,5 @@
"SQLAlchemy!=0.9.5,<2.0.0,>=0.7.8,>=1.0.0",
"python-memcached>=1.57,<2.0",
"xmltodict<=0.11,>=0.4.6",
"requests @ git+git://github.com/psf/[email protected]",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this line since it seemed to be useless IMO. I added it when I started working on my first PR in pip-tools for PEP 440 support and I had not yet written all the tests above.

],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from setuptools import setup

setup(name="small_fake_a", version=0.1)