PEP 440 Direct Reference support #1392

FlorentJeannot · 2021-04-25T01:03:46Z

This PR adds PEP 440 Direct References support to fix #1391.

pip can support direct references, so the egg can be omitted inside the URL (see the code from pip).

In my projects which have git repositories specified as dependencies, without a direct reference or an egg, pip was not able to determine the package name, which was causing an issue during pip-sync (inside the merge, comparing two identical requirements from two different files).

Also, inside setup.py, install_requires must be written with a direct reference (pip fails to install the dependencies if it is not using a direct reference). pip-tools was ignoring the direct reference, so it was not generating the requirements.txt correctly. See my example:

# setup.py
install_requires=["package @ git+ssh://url@version"]

# requirements.txt (from pip-tools)
git+ssh://url@version

Changelog-friendly one-liner: PEP 440 Direct References support

Contributor checklist

Provided the tests for the changes.
Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

FlorentJeannot · 2021-04-25T19:32:39Z

FYI I installed pip-tools in my project using my fork and I generated and compared the requirements.txt and dev-requirements.txt files with the new ones:

there was no regression
the direct references were generated when it was only needed
one sub-dependency generated a URL with an egg, without direct reference (which is fine: it keeps the original behavior of the tool)

And pip-sync worked (it was not without this fix; see #1391)

codecov · 2021-04-27T04:15:56Z

Codecov Report

Merging #1392 (4999357) into master (e8982fb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4999357 differs from pull request most recent head dee9ddf. Consider uploading reports for the commit dee9ddf to get more accurate results

@@           Coverage Diff           @@
##           master    #1392   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          33       33           
  Lines        3037     3044    +7     
  Branches      327      330    +3     
=======================================
+ Hits         3027     3034    +7     
  Misses          5        5           
  Partials        5        5

Impacted Files	Coverage Δ
tests/test_cli_compile.py	`100.00% <ø> (ø)`
piptools/utils.py	`100.00% <100.00%> (ø)`
tests/test_utils.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8982fb...dee9ddf. Read the comment docs.

FlorentJeannot · 2021-05-02T14:57:51Z

To help you understand this fix/enhancement, from the master branch, try the following tests:

Test 1

In pip-tools/tests/test_data/packages/fake_with_deps/setup.py, add in install_requires:

"requests @ git+git://github.com/psf/[email protected]"

In test_realistic_complex_sub_dependencies, add the following at the end:

print(out.stderr)
assert "requests @ git+git://github.com/psf/[email protected]" in out.stderr

This test will fail because it is missing the direct reference, inside the output there is:

git+git://github.com/psf/[email protected]

Also, note that there is no egg specified.

Test 2

In pip-tools/tests/test_resolver.py, add the following:

def test_combine_install_requirements_direct_ref(repository, from_line):
    first_dep = from_line("git+ssh://****@bitbucket.org/private-bucket/[email protected]", comes_from="-r requirements.in")
    second_dep = from_line("git+ssh://****@bitbucket.org/private-bucket/[email protected]", comes_from="-r dev-requirements.in")

    combined = combine_install_requirements(repository, [first_dep, second_dep])
    assert combined.comes_from == first_dep.comes_from  # shortest string
    assert set(combined._source_ireqs) == {first_dep, second_dep}
    assert str(combined.req.specifier) == ""

This is failing because both git URLs are missing a specifier. You can make the test pass by specifying a direct reference or an egg:

enums @ git+ssh://****@bitbucket.org/private-bucket/[email protected]
git+ssh://****@bitbucket.org/private-bucket/[email protected]#egg=enums

For example this works:

def test_combine_install_requirements_direct_ref(repository, from_line):
    first_dep = from_line("enums @ git+ssh://****@bitbucket.org/private-bucket/[email protected]", comes_from="-r requirements.in")
    second_dep = from_line("git+ssh://****@bitbucket.org/private-bucket/[email protected]#egg=enums", comes_from="-r dev-requirements.in")

    combined = combine_install_requirements(repository, [first_dep, second_dep])
    assert combined.comes_from == first_dep.comes_from  # shortest string
    assert set(combined._source_ireqs) == {first_dep, second_dep}
    assert str(combined.req.specifier) == ""

Conclusion

Test 1 does not generate the requirement with the direct reference, even though it was specified in install_requires
Test 2 fails if a specifier (a direct reference or an egg) is missing

Because pip-tools currently does not handle direct references:

when requirements.in contains a direct reference and dev-requirements.in contains the same direct reference
then pip-compile generates these files without their direct references
and pip-sync requirements.txt dev-requirements.txt fails when the resolver enters combine_install_requirements

I hope this helps to make it clearer and I am looking forward to reading your feedback!

tests/test_data/packages/fake_with_deps/setup.py

AndydeCleyre · 2021-05-07T20:09:55Z

Hey, you reached out last week over at #1329, and today I was able to start looking at this. It's possible that, aside from changes to tests, all that's needed on top of my WIP PR is:

diff --git a/piptools/utils.py b/piptools/utils.py
index 4f15729..2f81af1 100644
--- a/piptools/utils.py
+++ b/piptools/utils.py
@@ -1,6 +1,7 @@
 import collections
 import itertools
 import os
+import re
 import shlex
 from contextlib import contextmanager, suppress
 from typing import (
@@ -137,6 +138,8 @@ def format_requirement(
                         ireq.local_file_path, from_dir
                     ).replace(os.path.sep, "/")
             ireq_url = f"{path_url}{fragment_string(ireq)}"
+        if not re.match(r"^#(|.*&)egg=.*", fragment_string(ireq)) and ireq.name:
+            ireq_url = f"{ireq.name} @ {ireq_url}"
         line = f"-e {ireq_url}" if ireq.editable else ireq_url
 
     if marker:

I'll leave this diff right here, and if #1329 is ever done this will help me merge.

FlorentJeannot · 2021-05-07T20:33:14Z

@AndydeCleyre Thanks for your suggestion. Yeah it looks like that would work, the tests I've added will help you find out. :)

luzfcb · 2021-06-17T15:28:21Z

@FlorentJeannot I guess that's necessary to include these four test cases to ensure this continues to work in the future. Anyway looks good to me. (A github archive zip or tar.gz with and without the archive hash value)

The output should be the same as the input:

django-debug-toolbar@https://github.com/jazzband/django-debug-toolbar/archive/6ec6bfe0ba5912d7fd0a2b98176a215847157696.zip

django-taggit@https://github.com/jazzband/django-taggit/archive/74460fdc4a8b8bf2d36fb8a59691403cde7b4528.zip#sha1=594b7dd32bec37d8bf70a6ffa8866d30e93f3c42

…p440-direct-reference

FlorentJeannot · 2021-06-20T20:36:43Z

Hey @luzfcb, this PR would definitely fix your issue! :)

Concerning your suggestion, I think the existing tests are enough. Could you explain your point of view?

@atugushev Do you know when this PR will be reviewed?

Thanks!

piptools/utils.py

tests/test_utils.py

piptools/utils.py

…p440-direct-reference

atugushev

Thanks for addressing suggestions 👍

As a side note, this transformation feels confusing:

example @ https://example.com/example.zip#egg=example
→
https://example.com/example.zip#egg=example

I'd prefer the opposed instead:

https://example.com/example.zip#egg=example
→
example @ https://example.com/example.zip

AFAIK egg fragment appears to be discouraged for direct references. However, pip is going to refactor this within pypa/pip#1289, so it would make sense to address that later, as soon as the issue is resolved.

FlorentJeannot · 2021-07-07T20:25:24Z

Thanks for addressing suggestions 👍

As a side note, this transformation feels confusing:
example @ https://example.com/example.zip#egg=example
→
https://example.com/example.zip#egg=example
I'd prefer the opposed instead:
https://example.com/example.zip#egg=example
→
example @ https://example.com/example.zip
AFAIK egg fragment appears to be discouraged for direct references. However, pip is going to refactor this within pypa/pip#1289, so it would make sense to address that later, as soon as the issue is resolved.

Hello @atugushev,

Thank you for your feedback and your approval for this PR.
I suggest to mention "PEP 508 support" in the CHANGELOG.md instead of PEP 440 as it seems to be more appropriate.

Regarding your side note, I agree with you and I'll happily provide a PR for this when pypa/pip#1289 will be resolved.

atugushev · 2021-07-14T17:03:14Z

@FlorentJeannot

We should also support extras, see #1450 for details. Would you like to address it here or in a following up PR?

FlorentJeannot · 2021-07-14T18:47:47Z

@FlorentJeannot

We should also support extras, see #1450 for details. Would you like to address it here or in a following up PR?

@atugushev

I will do it in another PR since this one has been approved.

Also I have another question. It seems that pip already converts URLs to the PEP 508 format. See https://github.com/pypa/pip/blob/7da05f33345de4c59930398aefa011ed7ccdeca3/src/pip/_vendor/packaging/requirements.py#L148
Is there a good reason to wait for pypa/pip#1289 or could we do it like them now?

atugushev · 2021-07-17T01:27:07Z

@FlorentJeannot

Thanks for the PR! Awesome job 👍🏻

Is there a good reason to wait for pypa/pip#1289 or could we do it like them now?

Could you elaborate on how code in Requirement.__str__() related to egg fragment refactoring in pypa/pip#1289?

FlorentJeannot · 2021-07-17T01:55:11Z

@atugushev Thanks 👍 Happy to contribute to this project 😄

So I believe Requirement.__str__() is what is used to format the requirement in pip. I think that's the equivalent to the function I modified in piptools to support the direct reference.
In their code they seem to always convert requirements to the direct reference format when they have a URL with a name.

So for example if you do this:
pip install 'git+https://github.com/encode/[email protected]#egg=uvicorn[standard]'

Even though it was specified with an egg, the output of pip freeze will be a direct reference for uvicorn (which makes me wonder if we shouldn't do like them already):

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

atugushev · 2021-07-17T02:10:53Z

@FlorentJeannot

if we shouldn't do like them already

Ideally, we should. Feel free to shoot a PR if you think it's doable.

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

@FlorentJeannot

if we shouldn't do like them already

Ideally, we should. Feel free to shoot a PR if you think it's doable.

Will do!

Edit: @atugushev Done! #1455

FlorentJeannot added 2 commits April 25, 2021 01:45

add PEP 440 support and tests

abb07fc

add github url dependency in fake_with_deps

210fa12

FlorentJeannot changed the title ~~add PEP 440 support and tests~~ add PEP 440 Direct Reference support Apr 25, 2021

FlorentJeannot changed the title ~~add PEP 440 Direct Reference support~~ PEP 440 Direct Reference support Apr 25, 2021

no direct reference if egg in URL

4999357

Merge branch 'master' into pep440-direct-reference

dee9ddf

FlorentJeannot mentioned this pull request May 2, 2021

Improve instructions for new contributors #1394

Merged

3 tasks

FlorentJeannot commented May 2, 2021

View reviewed changes

tests/test_data/packages/fake_with_deps/setup.py Show resolved Hide resolved

atugushev mentioned this pull request Jun 11, 2021

pip-compile generates unexpected requirement format (PEP 508 support) #1418

Closed

atugushev added enhancement Improvements to functionality writer Related to results output writer component labels Jun 11, 2021

FlorentJeannot added 2 commits June 20, 2021 21:46

Merge branch 'master' into pep440-direct-reference

4ac34f1

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

3ec83ce

…p440-direct-reference

atugushev reviewed Jun 21, 2021

View reviewed changes

piptools/utils.py Outdated Show resolved Hide resolved

atugushev added the vcs Related to VCS requirements label Jun 21, 2021

FlorentJeannot added 2 commits June 21, 2021 23:19

update logic for file: and update tests

7f8685f

Merge branch 'master' into pep440-direct-reference

e850781

atugushev reviewed Jun 22, 2021

View reviewed changes

piptools/utils.py Outdated Show resolved Hide resolved

atugushev reviewed Jun 22, 2021

View reviewed changes

tests/test_utils.py Outdated Show resolved Hide resolved

FlorentJeannot added 4 commits June 23, 2021 00:59

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

atugushev reviewed Jul 7, 2021

View reviewed changes

piptools/utils.py Outdated Show resolved Hide resolved

atugushev reviewed Jul 7, 2021

View reviewed changes

piptools/utils.py Outdated Show resolved Hide resolved

FlorentJeannot added 3 commits July 7, 2021 06:46

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

atugushev approved these changes Jul 7, 2021

View reviewed changes

atugushev mentioned this pull request Jul 14, 2021

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

Closed

Merge branch 'master' into pep440-direct-reference

d7aa047

atugushev merged commit 53c9272 into jazzband:master Jul 17, 2021

FlorentJeannot deleted the pep440-direct-reference branch July 17, 2021 02:27

atugushev added this to the 6.3.0 milestone Jul 17, 2021

FlorentJeannot mentioned this pull request Jul 17, 2021

Generate PEP 440 direct reference whenever possible #1455

Merged

4 tasks

Eric-Arellano mentioned this pull request Jul 30, 2021

Lockfiles: Should we use pip-compile or teach Pex to generate lockfiles? pantsbuild/pants#12470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP 440 Direct Reference support #1392

PEP 440 Direct Reference support #1392

FlorentJeannot commented Apr 25, 2021 •

edited by atugushev

Loading

FlorentJeannot commented Apr 25, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

FlorentJeannot commented May 2, 2021 •

edited

Loading

AndydeCleyre commented May 7, 2021

FlorentJeannot commented May 7, 2021

luzfcb commented Jun 17, 2021 •

edited

Loading

FlorentJeannot commented Jun 20, 2021

atugushev left a comment

FlorentJeannot commented Jul 7, 2021

atugushev commented Jul 14, 2021

FlorentJeannot commented Jul 14, 2021

atugushev commented Jul 17, 2021

FlorentJeannot commented Jul 17, 2021

atugushev commented Jul 17, 2021 •

edited

Loading

FlorentJeannot commented Jul 17, 2021 •

edited

Loading

PEP 440 Direct Reference support #1392

PEP 440 Direct Reference support #1392

Conversation

FlorentJeannot commented Apr 25, 2021 • edited by atugushev Loading

Contributor checklist

FlorentJeannot commented Apr 25, 2021 • edited Loading

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

FlorentJeannot commented May 2, 2021 • edited Loading

Test 1

Test 2

Conclusion

AndydeCleyre commented May 7, 2021

FlorentJeannot commented May 7, 2021

luzfcb commented Jun 17, 2021 • edited Loading

FlorentJeannot commented Jun 20, 2021

atugushev left a comment

Choose a reason for hiding this comment

FlorentJeannot commented Jul 7, 2021

atugushev commented Jul 14, 2021

FlorentJeannot commented Jul 14, 2021

atugushev commented Jul 17, 2021

FlorentJeannot commented Jul 17, 2021

atugushev commented Jul 17, 2021 • edited Loading

FlorentJeannot commented Jul 17, 2021 • edited Loading

FlorentJeannot commented Apr 25, 2021 •

edited by atugushev

Loading

FlorentJeannot commented Apr 25, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

FlorentJeannot commented May 2, 2021 •

edited

Loading

luzfcb commented Jun 17, 2021 •

edited

Loading

atugushev commented Jul 17, 2021 •

edited

Loading

FlorentJeannot commented Jul 17, 2021 •

edited

Loading