Avoid URL requirement name collisions #1149

geokala · 2020-05-19T18:06:11Z

This avoids cache collisions when using URLs which both end with the same filename where a later dependency depends on an earlier one, e.g.
file:///tmp/mydep1/master.zip
file:///tmp/mydep2/master.zip
(where mydep2 depends on mydep1)

Changelog-friendly one-liner: Avoid name collisions with URL based requirements.

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

atugushev

Hello @geokala,

Thanks for the PR! I've skimmed the patch and noticed that it could break the cache when hashing files. Could you check this out?
https://github.com/jazzband/pip-tools/blob/65f2ebea007ade0dd723cec6f417df1131164eb1/piptools/repositories/pypi.py#L318

codecov · 2020-05-19T18:24:16Z

Codecov Report

Merging #1149 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1149   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files          36       36           
  Lines        2752     2773   +21     
  Branches      326      328    +2     
=======================================
+ Hits         2738     2759   +21     
  Misses          8        8           
  Partials        6        6

Impacted Files	Coverage Δ
piptools/repositories/pypi.py	`95.52% <100.00%> (+0.11%)`	⬆️
tests/conftest.py	`100.00% <100.00%> (ø)`
tests/test_repository_pypi.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 0ce999b...7bb1002. Read the comment docs.

tests/test_repository_pypi.py

geokala · 2020-05-20T16:44:53Z

Re: Cache directory, good point. I've factored out the logic to a _get_download_path function and used it in both places it appears to be needed.
I've not introduced a specific test for this as I believe it constitutes internal implementation detail rather than external behaviour that should be asserted upon, but let me know if a test is required.

atugushev · 2020-05-20T18:30:42Z

piptools/repositories/pypi.py

+ collisions.
+ """
+ if ireq.link:
+ salt = hashlib.sha256(ireq.link.url_without_fragment.encode()).hexdigest()


I've researched pip codebase and have found this function. It probably makes sense to do the same for caching files. What do you think?

Sure, that looks like it'd work nicely. Only issue would be that it's depending on another pip internal function, so increases the risk of breakage every time they update.
If you're comfortable with that then I can switch it to use that this evening.

Otherwise, I can grab the nesting part of it to give the same behaviour and avoid that dependency.

What's your preference?

I would grab the nesting part.

OK, that'll be in the next push. Just looking at the test now.

Could you update _get_download_path in accordance with this function?

Can do- but what's the objective?
I can do a straight copy of that function, which will not break on changes to that function's name, but will break if there are changes to the vendored packages that pip uses. Otherwise, what's currently in this PR is a partial copy which should address the concerns but may cause slight cache bloat.
Is the preference here to reduce cache bloat but at the possible expense of stability? If so, I could change it to call that function directly?

The intention is to kill two birds with one stone. But, I don't want to block this improvement due to mysterious bugs and rather approve this as is. Thanks for your contribution!

AndydeCleyre · 2020-05-23T04:54:31Z

Can we get a nice explanation of the problem being solved, as the docstring for the test?

geokala · 2020-05-23T09:35:11Z

@AndydeCleyre Sure, I've just added that- hopefully it's not too verbose?

geokala · 2020-05-23T09:42:16Z

Not sure why CI is failing at the moment, will take a look when it deigns to give me logs.

atugushev · 2020-05-23T10:59:10Z

@geokala

Not sure why CI is failing at the moment, will take a look when it deigns to give me logs.

I've fixed it in geokala#1. You could merge this if it works.

atugushev · 2020-05-23T12:22:43Z

@geokala

FYI, also fixed test test_name_collision due to changes in geokala@2c4c571.

geokala · 2020-05-24T16:13:19Z

@atugushev Awesome, thanks for that- makes sense, so I've merged it into my PR.

geokala · 2020-05-24T16:17:48Z

Looks like CI is failing on 2.7 pip latest (I think?):
File "/tmp/easy_install-P_Otzu/jaraco.windows-5.0.0/temp/easy_install-B_XSyo/path.py-12.4.0/temp/easy_install-kjgCqJ/more-itertools-8.3.0/more_itertools/init.py", line 1, in
File "/tmp/easy_install-P_Otzu/jaraco.windows-5.0.0/temp/easy_install-B_XSyo/path.py-12.4.0/temp/easy_install-kjgCqJ/more-itertools-8.3.0/more_itertools/more.py", line 482
yield from iterable
^
SyntaxError: invalid syntax

=================================== log end ====================================
ERROR: FAIL could not package project - v = InvocationError(u'/opt/hostedtoolcache/Python/2.7.18/x64/bin/python setup.py sdist --formats=zip --dist-dir /home/runner/work/pip-tools/pip-tools/.tox/dist', 1)

Has pip already stopped supporting 2.7 for make_sdist or something?

geokala · 2020-05-24T16:24:19Z

A local run of tox --notest -p auto --parallel-live doesn't give me that error, though of course my environment doesn't match travis.

geokala · 2020-05-24T16:27:54Z

So yeah, looks like more-itertools is being used by easy_install but doesn't support python2.7 any more- I'm guessing I already had an older version of easy_install installed so didn't run into this problem.
Presumably this means that something needs pinning- easy_install or more-itertools- and presumably it wants pinning in tox?

atugushev · 2020-05-25T07:42:28Z

Unfortunately, recent setuptools-scm-4.0.0 broke our CI. Working on a fix in #1151.

atugushev · 2020-05-25T22:01:26Z

@geokala

Please rebase onto master it should be fixed now.

Without changing the dir, manual runs of pytest in the pip-tools repo will result in sdists with unexpected content Co-Authored-By: geokala <geokala@users.noreply.github.com>

geokala · 2020-05-26T07:51:51Z

Rebased. Fingers crossed.

atugushev

LGTM 👍

atugushev · 2020-05-27T07:22:55Z

@geokala

Great first-time contribution 🚀 Thanks!

geokala · 2020-05-27T16:36:03Z

No worries, and thanks for your help!

geokala mentioned this pull request May 19, 2020

Multiple entries in requirements.in with the same file cause collision #1145

Closed

atugushev reviewed May 19, 2020

View reviewed changes

atugushev changed the title ~~Avoid URL requirement name collisions (#1145)~~ Avoid URL requirement name collisions May 19, 2020

atugushev added the bug fix label May 19, 2020

atugushev reviewed May 19, 2020

View reviewed changes

tests/test_repository_pypi.py Outdated Show resolved Hide resolved

geokala force-pushed the multiple-requirements-same-name branch from 65f2ebe to ecdf92e Compare May 20, 2020 16:43

atugushev reviewed May 20, 2020

View reviewed changes

geokala force-pushed the multiple-requirements-same-name branch 2 times, most recently from dfea595 to 07fdf9a Compare May 22, 2020 16:33

geokala force-pushed the multiple-requirements-same-name branch from 07fdf9a to 9790601 Compare May 23, 2020 09:34

Madda and others added 4 commits May 26, 2020 08:50

Avoid URL requirement name collisions (jazzband#1145)

e84e1ec

Chdir to a temp package when create wheels or sdists

100b466

Without changing the dir, manual runs of pytest in the pip-tools repo will result in sdists with unexpected content Co-Authored-By: geokala <geokala@users.noreply.github.com>

Convert path to str explicitly

8124123

Fix test_name_collision

7bb1002

geokala force-pushed the multiple-requirements-same-name branch from e546c9f to 7bb1002 Compare May 26, 2020 07:51

atugushev approved these changes May 26, 2020

View reviewed changes

atugushev added this to the 5.2.0 milestone May 27, 2020

atugushev merged commit 008865a into jazzband:master May 27, 2020

geokala deleted the multiple-requirements-same-name branch May 27, 2020 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid URL requirement name collisions #1149

Avoid URL requirement name collisions #1149

geokala commented May 19, 2020 •

edited by atugushev

Loading

atugushev left a comment •

edited

Loading

codecov bot commented May 19, 2020 •

edited

Loading

geokala commented May 20, 2020

atugushev May 20, 2020

geokala May 21, 2020

atugushev May 21, 2020

geokala May 22, 2020

atugushev May 26, 2020

geokala May 26, 2020

atugushev May 26, 2020

AndydeCleyre commented May 23, 2020

geokala commented May 23, 2020

geokala commented May 23, 2020

atugushev commented May 23, 2020

atugushev commented May 23, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020 •

edited

Loading

atugushev commented May 25, 2020 •

edited

Loading

atugushev commented May 25, 2020

geokala commented May 26, 2020

atugushev left a comment

atugushev commented May 27, 2020

geokala commented May 27, 2020

Avoid URL requirement name collisions #1149

Avoid URL requirement name collisions #1149

Conversation

geokala commented May 19, 2020 • edited by atugushev Loading

Contributor checklist

atugushev left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented May 19, 2020 • edited Loading

Codecov Report

geokala commented May 20, 2020

atugushev May 20, 2020

Choose a reason for hiding this comment

geokala May 21, 2020

Choose a reason for hiding this comment

atugushev May 21, 2020

Choose a reason for hiding this comment

geokala May 22, 2020

Choose a reason for hiding this comment

atugushev May 26, 2020

Choose a reason for hiding this comment

geokala May 26, 2020

Choose a reason for hiding this comment

atugushev May 26, 2020

Choose a reason for hiding this comment

AndydeCleyre commented May 23, 2020

geokala commented May 23, 2020

geokala commented May 23, 2020

atugushev commented May 23, 2020

atugushev commented May 23, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020

geokala commented May 24, 2020 • edited Loading

atugushev commented May 25, 2020 • edited Loading

atugushev commented May 25, 2020

geokala commented May 26, 2020

atugushev left a comment

Choose a reason for hiding this comment

atugushev commented May 27, 2020

geokala commented May 27, 2020

geokala commented May 19, 2020 •

edited by atugushev

Loading

atugushev left a comment •

edited

Loading

codecov bot commented May 19, 2020 •

edited

Loading

geokala commented May 24, 2020 •

edited

Loading

atugushev commented May 25, 2020 •

edited

Loading