Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only force gnureadline in setup.py with bdist_wheel #7047

Merged
merged 2 commits into from
Dec 1, 2014
Merged

Only force gnureadline in setup.py with bdist_wheel #7047

merged 2 commits into from
Dec 1, 2014

Conversation

xolox
Copy link
Contributor

@xolox xolox commented Nov 28, 2014

The asymmetry (inconsistency) of unconditionally adding gnureadline as a dependency of binary (but not source) distributions in setup.py can cause issues like reported at paylogic/pip-accel/issues/34. Would you be willing to accept this pull request to remove the asymmetry? For reference, it seems that this was introduced in ba0cd52 with the following comment:

adjust dependency check for gnureadline on OS X

  • always depend when making a bdist (for consistency)
  • otherwise only depend if missing (e.g. pip install -e)

I guess it's kind of ironic that the original reasoning was consistency and now I'm calling this inconsistent :-). I'm interested to hear whether you agree about the asymmetry though. If you need more context about why I want to fix the asymmetry please refer to paylogic/pip-accel/issues/34, I'm posting a detailed explanation there.

The inconsistency (asymmetry) of unconditionally adding gnureadline as a
dependency of binary distributions can cause issues like reported at
paylogic/pip-accel#34
@minrk
Copy link
Member

minrk commented Nov 28, 2014

The reason for bdist is that making a wheel on a machine that has readline should not exclude gnureadline from the dependency list, because it's still a dependency on the machines on which the wheel (or egg, etc.) will be installed.

@minrk
Copy link
Member

minrk commented Dec 1, 2014

This PR makes the behavior of creating a bdist less consistent because the resulting bdist is different depending on the computer building the bdist. In master, bdists always have the same dependencies, regardless of the compiling host. There is a fundamental difference between installing from sdists and bdists because when installing from sdist, the environment can be checked at the destination. With bdists, dependencies are computed without sufficient knowledge of the destination, so the least common denominator choice is taken.

Since IPython actually only cares about wheel bdists, and pip-accel uses bdist_dumb for some reason, if you change the check from startswith('bdist') to == 'bdist_wheel', it should serve both our purposes.

@xolox
Copy link
Contributor Author

xolox commented Dec 1, 2014

@minrk: Thanks for your initial reply and follow up! Last weekend I came to the same conclusion: The goal of ba0cd52 seemed to be to make sure that binary wheels would be generated including the proper dependencies (which makes sense of course). I'll change the pull request.

…r_readline() fails

This is a follow up to 4d9f6f0 based on
the discussion in pull request #7047 where we seem to have found a middle
ground that should make everyone happy :-)
@minrk
Copy link
Member

minrk commented Dec 1, 2014

At the time, it was true of eggs as well, and it remains the right behavior for all bdists intended for distribution. But the only official bdists of IPython from now on are wheels, so the reality is that most bdists other than our own wheels are meant for private re-use (e.g. pip-accel), where the least-common-denominator behavior is not helpful.

@minrk minrk added this to the 3.0 milestone Dec 1, 2014
@minrk minrk changed the title Always depend on gnureadline in setup.py if setupext.check_for_readline() fails Only force gnureadline in setup.py with bdist_wheel Dec 1, 2014
minrk added a commit that referenced this pull request Dec 1, 2014
Only force gnureadline in setup.py with bdist_wheel

instead of all bdists
@minrk minrk merged commit 69b1076 into ipython:master Dec 1, 2014
@xolox xolox deleted the setup-gnureadline-fix branch December 3, 2014 19:27
@xolox
Copy link
Contributor Author

xolox commented Dec 3, 2014

@minrk: Thanks for the feedback and for the quick merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants