Fix 'python setup.py bdist_dumb' on Mac OS X #582

Merged
merged 2 commits into from Sep 10, 2015

Projects

None yet

2 participants

@xolox
Contributor
xolox commented Sep 5, 2015

Hi Paramiko developers!

In 2013 I published pip-accel and it wasn't long before the first bug report came in that the python setup.py bdist_dumb command was broken for the ssh package on Mac OS X. After some analysis I realized that the ssh package included a distutils monkey patch (setup_helper.py) which had become incompatible with newer versions of distutils. My advice to the person who reported the bug was to switch to Paramiko because ssh was a fork of Paramiko, ssh seemed abandoned and Paramiko didn't include the distutils monkey patch.

Imagine my surprise when the same bug report was reopened in 2015 because the distutils hack had found its way from ssh to Paramiko :-). Multiple users have now complained about it. I can't reasonably fix this from the side of pip-accel because the distutils monkey patch is simply broken against new versions of distutils on Mac OS X. The only other option was a pull request against the Paramiko project to get the real bug fixed, so here I am :-).


Here's what happens when python setup.py bdist_dumb is run in a checkout of the Paramiko master branch on Mac OS X (tested on Yosemite (10.10.5)):

running install_scripts
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/var/folders/63/_tgckwx14vb40h5f_9gq4cjw0000gp/T/pip-Kh_t56-build/setup.py", line 91, in <module>
    **kw
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/command/bdist.py", line 146, in run
    self.run_command(cmd_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/cmd.py", line 326, in run_command
    self.distribution.run_command(command)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/command/bdist_dumb.py", line 124, in run
    owner=self.owner, group=self.group)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/cmd.py", line 392, in make_archive
    owner=owner, group=group)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/archive_util.py", line 237, in make_archive
    filename = func(base_name, base_dir, **kwargs)
TypeError: make_tarball() got an unexpected keyword argument 'owner'

With the minor changes to setup_helper.py in my feature branch the python setup.py bdist_dumb completes successfully and pip-accel can successfully install Paramiko. I tested this as follows:

This shows the issue:

VENV=/tmp/paramiko-master-broken
virtualenv $VENV \
 && (source $VENV/bin/activate \
      && pip install pip-accel \
      && pip-accel install https://github.com/paramiko/paramiko/archive/master.zip \
      && python -c 'import paramiko') \
 && echo "Successfully installed Paramiko on Mac OS X :-)" \
 || echo "Failed to install Paramiko on Mac OS X :-("

On my test system the above shell snippet outputs a "Failed to install Paramiko" message.

This shows that the issue is resolved in my feature branch:

VENV=/tmp/paramiko-pull-request-works
virtualenv $VENV \
 && (source $VENV/bin/activate \
      && pip install pip-accel \
      && pip-accel install https://github.com/xolox/paramiko/archive/fix-bdist-dumb-mac-os-x.zip \
      && python -c 'import paramiko') \
 && echo "Successfully installed Paramiko on Mac OS X :-)" \
 || echo "Failed to install Paramiko on Mac OS X :-("

On my test system the above shell snippet outputs a "Successfully installed Paramiko" message.


I hope I've clearly explained why I think my pull request would be an improvement. If you have any questions I would be happy to answer them.

Disclaimer: I don't know why setup_helper.py was originally added and I couldn't determine this based on a git blame of the file and looking up the old revisions; I couldn't find a public issue or discussion arguing about why the monkey patch is necessary in the first place. My pull request assumes the monkey patch is there for a good reason and just makes it work again. To the best of my knowledge my changes should be backwards compatible with both older and newer versions of distutils.

Thanks for your time!

@xolox xolox Try to fix `python setup.py bdist_dumb' on Mac OS X (paylogic/pip-acc…
…el#2)

Additions based on /usr/lib/python2.7/distutils/archive_util.py
from the Ubuntu 12.04 package python2.7 (2.7.3-0ubuntu3.8). I have not
looked into the compatibility of the software licenses of Paramiko vs
distutils however the original setup_helper.py code in Paramiko was
clearly also copied from distutils so to be honest it's not like I'm
changing the status quo.
6c5df36
@xolox xolox referenced this pull request in paylogic/pip-accel Sep 5, 2015
Closed

Errors when building/installing ssh==1.7.14 #2

@bitprophet
Member

Thanks for this!

Yea, that file dates from 2006 and lacks granularity in its commit message, so I have as little clue as you do re: its necessity.

Interestingly, I'd never seen this myself (and it appears to pop up even in python setup.py sdist - which is what we use for releases) because it only happens on Python 2.7 and up, and I run my development virtualenv under 2.6 as it's the oldest we support. I just checked after noting you were on 2.7, and sure enough, TypeError.

I confirm your fix works under 2.7 for sdist, and that the result is installable.

Unfortunately, the bdist (granted, I'm not usually dealing with these, so not sure if it's ever worked) explodes on install, having some problem unpacking things or whatnot, e.g.:

IOError: [Errno 2] No such file or directory: '/var/folders/yb/d3l0hbhn5jvgv2fxb7mlj7cr0000gn/T/pip-9XWn0I-build/setup.py'

(where that temp dir is the one it's trying to unpack into).

Furthermore, your fix doesn't work under 2.6 even on the build step, since tar.add doesn't seem to have a filter kwarg in that version:

Creating tar file /Users/jforcier/Code/oss/paramiko/dist/paramiko-1.16.0.macosx-10.10-intel.tar.gz with mode w:gz
Traceback (most recent call last):
  File "setup.py", line 91, in <module>
    **kw
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/core.py", line 152, in setup
    dist.run_commands()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/dist.py", line 975, in run_commands
    self.run_command(cmd)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/command/bdist_dumb.py", line 122, in run
    self.format, root_dir=archive_root)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/cmd.py", line 403, in make_archive
    base_name, format, root_dir, base_dir, dry_run=self.dry_run)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/archive_util.py", line 163, in make_archive
    filename = func(base_name, base_dir, **kwargs)
  File "/Users/jforcier/Code/oss/paramiko/setup_helper.py", line 127, in make_tarball
    tar.add(base_dir, filter=_set_uid_gid)
TypeError: add() got an unexpected keyword argument 'filter'

I don't have more time to dig into this but I'm assuming there are further contortions you could do to handle 2.6 compatibility within your additions? I've no idea about the bdist install issue and am curious if you see that on your end or not.

@xolox
Contributor
xolox commented Sep 9, 2015

Hi Jeff and thanks for the fast feedback!

Unfortunately, the bdist (granted, I'm not usually dealing with these, so not sure if it's ever worked) explodes on install, having some problem unpacking things or whatnot ...

pip doesn't support installation from "dumb binary distributions" and so expects to find a setup.py script, this is why it raises the error you reported. pip-accel is basically pip plus caching via "dumb binary distributions" which are installed by pip-accel itself (because pip can't do it, although it can do binary wheels nowadays, but I digress). So this error is expected.

Furthermore, your fix doesn't work under 2.6 even on the build step, since tar.add doesn't seem to have a filter kwarg in that version ...

Ouch, sorry about that, I missed that!

I don't have more time to dig into this but I'm assuming there are further contortions you could do to handle 2.6 compatibility within your additions?

It's not actually that bad :-). I just committed, pushed and tested a7c8266 which uses try ... except TypeError logic to first try the tar.add(...) call with the filter parameter and fall back to a call without the filter parameter if TypeError is raised.


I wrote a new test script and used that script to confirm Python 2.6, 2.7 and 3.4 compatibility. These are the Python versions I have available on a Mac OS X system I can (ab)use to test this :-). Here's the output of the script:

Testing branch 'master' of repository 'https://github.com/paramiko/paramiko.git' ..
python2.6 | sdist | works | /tmp/paramiko-pull-request-582/python2.6-miXWQx/output.log
python2.7 | sdist | fails | /tmp/paramiko-pull-request-582/python2.7-pTXM8T/output.log
python3.4 | sdist | fails | /tmp/paramiko-pull-request-582/python3.4-Z2pghq/output.log
python2.6 | bdist | works | /tmp/paramiko-pull-request-582/python2.6-Lcp5Xh/output.log
python2.7 | bdist | fails | /tmp/paramiko-pull-request-582/python2.7-QOwSue/output.log
python3.4 | bdist | fails | /tmp/paramiko-pull-request-582/python3.4-qrt4c5/output.log
Testing branch 'fix-bdist-dumb-mac-os-x' of repository 'https://github.com/xolox/paramiko.git' ..
python2.6 | sdist | works | /tmp/paramiko-pull-request-582/python2.6-V1z4xe/output.log
python2.7 | sdist | works | /tmp/paramiko-pull-request-582/python2.7-xQdjNc/output.log
python3.4 | sdist | works | /tmp/paramiko-pull-request-582/python3.4-RHMTCI/output.log
python2.6 | bdist | works | /tmp/paramiko-pull-request-582/python2.6-JcqbIn/output.log
python2.7 | bdist | works | /tmp/paramiko-pull-request-582/python2.7-EPuzU6/output.log
python3.4 | bdist | works | /tmp/paramiko-pull-request-582/python3.4-rx0KME/output.log

Here is the shell script used to perform the test:

#!/bin/bash -e

# A single directory to put our temporary files.
WORKING_DIRECTORY=/tmp/paramiko-pull-request-582

# Based on the classifiers in Paramiko's setup.py script.
SUPPORTED_PYTHON_VERSIONS=$(echo python{2.6,2.7,3.2,3.3,3.4})

main () {
  # Reset the working directory.
  rm -Rf $WORKING_DIRECTORY
  mkdir -p $WORKING_DIRECTORY
  # Test Paramiko's master branch.
  test_checkout https://github.com/paramiko/paramiko.git master
  # Test pull request 582.
  test_checkout https://github.com/xolox/paramiko.git fix-bdist-dumb-mac-os-x
}

test_checkout () {
  local repo="$1"
  local branch="$2"
  echo "Testing branch '$branch' of repository '$repo' .."
  foreach_version sdist "git clone $repo && cd paramiko && git checkout $branch && python setup.py sdist && pip install dist/*.tar.gz && python -c 'import paramiko'"
  foreach_version bdist "git clone $repo && cd paramiko && git checkout $branch && pip install pip-accel && pip-accel install . && python -c 'import paramiko'"
}

foreach_version () {
  local label="$1"
  local command_line="$2"
  for python_version in $SUPPORTED_PYTHON_VERSIONS; do
    if which $python_version &>/dev/null; then
      local directory=$(mktemp -d $WORKING_DIRECTORY/$python_version-XXXXXX)
      local log_file=$directory/output.log
      virtualenv --python=$python_version $directory &> $log_file
      if (cd "$directory" && source bin/activate && eval "$command_line" &> $log_file); then
        echo "$python_version | $label | works | $log_file"
      else
        echo "$python_version | $label | fails | $log_file"
      fi
    fi
  done
}

main

PS. The Travis CI build failed due to a timeout. I guess I can't retry the build because I'm not a repo owner but I have the impression that this was a coincidence, not related to the contents of my pull request.

@bitprophet
Member

Eh, when a single interpreter on Travis times out & the rest pass, that's always just their (free! and awesome) infrastructure taking a powder. I don't worry about it much.

Confirming that things work better on my end now (re: 2.6), thanks!

@bitprophet bitprophet merged commit a7c8266 into paramiko:master Sep 10, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@bitprophet bitprophet added a commit that referenced this pull request Sep 10, 2015
@bitprophet bitprophet Changelog fixes #582 97e134a
@xolox
Contributor
xolox commented Sep 10, 2015

Thanks for merging this so quickly!

@xolox xolox deleted the unknown repository branch Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment