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

Declare Numpy as a setup dependency #2445

Merged
merged 4 commits into from Sep 27, 2013

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Sep 20, 2013

This will install Numpy before matplotlib, so that we can get at its headers at compile-time.

Note that this doesn't work with python setup.py install, it only works when you do pip install . (I think that's by design). But this should also fix pip install matplotlib from PyPI, which is awfully nice.

Thanks to @tcaswell, @WeatherGod and @pelson for their thoughts about this on the mailing list, and https://github.com/h5py/h5py/pull/356/files from which a lot of this is shamelessly stolen.

@mdboom
Copy link
Member Author

mdboom commented Sep 20, 2013

BTW: I'm a little on the fence about whether this belongs on 1.3.x, but it would be nice to get it in and fix pip installs sooner rather than later.

@WeatherGod
Copy link
Member

I say that it is a bug fix for 1.3.x. The upcoming 1.3.1 release will contain many installation fixes, so it makes perfect sense to include this one with it. I am going to do some testing with this. I am going to see if I can make this break or not in a few different ways.

@WeatherGod
Copy link
Member

yeah, that issue with it not working for "python setup.py install" needs to be figured out. The problem is that tools like easy_install that can install from a source distribution basically does that under the hood.

For my own packages at work (much more simple than matplotlib's though), I use setup_requires for things like the coverage package and nose. It seems to always make sure I have them (probably a little too often) but it does seem to work as expected. Perhaps we are being a little too restrictive in when to set setup_requires?

@mdboom
Copy link
Member Author

mdboom commented Sep 20, 2013

Hmm... So in your other projects, setting setup_requires works with python setup.py install? It's not clear to me from the docs that's supposed to work.

I'm not sure how we could be less restrictive in when we set setup_requires. The only time it's turned off is when a --help argument is set.

@WeatherGod
Copy link
Member

Yes, so, if I have nose and coverage specified in both setup_requires and install_requires, then by the end of "python setup.py install" in a completely clean environment, I will have nose and coverage eggs both in my local directory (from setup_requires), and in my site-packages (from the install_requires). The error that is happening when calling install for this branch is that include_dirs_hook() is being called prior to any downloads of dependencies, which is strange, because that is being called by the build_ext command for the matplotlib.ft2font extension.

@mdboom
Copy link
Member Author

mdboom commented Sep 20, 2013

Yes, I would have assumed any build-time dependencies would be downloaded before built_ext is run. There's obviously something missing here.

@WeatherGod
Copy link
Member

I know they are now identical, but I have run into issues with distribute
and setuptools conflicting with each other. Perhaps we should explicitly
use setuptools?

Perhaps another avenue is to make a fake, simple package that only depends
on numpy and try to get that to work? I find it difficult to wade through
our setup.py and setupext.py.

@WeatherGod
Copy link
Member

While researching this a bit further, I came across an interesting feature we might want to use: "extras_require" and "features":

     |   'extras_require' -- a dictionary mapping names of optional "extras" to the
     |      additional requirement(s) that using those extras incurs. For example,
     |      this::
     |  
     |          extras_require = dict(reST = ["docutils>=0.3", "reSTedit"])
     |  
     |      indicates that the distribution can optionally provide an extra
     |      capability called "reST", but it can only be used if docutils and
     |      reSTedit are installed.  If the user installs your package using
     |      EasyInstall and requests one of your extras, the corresponding
     |      additional requirements will be installed if needed.
     |  
     |   'features' -- a dictionary mapping option names to 'setuptools.Feature'
     |      objects.  Features are a portion of the distribution that can be
     |      included or excluded based on user options, inter-feature dependencies,
     |      and availability on the current system.  Excluded features are omitted
     |      from all setup commands, including source and binary distributions, so
     |      you can create multiple distributions from the same source tree.
     |      Feature names should be valid Python identifiers, except that they may
     |      contain the '-' (minus) sign.  Features can be included or excluded
     |      via the command line options '--with-X' and '--without-X', where 'X' is
     |      the name of the feature.  Whether a feature is included by default, and
     |      whether you are allowed to control this from the command line, is
     |      determined by the Feature object.  See the 'Feature' class for more
     |      information.

@WeatherGod
Copy link
Member

Another situation where you would want the setup_requires list to be empty besides "--help" is for "clean".

@WeatherGod
Copy link
Member

Hmm, strange... in my dead simple setup.py where I have numpy as a setup_requires, simply doing "python setup.py clean" causes numpy to get installed locally. But for this branch, no attempt at downloading numpy happens.

@WeatherGod
Copy link
Member

An observation, while fixing this didn't make things work for me (yet), I noticed that the Extension objects and Distribute objects were coming from distutils instead of setuptools. Since setup_requires is a setuptools feature, I wouldn't be surprised that a Distribution from distutils wouldn't recognize it.

@pelson
Copy link
Member

pelson commented Sep 23, 2013

👍 for including this in v1.3.1

@mdboom
Copy link
Member Author

mdboom commented Sep 23, 2013

Ok -- I think this is working, I was just testing it wrong.

I have a test script in unit/test_clean_install.sh that demonstrates how this should work. Anyone want to confirm that it works for them. It's really nice to finally resolve this problem.

@WeatherGod
Copy link
Member

Haven't tested, but virtualenv.py does not technically work completely by itself. There is virtualenv_support folder that is needed as well.

@mdboom
Copy link
Member Author

mdboom commented Sep 23, 2013

I believe I have included the standalone version of virtualenv.py. Let me know if that's not the case.

setuptools monkeypatches distutils, so importing from either place is the same thing, AFAIK.

@WeatherGod
Copy link
Member

Seems to work for me, although the messages at the beginning from compiling numpy in the local space is fairly "scary". There was an odd warning towards the end of the install for me:

/tmp/easy_install-CNtSMb/numpy-1.7.1/numpy/distutils/misc_util.py:252: RuntimeWarning: Parent module 'numpy.distutils' not found while handling absolute import

Another possible command to ignore setup_requires might be "sdist".

@WeatherGod
Copy link
Member

A possible issue I just came across...

So, let's say that one time the numpy package gets installed at that local location, it then becomes the location to use for compiling matplotlib in any future builds regardless of whether numpy is installed or not, completely ignoring the available numpy for that environment.

This is technically also a similar issue to one where matplotlib gets installed prior to an update of an existing numpy install. The build will be made against the wrong numpy (which this PR wouldn't solve).

mdboom added a commit that referenced this pull request Sep 27, 2013
Declare Numpy as a setup dependency
@mdboom mdboom merged commit 87679af into matplotlib:v1.3.x Sep 27, 2013
@WeatherGod
Copy link
Member

Why was this merged? I think the issue I reported with having an install of numpy sitting locally needs to be discussed. This causes particular havoc when switching between different virtualenvs with different numpys in them because the local install takes precedence.

@mdboom
Copy link
Member Author

mdboom commented Sep 27, 2013

@WeatherGod: Sorry, I thought I had commented on this, but the comment apparently didn't stick.

I think the problem with things being left in the build tree that impact upgrading the dependencies is much larger than this. It happens even without this -- if you build with a particular version of numpy and then build for another virtualenv with another version of numpy in it, you still run into this problem. The only good procedure when moving between versions of numpy is to git clean -fxd anyway. I feel there is enough good here to justify it, and the issue you raise is not really new, unless I'm missing something.

@mdboom
Copy link
Member Author

mdboom commented Sep 27, 2013

Sorry for this, by the way -- I didn't mean to ignore your feedback. I really thought I had commented on it and I interpreted your lack of reply as agreement. It turns out I think the comment never posted (probably because I forgot to hit the "Comment" button). If there's strong disagreement about this, I'm happy to revert it before cutting 1.3.1.

@WeatherGod
Copy link
Member

Fair enough. And indeed, this isn't exactly anything new, it is just exacerbated, I think, with this PR. Perhaps that would be a good thing? We will need to continue making it clear that "git clean -fxd" should always be done.

@mdboom mdboom deleted the numpy-setup-dependency branch August 7, 2014 13:52
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