Skip to content

Commit

Permalink
Require an already created session to be passed into APIs
Browse files Browse the repository at this point in the history
  • Loading branch information
dstufft committed May 7, 2014
1 parent fb46902 commit 7037443
Show file tree
Hide file tree
Showing 7 changed files with 131 additions and 51 deletions.
8 changes: 6 additions & 2 deletions pip/download.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,9 @@ def get_file_content(url, comes_from=None, session=None):
"""Gets the content of a file; it may be a filename, file: URL, or
http: URL. Returns (location, content). Content is unicode."""
if session is None:
session = PipSession()
raise TypeError(
"get_file_content() missing 1 required keyword argument: 'session'"
)

match = _scheme_re.search(url)
if match:
Expand Down Expand Up @@ -494,7 +496,9 @@ def _copy_file(filename, location, content_type, link):
def unpack_http_url(link, location, download_cache, download_dir=None,
session=None):
if session is None:
session = PipSession()
raise TypeError(
"unpack_http_url() missing 1 required keyword argument: 'session'"
)

temp_dir = tempfile.mkdtemp('-unpack', 'pip-')
temp_location = None
Expand Down
19 changes: 12 additions & 7 deletions pip/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
UnsupportedWheel,
)
from pip.backwardcompat import urlparse, url2pathname
from pip.download import PipSession, url_to_path, path_to_url
from pip.download import url_to_path, path_to_url
from pip.wheel import Wheel, wheel_ext
from pip.pep425tags import supported_tags, supported_tags_noarch, get_platform
from pip._vendor import html5lib, requests, pkg_resources
Expand All @@ -39,6 +39,12 @@ def __init__(self, find_links, index_urls,
use_wheel=True, allow_external=[], allow_unverified=[],
allow_all_external=False, allow_all_prereleases=False,
session=None):
if session is None:
raise TypeError(
"PackageFinder() missing 1 required keyword argument: "
"'session'"
)

self.find_links = find_links
self.index_urls = index_urls
self.cache = PageCache()
Expand Down Expand Up @@ -73,7 +79,7 @@ def __init__(self, find_links, index_urls,
self.allow_all_prereleases = allow_all_prereleases

# The Session we'll use to make requests
self.session = session or PipSession()
self.session = session

def _sort_locations(self, locations):
"""
Expand Down Expand Up @@ -717,7 +723,9 @@ def __str__(self):
@classmethod
def get_page(cls, link, req, cache=None, skip_archives=True, session=None):
if session is None:
session = PipSession()
raise TypeError(
"get_page() missing 1 required keyword argument: 'session'"
)

url = link.url
url = url.split('#', 1)[0]
Expand Down Expand Up @@ -827,11 +835,8 @@ def _handle_fail(req, link, reason, url, cache=None, level=1, meth=None):
cache.add_page_failure(url, level)

@staticmethod
def _get_content_type(url, session=None):
def _get_content_type(url, session):
"""Get the Content-Type of the given url, using a HEAD request"""
if session is None:
session = PipSession()

scheme, netloc, path, query, fragment = urlparse.urlsplit(url)
if scheme not in ('http', 'https', 'ftp', 'ftps'):
# FIXME: some warning or something?
Expand Down
7 changes: 5 additions & 2 deletions pip/req/req_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import re

from pip.backwardcompat import urlparse
from pip.download import PipSession, get_file_content
from pip.download import get_file_content
from pip.req.req_install import InstallRequirement
from pip.util import normalize_name

Expand All @@ -12,7 +12,10 @@
def parse_requirements(filename, finder=None, comes_from=None, options=None,
session=None):
if session is None:
session = PipSession()
raise TypeError(
"parse_requirements() missing 1 required keyword argument: "
"'session'"
)

skip_match = None
skip_regex = options.skip_requirements_regex if options else None
Expand Down
10 changes: 8 additions & 2 deletions pip/req/req_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from pip._vendor import pkg_resources
from pip.backwardcompat import HTTPError
from pip.download import (PipSession, url_to_path, unpack_vcs_link, is_vcs_url,
from pip.download import (url_to_path, unpack_vcs_link, is_vcs_url,
is_file_url, unpack_file_url, unpack_http_url)
from pip.exceptions import (InstallationError, BestVersionAlreadyInstalled,
DistributionNotFound, PreviousBuildDirError)
Expand Down Expand Up @@ -54,6 +54,12 @@ def __init__(self, build_dir, src_dir, download_dir, download_cache=None,
target_dir=None, ignore_dependencies=False,
force_reinstall=False, use_user_site=False, session=None,
pycompile=True, wheel_download_dir=None):
if session is None:
raise TypeError(
"RequirementSet() missing 1 required keyword argument: "
"'session'"
)

self.build_dir = build_dir
self.src_dir = src_dir
self.download_dir = download_dir
Expand All @@ -74,7 +80,7 @@ def __init__(self, build_dir, src_dir, download_dir, download_cache=None,
self.as_egg = as_egg
self.use_user_site = use_user_site
self.target_dir = target_dir # set from --target option
self.session = session or PipSession()
self.session = session
self.pycompile = pycompile
self.wheel_download_dir = wheel_download_dir

Expand Down
Loading

17 comments on commit 7037443

@reversefold
Copy link

Choose a reason for hiding this comment

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

Why? This has caused us to pin an earlier version of pip due to non-obvious breakage. Suddenly requiring a session when one could be (and used to be) created automatically seems to break backwards compatibility without a clear reason.

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

pip does not have a public API, everything is internal and this means that there is no backwards compatibility promises. The reason we're requiring a session and not just creating one is because in our internal uses of this API it was too easy to forget to pass in the session kwarg and lose settings like proxy and timeout settings.

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

Parsing the requirements file should not need a HTTP Session object.

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

Sure it should, because you can point at remote files which need to be fetched with a HTTP session object.

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

It'd be nicer to have an API for remote objects and files, then.

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

You can do -r https://example.com/ inside of a requirements.txt. It's not possible to fully parse every possible requirements file without either removing that ability or passing in a session object.

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

This is for uses in setup.py files

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

i.e. pip as an API

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

pip doesn't have an API and importing pip inside of a setup.py is a bad idea.

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

@dstufft then what's the canonical way of expressing requirements for pip packages, then?

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

Put the requirements inside of the install_requires of the setup.py.

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

So you need to duplicate the requirements.txt file and the info in the setup.py file? Seems fraught.

@dstufft
Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you need a requirements.txt file at all? If you have your dependencies inside of setup.py you can do pip install . inside of a checkout, or you can do pip install -e . inside of a checkout or you can do pip install -e <vcs_url> to point to a checkout.

If you really want to support pip install -r requirements.txt inside of a check out you can just make a stub requirements.txt that has something like:

-e .

or

.

@AeroNotix
Copy link

Choose a reason for hiding this comment

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

Thanks for the info.

@reversefold
Copy link

Choose a reason for hiding this comment

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

There are a few upsides to using a requirements.txt.

  1. Dependencies are easier to look at casually, especially with more complicated setup.py scripts.
  2. You can use "pip freeze" to easily generate and update a requirements.txt.

@techtonik
Copy link
Contributor

Choose a reason for hiding this comment

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

#4105 should help when requirements don't include URLs.

Please sign in to comment.