PEP 440 direct reference tweak unit tests #1453

FlorentJeannot · 2021-07-17T01:58:29Z

Changes

Add test for direct reference with extras
Tweak existing tests

Contributor checklist

Provided the tests for the changes.
Assure PR title is short, clear, and good to be included in the user-oriented changelog
Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

…p440-direct-reference

# Conflicts: # tests/test_cli_compile.py

.gitignore

FlorentJeannot · 2021-07-17T02:03:11Z

tests/test_cli_compile.py

            "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"


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.

FlorentJeannot · 2021-07-17T02:04:46Z

tests/test_cli_compile.py

        # use pip-tools version prior to its use of setuptools_scm,
        # which is incompatible with https: install
-        (
+        pytest.param(


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.

FlorentJeannot · 2021-07-17T02:06:16Z

tests/test_cli_compile.py

-            "\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",


This one has been moved here: https://github.com/jazzband/pip-tools/pull/1453/files#diff-99b06d953a9978ee564df2e67547b8a76f3b1ab116ab7c89a9005b9239f2a904R590

FlorentJeannot · 2021-07-17T02:07:34Z

tests/test_cli_compile.py

-            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",


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

FlorentJeannot · 2021-07-17T02:08:34Z

tests/test_cli_compile.py

+    assert "file:small_fake_with_deps-0.1-py2.py3-none-any.whl" in out.stderr
+
+
+def test_direct_reference_with_extras(runner):


@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).

FlorentJeannot · 2021-07-17T02:11:34Z

tests/test_data/packages/fake_with_deps/setup.py

@@ -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/requests@v2.25.1",


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.

atugushev · 2021-07-17T02:30:46Z

tests/test_cli_compile.py

+        )
+    out = runner.invoke(cli, ["-n", "--rebuild"])
+    assert out.exit_code == 0
+    assert "pip-tools @ git+https://github.com/jazzband/pip-tools@6.2.0" in out.stderr


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

Suggested change

assert "pip-tools @ git+https://github.com/jazzband/pip-tools@6.2.0" in out.stderr

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

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

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. 🤔

And from pip:

❯ pip install 'git+https://github.com/encode/uvicorn.git@0.14.0#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?

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.

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.

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

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/coveragepy.git@coverage-5.1' [...] ❯ 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!

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

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.

atugushev

LGTM 👍

FlorentJeannot and others added 20 commits April 25, 2021 01:45

add PEP 440 support and tests

abb07fc

add github url dependency in fake_with_deps

210fa12

no direct reference if egg in URL

4999357

Merge branch 'master' into pep440-direct-reference

dee9ddf

Merge branch 'master' into pep440-direct-reference

4ac34f1

Merge remote-tracking branch 'origin/pep440-direct-reference' into pe…

3ec83ce

…p440-direct-reference

update logic for file: and update tests

7f8685f

Merge branch 'master' into pep440-direct-reference

e850781

update logic to parse URL in ireq and add more tests

d45422d

Merge branch 'master' into pep440-direct-reference

10cfb27

add more tests in test_cli_compile.py

54ab1ea

Merge branch 'master' into pep440-direct-reference

3f50330

Merge branch 'master' into pep440-direct-reference

cad473b

Merge branch 'master' into pep440-direct-reference

6c0d539

tweak logic in format_requirement

fe10214

Merge remote-tracking branch 'origin/pep440-direct-reference' into pe…

c75572b

…p440-direct-reference

Merge branch 'master' into pep440-direct-reference

d7aa047

tweak tests, add test for extras

aa50f02

fix test to work with Windows

b52eafe

Merge branch 'master' into pep440-direct-reference-tweak-tests

5ec56ec

# Conflicts: # tests/test_cli_compile.py

FlorentJeannot changed the title ~~Pep440 direct reference tweak unit tests~~ PEP 440 direct reference tweak unit tests Jul 17, 2021

FlorentJeannot commented Jul 17, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

FlorentJeannot commented Jul 17, 2021

View reviewed changes

FlorentJeannot mentioned this pull request Jul 17, 2021

pep440 Direct References don't include extras in requirements.txt #1450

Closed

tweak project name of small_fake_with_subdir

aa3049f

atugushev reviewed Jul 17, 2021

View reviewed changes

atugushev added tests Testing and related things skip-changelog Avoid listing in changelog labels Jul 17, 2021

revert .gitignore

14ce893

atugushev approved these changes Aug 2, 2021

View reviewed changes

Merge branch 'master' into pep440-direct-reference-tweak-tests

4413b41

atugushev merged commit 5c9df27 into jazzband:master Aug 2, 2021

This was referenced Feb 20, 2022

Direct references show extra requirements in .txt files #1582

Merged

Extra requirements are lost in pip-compile comments #1577

Open

FlorentJeannot mentioned this pull request Apr 3, 2022

Remove extras from compiled file #1608

Closed

4 tasks

FlorentJeannot mentioned this pull request Apr 11, 2022

Always remove extras in compiled files #1613

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP 440 direct reference tweak unit tests #1453

PEP 440 direct reference tweak unit tests #1453

FlorentJeannot commented Jul 17, 2021 •

edited by atugushev

Loading

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021

atugushev Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading

graingert Jul 17, 2021

FlorentJeannot Jul 17, 2021 •

edited

Loading

graingert Jul 17, 2021

FlorentJeannot Jul 17, 2021 •

edited

Loading

atugushev left a comment

		assert "file:small_fake_with_deps-0.1-py2.py3-none-any.whl" in out.stderr


		def test_direct_reference_with_extras(runner):

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

PEP 440 direct reference tweak unit tests #1453

PEP 440 direct reference tweak unit tests #1453

Conversation

FlorentJeannot commented Jul 17, 2021 • edited by atugushev Loading

Changes

Contributor checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorentJeannot Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorentJeannot Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

FlorentJeannot Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorentJeannot Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorentJeannot Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

atugushev left a comment

Choose a reason for hiding this comment

FlorentJeannot commented Jul 17, 2021 •

edited by atugushev

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading

FlorentJeannot Jul 17, 2021 •

edited

Loading