Skip to content

Simplify declaration of install_requires. #9536

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

Merged
merged 1 commit into from
Oct 28, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 23, 2017

The previous way of declaring install_requires is way too overengineered
now that we unconditionally rely on the standard dependency resolution
machinery.

Don't use environment markers on install_requires as who knows what
version of setuptools & pip does the end user have.

Drop the check on Tornado as we don't do anything with it.

(Done in preparation of declaring a dependency on contextlib2 on Py2 to fix #9521 (comment).)

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

The previous way of declaring install_requires is way too overengineered
now that we unconditionally rely on the standard dependency resolution
machinery.

Don't use environment markers on install_requires as who knows what
version of setuptools & pip does the end user have.

Drop the check on Tornado as we don't do anything with it.
@anntzer anntzer added the Build label Oct 23, 2017
@jklymak
Copy link
Member

jklymak commented Oct 24, 2017

Not sure if it units as a review, but I used this in a fresh environment on my machine and it installed fine using pip install -e .

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 on the changes, but I'm not knowledgable about the previous system to know if there was some reason we had all that machinery.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 25, 2017

I'm pretty sure the machinery dates back to when dependency installation was a mess and we used to vendor the dependencies, see https://github.com/matplotlib/matplotlib/wiki/MEP11#current-behavior.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works my setup and passes all the tests, which I think all download and setup, so I think this works.

@Kojoley Kojoley added this to the v2.2 milestone Oct 28, 2017
@Kojoley Kojoley merged commit e344d21 into matplotlib:master Oct 28, 2017
@anntzer anntzer deleted the setuptools-install_requires branch October 28, 2017 14:36
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants