Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix issue #760: broken external requirements on Python 3 #874

Merged
merged 4 commits into from

5 participants

@hltbra
Owner

Applied g2p's pull request #851, changed test url, and moved python3/python2 details to backwardscompat module.

g2p and others added some commits
@g2p g2p Decode downloaded contents.
Also add a test (the exact url depends on pulling a change to pip-test-package).
26fd622
@hltbra hltbra Change test_requirements:test_remote_reqs to use hltbra's gist
g2p's pull request for issue #760 requires changes on pip-test-package.
The gist was created to remove this dependency.
bf35ea6
@hltbra hltbra Move python3/python2 details from ``download.get_file_content`` to ba…
…ckwardcompat module
274c4bd
@hltbra hltbra was assigned
pip/backwardcompat/__init__.py
@@ -87,6 +90,10 @@ def console_to_str(s):
def fwrite(f, s):
f.write(s)
+ def get_http_message_param(http_message, param, default_value):
+ encoding = http_message.getparam(param)
@pnasrat Owner
pnasrat added a note

Technically this is not necessarily encoding

@hltbra Owner
hltbra added a note

Ouch, my bad! Changed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pnasrat
Owner

Minor nit about naming if you fix that we're good to go.

@ptone

It is unclear what the best default encoding should be when no charset is explicitly defined.

http://www.w3.org/International/O-HTTP-charset#charset

The standard is latin-1, but the reality is that there could be utf-8 without a charset param

@g2p
g2p commented

I picked utf-8 as it seemed more compatible to me (I deliberately ignored latin1 as a browser thing), but after some thought ASCII might be a better choice when the charset is missing.

https://tools.ietf.org/html/rfc6657 updates the default mime handling for text/*, which was ambiguous before (http and mime conflicted for those types). requirements.txt doesn't have its own mime, it's text/plain and according to RFC 6657, which reaffirms RFC 2046, it should be ASCII if no charset is given.

FWIW, GitHub serves *.txt with the utf-8 charset, so people who need Unicode can safely host there.

@ptone

So here is my branch that has this one merged in, and uses it to address the issues tackled in #810, #850, #862

https://github.com/ptone/pip/compare/unicode-errors

#810 tries to use the backcompat u/utf-8, and falls back on latin-1, with no attempt to get charset
#850 tries to change the backcompat function to handle other decodings, but tries to match all archive content-types, instead of just doing 'not text/html'
#862 adds additional content type checks, but leaves the redundant filename based check, and doesn't handle charset param at all.

I can open a PR based on my branch after this one lands if that makes sense.

@pnasrat pnasrat merged commit 5256d57 into from
@ptone ptone referenced this pull request
Closed

Page charset fix #886

@ptone

is it a good idea to have a test dependency on a non pypa gist, or a gist at all here?

Owner

better to add a file to the pip-test-package, like before the diff.
there was a pull to add it: pypa/pip-test-package#2

@ptone

I see that this is merged - but the test doesn't seem to test the changed code, and in fact when I try - the test passes without the patch on py3

@g2p
g2p commented

The test does fail if I revert download.py, and pass if I don't:

git checkout 26fd6225e43fc04c1dda8754aedfd8a629fc7b47^ pip/download.py
nosetests-3.3 test_requirements.py:test_remote_reqs

This is true both on g2p/remote-reqs-py3 (26fd622) and pypa/develop (4ac9406).

As for the gist, it's better to have this on the pypa account since it can install arbitrary packages.

@ptone

@g2p

I don't mean to belabor the point, what am I missing? Doing these steps and the test still passes:

git co -b charset-test
git co 335bf12 pip/download.py
cd tests/
nosetests -v test_requirements:test_remote_reqs
@g2p
g2p commented

Remove any pyo/pyc, run nosetests-3.3, and don't cd to tests so that the development version of pip is in your path.

find \( -name '*.pyc' -o -name '*.pyo' \) -print -delete
nosetests-3.3 test_requirements.py:test_remote_reqs
@ptone

heh - no, I had all that dialed, it was just that I was spacing that this was a py3 bug, and was running it in my pip-dev py27 env

This is all water under the bridge, but a unit test for the new functionality could fail on py2/py3, instead this integration test doesn't even reach the assert in the test, as the pip_run fails the test before that.

@qwcode
Owner

@ptone @hltbra @g2p

fyi, I refactored this test

35db025

1) it uses a req file from our "pip-test-package", not a personal gist
2) it's now a unit test against parse_requirements. that was enough. no need for reset_env and installing
3) it has no assertions, but would "fail" with errors, if the code in question were reverted. writing up a "doesn't fail with this exception" test doesn't seem worth it to me.

let me know if anyone has concerns.

@hltbra
Owner

I am glad you replaced that super high-level test by a lower level one, @qwcode. We need more of these :-)

@g2p

I do like high-level functional tests, unlike unit tests they can catch problems one didn't anticipate.
The former describe what is important to users, the latter give developers extra confidence.
Ideally pip would have both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 28, 2013
  1. @g2p

    Decode downloaded contents.

    g2p authored
    Also add a test (the exact url depends on pulling a change to pip-test-package).
Commits on Mar 29, 2013
  1. @hltbra

    Change test_requirements:test_remote_reqs to use hltbra's gist

    hltbra authored
    g2p's pull request for issue #760 requires changes on pip-test-package.
    The gist was created to remove this dependency.
  2. @hltbra
Commits on Mar 30, 2013
  1. @hltbra
This page is out of date. Refresh to see the latest.
View
7 pip/backwardcompat/__init__.py
@@ -59,6 +59,9 @@ def console_to_str(s):
def fwrite(f, s):
f.buffer.write(b(s))
+ def get_http_message_param(http_message, param, default_value):
+ return http_message.get_param(param, default_value)
+
bytes = bytes
string_types = (str,)
raw_input = input
@@ -87,6 +90,10 @@ def console_to_str(s):
def fwrite(f, s):
f.write(s)
+ def get_http_message_param(http_message, param, default_value):
+ result = http_message.getparam(param)
+ return result or default_value
+
bytes = str
string_types = (basestring,)
reduce = reduce
View
7 pip/download.py
@@ -10,7 +10,7 @@
import tempfile
from pip.backwardcompat import (xmlrpclib, urllib, urllib2, httplib,
- urlparse, string_types, ssl)
+ urlparse, string_types, ssl, get_http_message_param)
if ssl:
from pip.backwardcompat import match_hostname, CertificateError
from pip.exceptions import InstallationError, PipError, NoSSLError
@@ -32,7 +32,7 @@
def get_file_content(url, comes_from=None):
"""Gets the content of a file; it may be a filename, file: URL, or
- http: URL. Returns (location, content)"""
+ http: URL. Returns (location, content). Content is unicode."""
match = _scheme_re.search(url)
if match:
scheme = match.group(1).lower()
@@ -54,7 +54,8 @@ def get_file_content(url, comes_from=None):
else:
## FIXME: catch some errors
resp = urlopen(url)
- return geturl(resp), resp.read()
+ encoding = get_http_message_param(resp.headers, 'charset', 'utf-8')
+ return geturl(resp), resp.read().decode(encoding)
try:
f = open(url)
content = f.read()
View
11 tests/test_requirements.py
@@ -29,6 +29,17 @@ def test_requirements_file():
assert result.files_created[env.site_packages/fn].dir
+def test_remote_reqs():
+ """
+ Test installing from a remote requirements file.
+ """
+ env = reset_env()
+ result = run_pip(
+ 'install', '--download', env.scratch_path, '-r',
+ 'https://gist.github.com/hltbra/5271779/raw/pip-test-package-requirements.txt')
+ assert result.files_created, result.files_created
+
+
def test_schema_check_in_requirements_file():
"""
Test installing from a requirements file with an invalid vcs schema..
Something went wrong with that request. Please try again.