Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fixes test helper to not import astropy.config if invoked from setup #197

Merged
merged 3 commits into from

4 participants

@eteq
Owner

Currently, running setup.py causes astropy.config to be imported, because it imports astropy/tests/helpers.py, which uses ConfigurationItem. This is problematic for a few reasons - it means that anything in astropy.config has to be 2to3-independent, and it casues the ~/.astropy to be created just for running setup.py, which is neither intended nor expected behavior. This PR fixes these by just not using the ConfigurationItem trick if we are running inside setup.py.

The configuration item can still be updated using the environment variable ASTROPY_USE_SYSTEM_PYTEST - once #48 is fixed, this should be updated to reflect whatever envar naming scheme is used there.

@mdboom, the solution here for knowing we are in setup.py is inspired by the way you dealt with is_in_build_mode. I realize this seems redundant, given that set_build_mode is called in setup.py, but the problem here is that setup_helpers.py itself imports the tests/helpers.py. What do you think of this?

@mdboom
Collaborator

This seems fine. We could reduce some of the redundancy by using the same key for this as we currently are...

i.e. replace _in_setup with _build_mode and remove the call to set_build_mode in setup.py (and probably remove the set_build_mode function from setup_helpers.py.

But that's all just finesse to remove duplication -- I think in other ways this patch is fine as-is.

setup.py
@@ -11,6 +11,9 @@
import os
from setuptools import setup, find_packages
+#A dirty hack to get around some early import/configurations ambiguities
+__builtins__._in_setup = True
@embray Collaborator
embray added a note

I always liked how Numpy uses __NUMPY_SETUP__ for this purpose, and sometimes felt that we should have something similar but name it __ASTROPY_SETUP__. Not that the naming matters so much, since it's just a hack. But it looks more "important" like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
astropy/tests/helper.py
@@ -14,11 +14,23 @@
from distutils.core import Command
-from ..config import ConfigurationItem
-
-USE_SYSTEM_PYTEST = ConfigurationItem('use_system_pytest', False,
- 'Set to True to load system pytest',
- 'boolean', 'astropy.tests.helper')
+#If we are in setup.py, we don't want to import astropy.config
+if __builtins__.get('_in_setup'):
@embray Collaborator
embray added a note

And not that there's anything wrong with this either, but by putting _in_setup in __builtins__, in astropy/__init__.py we could put:

try:
    __ASTROPY_SETUP__
except NameError:
   __builtins__. __ASTROPY_SETUP__ = False

That's pretty much what Numpy does, which I've always kind of liked. Though I don't think there's necessarily any advantage to it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdboom
Collaborator

@iguananaut: Those seem like good improvements over what's there now.

@eteq
Owner

@iguananaut and @mdboom - I also like @iguananaut's idea, and have implemented it (except I used single-underscores instead of double-underscores... I have a vague memory of a Guido post about how double-underscore surrounded variables should be avoided for variables that aren't actually part of python itself... although it really isn't that terribly important, I suppose).

Two things about this, though:

  • @iguananaut, I tried your suggestion for astropy/__init__.py and found I had to instead use builtins as a dictionary instead of using attribute access inside the except clause... this is very magical/mysterious behavior to me - do you understand why that is (although it does work as you suggested in this form, I think)
  • As it stands now, when an affiliated package runs it's setup.py, it will still set the _ASTROPY_SETUP variable - is this what we want, or do we want _ASTROPY_SETUP_ to be different from the "build mode" that might be applicable for each affiliated package. My feeling is that this is just fine the way it is, as we can always change it later if some affiliated package really cares about this distinction... but I thought I'd see if anyone else cares.
@embray
Collaborator

Ah, right, I didn't think of that. From the Python docs (http://docs.python.org/library/__builtin__.html <-- bad GitHub parsing the underscores in that URL!):

CPython implementation detail: Most modules have the name builtins (note the 's') made available as part of their globals. The value of builtins is normally either this module or the value of this modules’s dict attribute. Since this is an implementation detail, it may not be used by alternate implementations of Python.

It's an implementation detail, for example, that at the command-line the __builtins__ in the __main__ module directly references the __builtin__ module (or builtins in Python 3).

I think a more correct approach would be to import __builtin__ (or for Python 3 import builtins as __builtin__) and use that, rather than rely on the __builtins__ global. Also, in the places where you have something like __builtins__.get('_ASTROPY_SETUP_') you can just use the _ASTROPY_SETUP_ directly. That's the advantage of putting it in the builtins--it makes it available in the global namespace of every module.

Looks good otherwise :)

@eteq
Owner

Thanks for the tips @iguananaut, this should be safer against future changes (and more py3-happy).

Unless there are objections I'll merge this shortly. If we run into a bizzare use-case where someone really needs their package's "build mode" to be separate from the astropy core build mode, we can revisit then.

@embray
Collaborator

Works for me.

@astrofrog
Owner

Looks good to me!

@eteq eteq merged commit 76ddf69 into astropy:master
@keflavich keflavich referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@eteq eteq deleted the eteq:fix-test-helper-imports branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 9, 2012
  1. @eteq
Commits on Apr 12, 2012
  1. @eteq
Commits on Apr 18, 2012
  1. @eteq
This page is out of date. Refresh to see the latest.
View
11 astropy/__init__.py
@@ -6,6 +6,17 @@
managing them.
"""
+#this indicates whether or not we are in astropy's setup.py
+try:
+ _ASTROPY_SETUP_
+except NameError:
+ from sys import version_info
+ if version_info[0] >= 3:
+ import builtins
+ else:
+ import __builtin__ as builtins
+ builtins._ASTROPY_SETUP_ = False
+ del version_info
try:
from .version import version as __version__
View
37 astropy/setup_helpers.py
@@ -434,11 +434,42 @@ def adjust_compiler():
def is_in_build_mode():
- return __builtins__.get('_build_mode')
+ """
+ Determines if the current package is being built.
+
+ Returns
+ -------
+ buildmode : bool
+ True if the current package is in the process of being built.
+
+ See Also
+ --------
+ `set_build_mode`
+ """
+ #_ASTROPY_SETUP_ is added to the builtins in setup.py or astropy/__init__.py
+ return _ASTROPY_SETUP_
+
+def set_build_mode(val=True):
+ """
+ Sets whether or not the current package is being built.
+
+ Parameters
+ ----------
+ val : bool
+ Whether or not build mode should be activated.
-def set_build_mode():
- __builtins__['_build_mode'] = True
+ See Also
+ --------
+ `is_in_build_mode`
+ """
+ from sys import version_info
+
+ if version_info[0] >= 3:
+ import builtins
+ else:
+ import __builtin__ as builtins
+ builtins._ASTROPY_SETUP_ = val
def setup_test_command(package_name):
View
22 astropy/tests/helper.py
@@ -14,11 +14,23 @@
from distutils.core import Command
-from ..config import ConfigurationItem
-
-USE_SYSTEM_PYTEST = ConfigurationItem('use_system_pytest', False,
- 'Set to True to load system pytest',
- 'boolean', 'astropy.tests.helper')
+#If we are in setup.py, we don't want to import astropy.config
+if __builtins__.get('_ASTROPY_SETUP_'):
+ if os.environ.get('ASTROPY_USE_SYSTEM_PYTEST'):
+ USE_SYSTEM_PYTEST = lambda: True
+ else:
+ USE_SYSTEM_PYTEST = lambda: False
+else:
+ from ..config import ConfigurationItem
+
+ USE_SYSTEM_PYTEST = ConfigurationItem('use_system_pytest', False,
+ 'Set to True to load system pytest. '
+ 'This item will *not* be obeyed if '
+ 'using setup.py. In that case the '
+ 'environment variable '
+ 'ASTROPY_USE_SYSTEM_TEST must be '
+ 'used',
+ 'boolean', 'astropy.tests.helper')
if USE_SYSTEM_PYTEST():
import pytest
View
13 setup.py
@@ -9,8 +9,18 @@
import glob
import os
+import sys
from setuptools import setup, find_packages
+#A dirty hack to get around some early import/configurations ambiguities
+#This is the same as setup_helpers.set_build_mode(), but does not require
+#importing setup_helpers
+if sys.version_info[0] >= 3:
+ import builtins
+else:
+ import __builtin__ as builtins
+builtins._ASTROPY_SETUP_ = True
+
import astropy
from astropy import setup_helpers
from astropy.version_helper import get_git_devstr, generate_version_py
@@ -25,9 +35,6 @@
# broken one.
setup_helpers.adjust_compiler()
-# Indicate that we are in building mode
-setup_helpers.set_build_mode()
-
if not release:
version += get_git_devstr(False)
generate_version_py('astropy', version, release,
Something went wrong with that request. Please try again.