Work around for side effects in setup.py script

[xolox]

As reported in #1253 I ran into problems with setup_requires and related magic in the setup.py script of the cryptography package. Thanks to your comments in #1253 I now understand why setup.py does what it does and why this is required (the cffi package requires it to build extension modules).

As suggested by @dstufft I went source diving setuptools and distutils, unfortunately I came back disappointed and without a solution. Because I'd like to help make things better and not just complain I decided to try a different route in getting #1253 resolved: I looked for other projects with similar problems. Fairly quickly I ran into pypa/pip#25 which describes almost the same issue and suggests a solution to the problem. That solution was implemented for SciPy (see their setup.py script) and it seems to work well for them.

I implemented the same solution in the setup.py script of the cryptography package and would like you to review and hopefully merge this minor change. It may be a bit pragmatic but it completely avoids the problem reported in #1253 so if you ask me it's definitely worth it. I've tested creating a source distribution of the cryptography package and installing that and with the changes it works fine and as I originally expected it to: During pip's metadata discovery phase it doesn't do anything funky, only once pip invokes python setup.py install are the setup_requires dependencies installed and the extension modules compiled.

Thanks for your time and for the comments in #1253.

[jenkins-cryptography]

Can one of the admins verify this patch?

[alex]

ok to test

[alex]

I'm still reviewing this patch and thinking about it (going to be doing that for a while :-)). But two small notes:

• Could you make this function take argv as an argument and pass sys.argv to it?
• House style would be to write this as:

return {
    "setup_requires": requirements,
    "cmdclass": {
        "build": CFFIBuild,
        "install": CFFIInstall,
        "test": PyTest,
    }
}

[dstufft]

So I'm going to have to do a little bit of source diving to make sure I grok the side effects of this. One thing I would like to see is instead of just silently not overriding the build and install command that this replaces any command that would depend on the setup_requires with a no-op command that just raises a RuntimeError. That way if there is some bug where it tries to build or whatever and it gets detected as a "we should be side effect free" then it'll raise an immediate error instead of just not installing stuff.

[jenkins-cryptography]

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2160/

[jenkins-cryptography]

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2161/

[coveralls]

Coverage remained the same when pulling 2793b6c on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[jenkins-cryptography]

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2163/

[xolox]

Thanks for the feedback so far! Some questions:

1. @dstufft: Do you agree with my implementation in 6edda86? I can explain why I went with this implementation if needed.
2. Should I add the sdist command so that the source distributions you upload to PyPI no longer include the __pycache__ directories with generated C source code?
3. I tested 6edda86 by exploiting the fact that python setup.py clean build now fails, because the code I copied from SciPy is kind of naive. Should I fix this?

[xolox]

It looks like Jenkins just pointed out to me that I have some work to do...

[coveralls]

Coverage remained the same when pulling 94cec73 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[xolox]

Hmm, I was afraid of that: It looks like distutils / setuptools always calls finalize_options() on each custom build command. If my impression is correct maybe I can remove the raise from finalize_options() and add it to the actual build/install/test function inside each command class. I'll investigate.

[xolox]

I think 2705e80 should resolve the problem pointed out by Jenkins, but let's see if Jenkins actually agrees with me.

[jenkins-cryptography]

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2164/

[xolox]

The tests now pass on all but one platform: freebsd92 + py27. I don't really understand the failure though, there's no traceback or anything similar. It almost seems like a false negative.

[alex]

freebsd92 randomly fails sometimes unfortunately, don't worry about it -
travis are the canonical "must always pass" tests.

On Fri, Jul 11, 2014 at 5:56 PM, Peter Odding notifications@github.com
wrote:

The tests now pass on all but one platform: freebsd92 + py27. I don't
really understand the failure though, there's no traceback or anything
similar. It almost seems like a false negative.

—
Reply to this email directly or view it on GitHub
#1257 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

[xolox]

@alex wrote:

freebsd92 randomly fails sometimes unfortunately, don't worry about it - travis are the canonical "must always pass" tests.

Thanks for the confirmation. I'll wait for Travis to process all of my commits (given the test suite of the cryptography package I guess this may take a while :-).

---

I previously wrote:

I tested 6edda86 by exploiting the fact that python setup.py clean build now fails, because the code I copied from SciPy is kind of naive. Should I fix this?

This was bothering me so I fixed it in 9e34c14 by including a white list of arguments and 'shallow parsing' sys.argv. I think this should be more or less bullet proof, it even recognizes things like python setup.py -vn ... (combined short options).

However I can imagine you would find this last commit overkill. If needed I can remove this commit or change it according to your feedback.

[coveralls]

Coverage remained the same when pulling 2705e80 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[coveralls]

Coverage remained the same when pulling 2705e80 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[jenkins-cryptography]

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2165/

[coveralls]

Coverage remained the same when pulling 9e34c14 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[alex]

Why does this subclass CFFIBuild and then not use it's finalize_options? Seems like this should just subclass build and then not have a finalize_options method? Same for the other classes.

[Ayrx]

I believe if this PR works and gets merged in it will fix the problem pointed out in #716 as well.

[xolox]

@alex:

Why does this subclass CFFIBuild and then not use it's finalize_options? Seems like this should just subclass build and then not have a finalize_options method? Same for the other classes.

I thought it was an elegant way not to repeat the name of the superclass. I'll change it.

@Ayrx:

I believe if this PR works and gets merged in it will fix the problem pointed out in #716 as well.

Yes it should fix that issue.

[xolox]

Why does this subclass CFFIBuild and then not use it's finalize_options? Seems like this should just subclass build and then not have a finalize_options method? Same for the other classes.

I thought it was an elegant way not to repeat the name of the superclass. I'll change it.

Except that I was repeating the name of the superclass anyway (in the finalize_options methods) so that made absolutely no sense. It's now fixed, I simplified the code as @alex suggested, thanks for the suggestion.

[jenkins-cryptography]

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2173/

[xolox]

Test failed. Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2173/

Just checked the Jenkins test failures, these all seem to be timeouts on a specific test node. So far all tests on Travis CI are passing.

[coveralls]

Coverage remained the same when pulling b077c15 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[xolox]

I was testing the change suggested by me in this pull request and noticed that it's not finished yet; it doesn't actually work for pip's metadata discovery phase! This is because of the command line (magic) that pip uses:

Downloading/unpacking cryptography>=0.2.1 (from ...)
  Running setup.py egg_info for package cryptography
    XXX sys.argv of setup.py: ['-c', 'egg_info', '--egg-base', 'pip-egg-info']

Two things make this incompatible with the pull request as implemented:

1. The -c option.
2. The --egg-base option and its argument pip-egg-info.

The second one can be fixed by adding support for recognizing the --egg-base option and its argument pip-egg-info as not needing setup_requires. The first one is a bit more obscure because the -c option is not actually accepted by setup.py:

peter@macbook> python setup.py -c egg_info
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: option -c not recognized

The -c option appears because of how pip executes the setup.py egg_info command. Repeating all of the nasty implementation details here is out of scope, but basically this is what pip does:

peter@macbook> cat setup.py
import sys
print "sys.argv = %s" % sys.argv
peter@macbook> python -c 'exec(open("setup.py").read())' egg_info
sys.argv = ['-c', 'egg_info']

Although I've seen this behavior of the Python command line interpreter before, I'm not actually sure why or how it happens.

I'll return here with a solution. I'm not sure we're going to like it, but I'm interested to see if it's possible to compensate for the problems caused by setup_requires in a setup.py script without getting too creative or verbose (this could turn into an interesting "design pattern" for packages that really need setup_requires but acknowledge the problems it can cause).

---

Update: Okay fixing the issue wasn't that exciting :-). A colleague of mine pointed out that -c was sys.argv[0] which explains why distutils never complained about the option. The only remaining thing that had to be fixed (see 1784811) was support for the --egg-base option and its argument.

[xolox]

I just pushed my (hopefully) final commit to this pull request. With the latest commit included the setup.py now allows all "read only" options that I know of and it's compatible with pip's metadata discovery phase (just tested this to confirm).

@alex, @dstufft: If there is anything else I can do to get this pull request merged please let me know! Thanks for the reviews / feedback so far.

---

I needed this patch for version 0.5.2 so I wrote a small shell script that downloads a cryptography source distribution archive from PyPI, unpacks it, applies the patch (downloaded from GitHub) and repacks the source distribution as a tar archive so that it can be uploaded to a private PyPI mirror (e.g. devpi-server). I tested this with 0.5.2 and the patch applies with minimal offset and works as intended. Here's the shell script:

#!/bin/bash -e

VERSION=0.5.2
BASENAME=cryptography-$VERSION
ARCHIVE=$BASENAME.tar.gz
TEMPDIR=$(mktemp --directory)

cd $TEMPDIR
wget https://pypi.python.org/packages/source/c/cryptography/$ARCHIVE
wget https://github.com/pyca/cryptography/pull/1257.diff
tar xf $ARCHIVE
patch -d $BASENAME -p 1 < 1257.diff
find $BASENAME -type d -name __pycache__ -print0 | xargs -0 rm -R
tar czf /tmp/$ARCHIVE $BASENAME
echo -e "\nFinished patching source distribution:"
ls -lh /tmp/$ARCHIVE

[jenkins-cryptography]

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2177/

[coveralls]

Coverage remained the same when pulling 1784811 on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[xolox]

I just pushed my (hopefully) final commit to this pull request.

Error — The Travis CI build could not complete due to an error

Okay I should have seen that coming, it failed on PEP-8 violations. Fixed now.

[jenkins-cryptography]

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2178/

[coveralls]

Coverage remained the same when pulling cc3b84d on xolox:setup-script-side-effects-fix into 1872e75 on pyca:master.

[alex]

Merge conflicts here :-( Besides that, @dstufft can I ask you to review this?

[reaperhulk]

closing in favor of #1366 (a rebase of this).

[xolox]

Thanks @alex and @dstufft for the reviews, feedback and merging and thanks to @reaperhulk for rebasing this so it could be merged! I'm glad cryptography's setup.py script can now be introspected without (unexpected) side effects!