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

setupext.py: problems parsing setup.cfg (not updated to changes in configparser) #5846

Closed
khyox opened this issue Jan 13, 2016 · 3 comments
Closed
Labels
Milestone

Comments

@khyox
Copy link
Contributor

khyox commented Jan 13, 2016

The problem

Last matplotlib master commit (matplotlib release 1.5.1+939.g25590c8) under Python 3.5.1. Excerpt from setup.cfg (unchanged):

[packages]
# There are a number of subpackages of matplotlib that are considered
# optional.  They are all installed by default, but they may be turned 
# off here.    
#
tests = True 
sample_data = True  
toolkits = True  

Excerpt of python3 setup.py build:

OPTIONAL SUBPACKAGES
           sample_data: no  [skipping due to configuration]
              toolkits: no  [skipping due to configuration]
                 tests: no  [skipping due to configuration]
        toolkits_tests: yes [using nose version 1.3.7 / using unittest.mock]

So optional subpackages not installed despite setup.cfg and testing not available:

>>> import matplotlib
>>> matplotlib.test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/software/Python/Python-3.5.1/lib/python3.5/site-packages/matplotlib-1.5.1+939.g25590c8-py3.5-linux-x86_64.egg/matplotlib/__init__.py", line 1544, in test
    _init_tests()
  File "/software/Python/Python-3.5.1/lib/python3.5/site-packages/matplotlib-1.5.1+939.g25590c8-py3.5-linux-x86_64.egg/matplotlib/__init__.py", line 1503, in _init_tests
    raise ImportError("matplotlib test data is not installed")
ImportError: matplotlib test data is not installed

The cause

In setupext.py, class OptionalPackage, initial excerpt of method check (lines 512-519):

        # Check configuration file                                                                                                                                                                        
        conf = self.get_config()
        # Default "auto" state or install forced by user                                                                                                                                                  
        if conf in [True, 'auto']:
            message = "installing"
            # Set non-optional if user sets `True` in config                                                                                                                                              
            if conf is True:
                self.optional = False

and active part of classmethod get_config (lines 501 to 503 of setupext.py):

        if config is not None and config.has_option(cls.config_category, cls.name):
            return config.get(cls.config_category, cls.name)
        return "auto"

where config = configparser.SafeConfigParser(). The class SafeConfigParser() of the configparser module is deprecated from Python 3.2, but that is not the key issue. The problem is that its get method is not properly documented (I opened documentation bug http://bugs.python.org/issue26094) and it currently returns just a string (and not a boolean), as the header doc for the module last release source code (https://hg.python.org/cpython/file/3.5/Lib/configparser.py) highlights:

    get(section, option, raw=False, vars=None, fallback=_UNSET)
        Return a string value for the named option. (...)

So, concerning the method check, the condition of line 515 (if conf in [True, 'auto']:) will never be satisfied unless the packages are marked as auto in setup.cfg:

[packages]
# (...)
tests = auto 
sample_data = auto  
toolkits = auto  

since then:

OPTIONAL SUBPACKAGES
           sample_data: yes [installing]
              toolkits: yes [installing]
                 tests: yes [using nose version 1.3.7 / using unittest.mock]
        toolkits_tests: yes [using nose version 1.3.7 / using unittest.mock]

A solution

To recover the intended functionality, I suggest the next modification in the classmethod get_config of the class OptionalPackage in setupext.py:

    @classmethod
    def get_config(cls):
        """                                                                                                                                                                                               
        Look at `setup.cfg` and return one of ["auto", True, False] indicating                                                                                                                            
        if the package is at default state ("auto"), forced by the user (True)                                                                                                                            
        or opted-out (False).                                                                                                                                                                             
        """
        conf = "auto"
        if config is not None and config.has_option(cls.config_category, cls.name):
            try:
                conf = config.getboolean(cls.config_category, cls.name)
            except ValueError:
                conf = config.get(cls.config_category, cls.name)
        return conf

I will prepare a pull request with those changes in case they were to be accepted. Hope this helps. Thanks.

@jenshnielsen
Copy link
Member

Thanks You analysis sounds correct to me. A pull request fixing this would be much appreciated. In general I think the setupext module needs some work but Im not sure when I will get a chance to look at it.

@tacaswell tacaswell added this to the Critical bug fix release (1.5.2) milestone Jan 13, 2016
@khyox
Copy link
Contributor Author

khyox commented Jan 13, 2016

Thanks @jenshnielsen! The corresponding pull request is #5849. I added other minor fixes, one to avoid the DeprecationWarning in newer releases of configparser due to the use of the SafeConfigParserclass.

tacaswell added a commit that referenced this issue Jan 17, 2016
Update setupext.py to solve issue #5846
tacaswell added a commit that referenced this issue Jan 17, 2016
Update setupext.py to solve issue #5846
@jenshnielsen
Copy link
Member

Close since #5849 is merged

tacaswell added a commit to tacaswell/matplotlib that referenced this issue May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants