Skip to content

Conversation

@eteq
Copy link
Member

@eteq eteq commented Dec 15, 2014

This updates setup_helpers to the latest version to allow @astrofrog to finish up #3166 .

@embray - was it intentional to require this modification to setup.py? Or did you not realize that is_distutils_display_option is also used in setup? An alternative to this PR is to put is_distutils_display_option back into setup_helpers in the setup_helpers package.

See related discussions in astropy/astropy-helpers#110 and #3166

@embray
Copy link
Member

embray commented Dec 15, 2014

Ah, didn't realize. Though I've been occasionally voicing an opinion that we shouldn't have setup_requires=['numpy'] at all. Although it's technically true, the way setup_requires works is problematic for large requirements like Numpy (ideally it would be the correct thing to do, but in practice it's problematic...)

@eteq
Copy link
Member Author

eteq commented Dec 15, 2014

Ok, so are you ok with just adding that link back into the astropy_helpers module, @embray ? That would remove the need to update setup.py here and in the affiliated packages/template (the latter being the more probelmatic one)

@embray
Copy link
Member

embray commented Dec 15, 2014

The only thing is, I'd rather keep it where it is now. What I could do (since this didn't actually intend to break anything) is the same thing I did with write_if_different, in that I'll go ahead and put
from .distutils_helpers import * in astropy_helpers.setup_helpers. But I'd rather change any imports that are using these (so keep this PR as is).

@embray
Copy link
Member

embray commented Dec 15, 2014

Basically a soft, pending deprecation/move.

@embray
Copy link
Member

embray commented Dec 15, 2014

Pushed a few changes to astropy_helpers to fix this.

@eteq
Copy link
Member Author

eteq commented Dec 15, 2014

ok, cool - will update this to the latest and remove the setup.py change

@eteq eteq force-pushed the update-setup-helpers branch from defe2ba to 485be49 Compare December 15, 2014 22:23
@embray
Copy link
Member

embray commented Dec 15, 2014

You misunderstood (or I was unclear). I suggested to not remove the setup.py change.

@eteq
Copy link
Member Author

eteq commented Dec 15, 2014

@embray oh, gotcha, but the fixes are so that affiliated packages are still ok. See the new commit.

@embray
Copy link
Member

embray commented Dec 15, 2014

👍 Thanks!

@embray
Copy link
Member

embray commented Dec 16, 2014

I think this needs to be updated now to incorporate astropy/astropy-helpers#112, so it can be used by #3166 .

@astrofrog
Copy link
Member

Yes, and once this is merged I'll rebase #3166

@eteq
Copy link
Member Author

eteq commented Dec 16, 2014

Updated to lastest astropy-helpers. It seems to work, except now every call to setup.py yields a bunch of warnings (shown below). Everything seems to work, but the warnings suggest something isn't quite right... Any idea, @embray ?

astropy/erfa/setup_package.py:3: RuntimeWarning: Parent module 'astropy.erfa' not found while handling absolute import
  import os
astropy/erfa/setup_package.py:4: RuntimeWarning: Parent module 'astropy.erfa' not found while handling absolute import
  from distutils.extension import Extension
astropy/erfa/setup_package.py:6: RuntimeWarning: Parent module 'astropy.erfa' not found while handling absolute import
  from astropy_helpers import setup_helpers
astropy/table/setup_package.py:2: RuntimeWarning: Parent module 'astropy.table' not found while handling absolute import
  import os
astropy/table/setup_package.py:3: RuntimeWarning: Parent module 'astropy.table' not found while handling absolute import
  from distutils.extension import Extension
astropy/io/ascii/setup_package.py:3: RuntimeWarning: Parent module 'astropy.io.ascii' not found while handling absolute import
  import os
astropy/io/ascii/setup_package.py:4: RuntimeWarning: Parent module 'astropy.io.ascii' not found while handling absolute import
  from distutils.extension import Extension
astropy/io/fits/setup_package.py:3: RuntimeWarning: Parent module 'astropy.io.fits' not found while handling absolute import
  import os
astropy/io/fits/setup_package.py:5: RuntimeWarning: Parent module 'astropy.io.fits' not found while handling absolute import
  from distutils.core import Extension
astropy/io/fits/setup_package.py:6: RuntimeWarning: Parent module 'astropy.io.fits' not found while handling absolute import
  from glob import glob
astropy/io/fits/setup_package.py:8: RuntimeWarning: Parent module 'astropy.io.fits' not found while handling absolute import
  from astropy_helpers import setup_helpers
astropy/io/votable/setup_package.py:2: RuntimeWarning: Parent module 'astropy.io.votable' not found while handling absolute import
  from distutils.core import Extension
astropy/io/votable/setup_package.py:3: RuntimeWarning: Parent module 'astropy.io.votable' not found while handling absolute import
  from os.path import join
