! Fixes packaging and installation behaviour under Python2 and Python3. #939

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@ghost

ghost commented Nov 16, 2012

Long short story again... This PR aims at fixing #916 and #918 while still keeping dependencies vendored (unlike in #929 where I had removed them)... So, what I did was to remove everything except __init__.py from thepackages folder (which made up requests.packages) from inside "requests" and separate it into three packages inside the dependencies folder:

  • dependencies/common: This contains all the packages needed by requests and which work both on PY2 and PY3 ( currently urllib3 only ). It will be made into a package (requests.packages.common) by setuptools/distutils during installation via the package_dir instruction.
  • dependencies/python2: This contains PY2 specific packages ( currently oauthlib and chardet ). It will be made into a package (requests.packages.environment) during installation on PY2 (via the package_dir instruction).
  • dependencies/python3: This contains PY3 specific packages ( currently chardet2 ). It will be made into a package (requests.packages.environment) during installation on PY3 (via the package_dir instruction).

Next, I added a recursive-include targeting the dependencies folder, so that all the packages get included when sdisting (no matter what version of Python is used to do the actual packaging)... Finally I moved the requests source folder to source/requests to cheat nose's test loader and force it to use the installed version instead. A few changes to the Makefile were also necessary, and that's all there's to it.

I have tested this in every supported Python version and is working great (Travis CI says the same). If you want to test pip before merging I have uploaded a testing package of this precise version which installs flawlessly under Python2.6+.

pip install requests-packaging-fixes

Edit: The problems described in #916 and #916 had been caused not by a change on setup.py (since there were none which could potentially have caused this) but due to the fact that 0.14.1 and 0.14.2 were packaged under different versions of Python. I could reproduce the current behaviour by checking-out both sources and packaging them with Python3 for 0.14.1 (this would include all vendored packages) and with Python2 for 0.14.2 (this would leave out chardet2). Weird, but true.

Owner

Lukasa commented Nov 16, 2012

This is impressive work! =D

That said, I'm not sure @kennethreitz is going to be super-keen on moving the primary requests files into a directory not called "requests". If you could find another way around the nose test loader (which might mean a hack involving sys.path), he'll probably be quite a bit happier.

@ghost

ghost commented Nov 16, 2012

@Lukasa: Thank you :-) Hmm... Let's wait for his opinion. I actually tried to do that, but it didn't prove easy... :p

@RishiRamraj RishiRamraj commented on the diff Nov 16, 2012

- 'requests.packages.oauthlib.oauth1',
- 'requests.packages.oauthlib.oauth1.rfc5849',
- 'requests.packages.oauthlib.oauth2',
- 'requests.packages.oauthlib.oauth2.draft25',
- 'requests.packages.chardet',
- ])
+py3packages = [
+ 'requests.packages.environment.chardet2',
+]
+
+package_dir = {
+ 'requests': 'source/requests',
+ 'requests.packages.common': 'dependencies/common',
+}
+
+if sys.version_info[0] == 2:
@RishiRamraj

RishiRamraj Nov 16, 2012

I may be wrong, but I think this line will still cause pypi installations to fail for python 3. The problem is that setup.py evaluates this line at package time, causing only the python 2 packages to be bundled on pypi (assuming you package with python 2). The only way this code would work, as far as I know, is if you create two separate pypi projects and upload them separately; once with python 2 and once with python 3.

@ghost

ghost Nov 16, 2012

@RishiRamraj: Sorry to say that, but you're indeed wrong. It's now the MANIFEST.in file (and not setup.py) that takes care of bundling all the necessary dependencies (for both Python2 and Python3). Then, after packaging, setup.py takes care of unpacking only the necessary dependencies for the underlying Python version at install-time. Test for yourself by running this on a PY3 virtualenv :-) :

pip install requests-packaging-fixes
@RishiRamraj

RishiRamraj Nov 16, 2012

@bmcustodio: Ah, brilliant! +1

@ghost

ghost Nov 16, 2012

@RishiRamraj: Thank you :-) Hope that explains the trick. ;-)

@ghost

ghost commented Nov 24, 2012

@kennethreitz Are you considering merging this as-is or is there anything you'd like me to change?

Owner

Lukasa commented Nov 24, 2012

It looks like Kenneth is going in a different direction with chardet (see issue #951). Additionally, it seems that newer versions of oauthlib support Python 3, so these two things together might solve this issue. This is @kennethreitz's call, but I suspect a new version of oauthlib and a fork of chardet will solve our problems.

Unknown referenced this pull request Nov 24, 2012

Closed

Fork Chardet #951

Owner

kennethreitz commented Nov 27, 2012

@bmcustodio I didn't intentionally close the ticket! I am doing some cleanup and switched the default branch from 'develop' to 'master'. That apparently closed all pending pull requests :(

@ghost

ghost commented Nov 27, 2012

@kennethreitz Oh sorry then. My bad :(

Owner

kennethreitz commented Nov 27, 2012

I'll respond soon — I haven't yet because i honestly haven't figured out why it broke....

Owner

sigmavirus24 commented Nov 27, 2012

@kennethreitz it broke because you ran setup.py register with only one version of python. With that in mind, only that version's vendored dependencies were included, i.e., python2's were included by not python3's.

Owner

kennethreitz commented Nov 27, 2012

I've always done it that way. Something else changed.

Owner

sigmavirus24 commented Nov 27, 2012

@kennethreitz the setup.py file you used changed. It conditionally included the packages based on python version. Unfortunately it also based it on which version you ran it with. This is the line in question

miohtama referenced this pull request in miohtama/vvv Dec 4, 2012

Open

Python 3 + chardet #16

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