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 URLs as packages #807

Merged
merged 2 commits into from
May 8, 2019
Merged

Conversation

jcushman
Copy link
Contributor

@jcushman jcushman commented May 3, 2019

Fix #272

This PR handles URLs in requirements.in, building on work by @nim65s and @toejough.

This is similar to the previous PR #764, but I rebased @toejough's latest on top of master, updated the PyPI download url and some tests, and added support for --generate-hashes.

Note that URL requirements don't work for packages that use setuptools_scm, including pip-tools itself, but that's an inherent limitation of URL requirements and not an issue with pip-compile.

Changelog-friendly one-liner: Support URLs as packages

Contributor checklist
  • Provided the tests for the changes.
  • Requested a review from another contributor.
  • 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).

@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #807 into master will increase coverage by 0.01%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
+ Coverage   98.82%   98.83%   +0.01%     
==========================================
  Files          36       36              
  Lines        2119     2147      +28     
  Branches      275      278       +3     
==========================================
+ Hits         2094     2122      +28     
  Misses         15       15              
  Partials       10       10
Impacted Files Coverage Δ
piptools/repositories/base.py 100% <ø> (ø) ⬆️
tests/test_resolver.py 100% <ø> (ø) ⬆️
piptools/exceptions.py 86.11% <ø> (-2.27%) ⬇️
piptools/resolver.py 100% <100%> (ø) ⬆️
piptools/utils.py 100% <100%> (ø) ⬆️
piptools/scripts/compile.py 100% <100%> (ø) ⬆️
piptools/writer.py 100% <100%> (ø) ⬆️
tests/test_utils.py 100% <100%> (ø) ⬆️
tests/test_cli_compile.py 100% <100%> (ø) ⬆️
tests/test_sync.py 100% <100%> (ø) ⬆️
... and 2 more

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 990a3d0...de874f4. Read the comment docs.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

it would be great if you could increase coverage

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

So many tries to implement this feature, hope this will be eventually merged. Thank you for great work! Have minor comments. And it would be nice not to decrease the test coverage.

piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/repositories/pypi.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
@atugushev atugushev added enhancement Improvements to functionality non-editable VCS/URL labels May 4, 2019
@auvipy auvipy added this to the 3.7.0 milestone May 4, 2019
@blueyed
Copy link
Contributor

blueyed commented May 4, 2019

Thanks for working on this!

Tried this quickly, and it fails when not using #egg=foo at the end:

Current constraints:
  git+https://github.com/blueyed/starlette.git@local

Finding the best candidates:
  found candidate git+https://github.com/blueyed/starlette.git@local (constraint was <any>)

Finding secondary dependencies:
------------------------------------------------------------
Result of round 1: stable, done

None
Traceback (most recent call last):
  File "…/Vcs/pip-tools/.venv/bin/pip-compile", line 11, in <module>
    load_entry_point('pip-tools', 'console_scripts', 'pip-compile')()
  File "…/Vcs/pip-tools/.venv/lib/python3.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "…/Vcs/pip-tools/.venv/lib/python3.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "…/Vcs/pip-tools/.venv/lib/python3.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "…/Vcs/pip-tools/.venv/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "…/Vcs/pip-tools/.venv/lib/python3.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "…/Vcs/pip-tools/piptools/scripts/compile.py", line 415, in cli
    key_from_req(ireq.req) for ireq in constraints if not ireq.constraint
  File "…/Vcs/pip-tools/piptools/scripts/compile.py", line 415, in <setcomp>
    key_from_req(ireq.req) for ireq in constraints if not ireq.constraint
  File "…/Vcs/pip-tools/piptools/utils.py", line 30, in key_from_req
    key = req.name
AttributeError: 'NoneType' object has no attribute 'name'

Used git+https://github.com/blueyed/starlette.git@local - with git+https://github.com/blueyed/starlette.git@local#egg=starlette it worked, but just left it as-is - is that expected?

@jcushman jcushman force-pushed the url-requirements branch 2 times, most recently from 5d59167 to 3263535 Compare May 5, 2019 02:03
@jcushman
Copy link
Contributor Author

jcushman commented May 5, 2019

Thanks for the reviews! I believe my updates address all the comments so far. I was able to greatly simplify the is_url_requirement function, which was the messiest part of the logic.

@blueyed:

it fails when not using #egg=foo at the end

Good catch -- this should be fixed now.

with git+https://github.com/blueyed/starlette.git@local#egg=starlette it worked, but just left it as-is - is that expected?

I think so, yes -- it should just copy URLs directly to requirements.txt, along with their deps. Looks like starlette doesn't have any dependencies so you'd just see the original URL.

@jcushman jcushman requested a review from atugushev May 5, 2019 02:30
@blueyed
Copy link
Contributor

blueyed commented May 5, 2019

it should just copy URLs directly to requirements.txt

Shouldn't it pin it at the current commit? (that's what I did in #169 back then)

@jcushman jcushman force-pushed the url-requirements branch from ce26f39 to 8e68a18 Compare May 5, 2019 16:12
@jcushman
Copy link
Contributor Author

jcushman commented May 5, 2019

Update: the code I was adapting only applied to http/scm URLs and excluded file URLs, but I think file URLs actually work just fine. The latest commit removes the exclusion and adds tests for file URLs. This also lets us drop the whole UnsupportedConstraint codepath because we're no longer excluding anything as unsupported.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can we skip the pip master failures and merged this as is?

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

I've noticed that pip-sync would try to sync URL package every time.

$ pip-compile
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile
#
https://github.com/pallets/click/archive/7.0.zip

$ pip-sync
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Collecting https://github.com/pallets/click/archive/7.0.zip (from -r /var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/tmp7O5s_H (line 1))
  Downloading https://github.com/pallets/click/archive/7.0.zip
     - 1.3MB 2.6MB/s
Building wheels for collected packages: Click
  Building wheel for Click (setup.py) ... done
  Stored in directory: /private/var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/pip-ephem-wheel-cache-1eHUT2/wheels/3f/4c/b1/bb74f5eeb10363c51faaaa45211c996886de6beb3fbd804067
Successfully built Click
Installing collected packages: Click
  Found existing installation: click 6.7
    Uninstalling click-6.7:
      Successfully uninstalled click-6.7
Successfully installed Click-7.0
 
$ pip-sync
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Collecting https://github.com/pallets/click/archive/7.0.zip (from -r /var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/tmpW22iN9 (line 1))
  Downloading https://github.com/pallets/click/archive/7.0.zip
     - 624kB 2.2MB/s
Requirement already satisfied (use --upgrade to upgrade): Click==7.0 from https://github.com/pallets/click/archive/7.0.zip in ./.venv/lib/python2.7/site-packages (from -r /var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/tmpW22iN9 (line 1))
Building wheels for collected packages: Click
  Building wheel for Click (setup.py) ... done
  Stored in directory: /private/var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/pip-ephem-wheel-cache-oY4xDG/wheels/3f/4c/b1/bb74f5eeb10363c51faaaa45211c996886de6beb3fbd804067
Successfully built Click

On the second pip-sync it's only built wheels and warned that Requirement already satisfied.

@jcushman
Copy link
Contributor Author

jcushman commented May 5, 2019

Shouldn't it pin it at the current commit? (that's what I did in #169 back then)

Oh, I see what you mean! Yes, it would be better to pin vcs URLs at the current commit.

What do you think of saving that feature for a separate pull request? The current PR is not a regression, AFAIK -- it treats git+https://github.com/blueyed/starlette.git@local#egg=starlette the same as -e git+https://github.com/blueyed/starlette.git@local#egg=starlette, which is also not pinned. In the meantime, you can work around the limitation (as I already do with -e) by using a commit URL instead of a tag/branch.

@jcushman jcushman force-pushed the url-requirements branch from b51b212 to 225ebb9 Compare May 5, 2019 18:01
@jcushman
Copy link
Contributor Author

jcushman commented May 5, 2019

I've noticed that pip-sync would try to sync URL package every time.

Good catch. The latest commit allows you to avoid this by providing the pinned version in the egg fragment:

$ pip-compile
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile
#
https://github.com/pallets/click/archive/7.0.zip#egg=click==7.0
$ pip-sync
Everything up-to-date

I think that's the best we can do, right? If the version isn't provided in the URL, the archive has to be downloaded and checked.

@atugushev
Copy link
Member

Good catch. The latest commit allows you to avoid this by providing the pinned version in the egg fragment:

Great!

I think that's the best we can do, right? If the version isn't provided in the URL, the archive has to be downloaded and checked.

You are right. Turns out that's correct behaviour.

BTW, i've tried egg fragment with only package name:

$ pip-compile
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile
#
https://github.com/pallets/click/archive/7.0.zip#egg=click

$ pip-sync
Everything up-to-date

Shouldn't it download and check version if there is only package name in egg fragment and no version?

@jcushman jcushman force-pushed the url-requirements branch 2 times, most recently from 393d434 to 6129847 Compare May 6, 2019 02:53
@jcushman
Copy link
Contributor Author

jcushman commented May 6, 2019

Shouldn't it download and check version if there is only package name in egg fragment and no version?

Oops, yes it should. In fact it should download and reinstall, even if the version number matches in the download, because we don't know if the URL has changed. For example, suppose you change requirements.in from:

https://github.com/jazzband/pip-tools/archive/master.zip#egg=example

to

https://github.com/jcushman/pip-tools/archive/master.zip#egg=example

It would be surprising if pip-sync didn't change anything because I didn't update the version in my fork, right? We don't know how the installed module was specified, so we have to uninstall and reinstall on every pip-sync.

My latest commit hopefully strikes a good balance:

  • If you choose to explicitly provide #egg=example==7.0 in requirements.txt, then pip-sync believes you and only downloads and reinstalls if the version doesn't match.
  • If you just provide #egg=example, or no name, then pip-sync downloads, uninstalls, and reinstalls.

@atugushev
Copy link
Member

My latest commit hopefully strikes a good balance:

  • If you choose to explicitly provide #egg=example==7.0 in requirements.txt, then pip-sync believes you and only downloads and reinstalls if the version doesn't match.
  • If you just provide #egg=example, or no name, then pip-sync downloads, uninstalls, and reinstalls.

Awesome! Would've been nice to have tests for each scenario in test_cli_sync.py.

@jcushman
Copy link
Contributor Author

jcushman commented May 6, 2019

Would've been nice to have tests for each scenario in test_cli_sync.py.

Those scenarios are tested via diff() unit tests in test_sync.py, the same as the rest of the pip-sync diff behavior. test_cli_sync.py only seems to have tests for cli wrapper flags and error messages.

What else is left to get this merged? I looked at the codecov/patch report, and I think the only lines that lost coverage are guards that are no longer hit, but that I'm wary of removing in case there are edge cases I don't know about.

@jcushman jcushman force-pushed the url-requirements branch from 6129847 to de874f4 Compare May 6, 2019 18:43
@jcushman
Copy link
Contributor Author

jcushman commented May 6, 2019

Squashed! git rebase -i HEAD~19 is a personal record for me. :)

@jazzband jazzband deleted a comment from codecov bot May 6, 2019
@jcushman
Copy link
Contributor Author

jcushman commented May 8, 2019

Anything else pending on this?

@jedie
Copy link

jedie commented May 9, 2019

Anything else pending on this?

Seems that this will land in 3.7.0 and there is one issued open, see: https://github.com/jazzband/pip-tools/milestone/19 isn't it?

@atugushev
Copy link
Member

Seems that this will land in 3.7.0 and there is one issued open, see: https://github.com/jazzband/pip-tools/milestone/19 isn't it?

We can postpone this issue to the next release.

@atugushev
Copy link
Member

I'll prepare a release within next 24 hours.

@vphilippon
Copy link
Member

@atugushev I validated pip-tools 3.7.0 release, it's on pypi.org now

chigby added a commit to freedomofpress/securedrop.org that referenced this pull request Dec 17, 2019
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.

One other thing I do here is reconcile the pshtt/ssylze/cryptography
dependency-hell situation by pinning the version of sslyze to the
latest one that supports (a) the most recent, secure version of
cryptography, and (b) python 3.5.  Some of the versions of these
packages were not release the last time this situation was examined.
chigby added a commit to freedomofpress/securedrop.org that referenced this pull request Dec 17, 2019
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.

One other thing I do here is reconcile the pshtt/ssylze/cryptography
dependency-hell situation by pinning the version of sslyze to the
latest one that supports (a) the most recent, secure version of
cryptography, and (b) python 3.5.  Some of the versions of these
packages were not release the last time this situation was examined.
chigby added a commit to freedomofpress/securethenews that referenced this pull request Dec 17, 2019
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.
chigby added a commit to freedomofpress/securethenews that referenced this pull request Dec 17, 2019
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.
chigby added a commit to freedomofpress/securethenews that referenced this pull request Dec 17, 2019
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.
harrislapiroff pushed a commit to freedomofpress/securethenews that referenced this pull request Jan 7, 2020
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.
chigby added a commit to freedomofpress/pressfreedomtracker.us that referenced this pull request Jan 15, 2020
See jazzband/pip-tools#807 for more
information about permitting URLs as packages.  We want this because
--generate-hashes works with URL-based requirements but not with
Git-repo-based requirements.  Subtle different, but it works.  As such
I think it's okay to remove the separate github URL requirements file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with pip : pip-compile does not support URLs as packages
6 participants