astropy/io/votable/setup_package.py:5: RuntimeWarning: Parent module 'astropy.io.votable' not found while handling absolute import
  from astropy_helpers import setup_helpers
astropy/utils/xml/setup_package.py:2: RuntimeWarning: Parent module 'astropy.utils.xml' not found while handling absolute import
  from distutils.core import Extension
astropy/utils/xml/setup_package.py:3: RuntimeWarning: Parent module 'astropy.utils.xml' not found while handling absolute import
  from os.path import join
astropy/utils/xml/setup_package.py:4: RuntimeWarning: Parent module 'astropy.utils.xml' not found while handling absolute import
  import sys
astropy/utils/xml/setup_package.py:6: RuntimeWarning: Parent module 'astropy.utils.xml' not found while handling absolute import
  from astropy_helpers import setup_helpers
astropy/vo/samp/setup_package.py:3: RuntimeWarning: Parent module 'astropy.vo.samp' not found while handling absolute import
  import os

@eteq eteq force-pushed the update-setup-helpers branch from b27897a to 0730e28 Compare December 16, 2014 16:00
@embray
Copy link
Member

embray commented Dec 16, 2014

That's bizarre. No, I don't see anything like that. I'll try to see if I can figure out what might cause such a warning though. What Python version?

@embray
Copy link
Member

embray commented Dec 16, 2014

I am seeing these warnings if I run python2.7 -Wall; @eteq do you have some setting on your Python to enable all warnings by default?

@embray
Copy link
Member

embray commented Dec 16, 2014

Testing a change that might fix this, though it's not really a big problem. I'm still curious why you saw it by default and not me.

@embray
Copy link
Member

embray commented Dec 16, 2014

Well, first attempt at a fix didn't do it, as it seems this is actually from deep in the import system. But it seems that adding from __future__ import absolute_import to the relevant modules will shush this.

@astrofrog
Copy link
Member

The warnings do show up on Travis by the way, but only for Python 2.x, not 3.x. How shall we proceed, can we go ahead and merge this anyway since the warnings are harmless?

@embray
Copy link
Member

embray commented Dec 16, 2014

Yes, I'll submit a separate PR that will fix these warnings.

@astrofrog
Copy link
Member

Ok, merging this and rebasing #3166

astrofrog added a commit that referenced this pull request Dec 16, 2014
update setup_helpers and a corresponding setup.py change
@astrofrog astrofrog merged commit d88631a into astropy:master Dec 16, 2014
@eteq
Copy link
Member Author

eteq commented Dec 16, 2014

@embray - the from __future__ import absolute_import is probably a fine solution - that's one of the "standard" imports, anyway, for a lot of the regular code.

Note also that these are only for modules that have C code in them... Maybe this is an artifact of the builtins._ASTROPY_SETUP_ tricks or something? Not sure why that would have changed, though.

And I'm seeing what @astrofrog noted on Travis: py 2.7 by default shows this, while 3.x does not. As far as I know I don't have any atypical warning settings...

@eteq eteq deleted the update-setup-helpers branch December 16, 2014 17:26
@embray
Copy link
Member

embray commented Dec 16, 2014

Yeah, this is just because prior to PEP 328 the default behavior for the import statement was to look for package-relative imports first, so if you're in astropy.io.fits and call import os it looks first for astropy.io.fits.os rather than looking from the top of sys.path. Because we're loading these setup_package modules without actually creating the package they belong to this warning occurs when it goes looking for the astropy.io.fits package to look in for the os module.

Enabling from __future__ import absolute_import disables the old functionality, so that these import statements are no longer treated as possible relative imports.

embray added a commit to embray/astropy that referenced this pull request Dec 16, 2014
@eteq
Copy link
Member Author

eteq commented Dec 18, 2014

aah, gotcha, that makes sense. Thanks for the clarification!

@bsipocz
Copy link
Member

bsipocz commented Dec 19, 2014

I'm afraid I have a broken setup.py in the latest master, most probably due to this PR.

 python setup.py develop
Traceback (most recent call last):
  File "setup.py", line 21, in <module>
    from astropy_helpers.distutils_helpers import is_distutils_display_option
ImportError: No module named distutils_helpers

Is it something trivial I missed, or it's better to open a separate issue for it?

@embray
Copy link
Member

embray commented Dec 19, 2014

@bsipocz Not sure if this helps, but try running git submodule update.

@bsipocz bsipocz mentioned this pull request Dec 19, 2014
@bsipocz
Copy link
Member

bsipocz commented Dec 19, 2014

@embray - Sadly it doesn't help locally. However the build starts when I test it on a linux server, but it fails with #3241

@bsipocz
Copy link
Member

bsipocz commented Dec 20, 2014

So, I think the problem was that I had an obsolete astropy_helpers repo in the same virtenv, and setup.py tried to import stuff from that one and not from the submodule.
The build is now working fine.

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.

4 participants