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

Fix configparser import for python2 #3887

Closed
wants to merge 1 commit into from

Conversation

marsam
Copy link

@marsam marsam commented Jul 19, 2019

The latest release breaks scrapy on python2
Closes #3889

@kmike
Copy link
Member

kmike commented Jul 19, 2019

wow, I wonder why aren't tests detecting this - Python 2.7 tests pass on CI

@elacuesta
Copy link
Member

elacuesta commented Jul 20, 2019

Well, this is on me, sorry ✋ #3877
In my defense, the Travis env uses Python 2.7.14, and definitely has the lowercase version available.

Locally I have 2.7.15+ and they are different modules AFAICT

Python 2.7.15+ (default, Nov 27 2018, 23:36:35) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import configparser
>>> import ConfigParser
>>> configparser
<module 'configparser' from '/home/eugenio/.local/lib/python2.7/site-packages/configparser.pyc'>
>>> ConfigParser
<module 'ConfigParser' from '/usr/lib/python2.7/ConfigParser.pyc'>
>>>

I couldn't find anything in the 2.7.14 changelog (which includes everything up to 2.7.14) that indicates an early (before 3.0) renaming. However, I did found https://pypi.org/project/configparser/, which is indeed what my local 2.7.15 runtime is using and I suspect is also what Travis uses.

$ head /home/eugenio/.local/lib/python2.7/site-packages/configparser.py
#!/usr/bin/env python
# -*- coding: utf-8 -*-

"""Convenience module importing everything from backports.configparser."""

@kmike
Copy link
Member

kmike commented Jul 22, 2019

In https://github.com/scrapy/scrapy/pull/3877/files#diff-ab96eed25d6502f751749c9b001f7393 SafeConfigParser call was changed to ConfigParser, which seems to be "slightly backwards incompatible"; this PR fixes the problem with Python 2, but has the same issue. I wonder if we should revert the change in this file instead.

Initially I was thinking it may be also related to case sensitivity of file system, but lower-case configparser backport makes sense. Maybe some other code brings this library as a dependency. We shouldn't rely on it.

@kmike kmike added the bug label Jul 22, 2019
@elacuesta
Copy link
Member

In py3 (3.2 to be precise, but that's not supported here anymore) SafeConfigParser was renamed to ConfigParser (source) so there is no change there.
However, I must admit that it might have been bad move in my behalf to make the change in py2, although I suspect it shouldn't make an actual difference.

@kmike I'm not sure exactly what's the object that the six.moves module ends up using (and I'm too lazy to look it up 😛 ), what do you think about the following? It's more explicit IMHO.

if six.PY2:
    from ConfigParser import SafeConfigParser as ConfigParser
else:
    from configparser import ConfigParser

@kmike
Copy link
Member

kmike commented Jul 22, 2019

@elacuesta your proposed fix looks good to me.

@elacuesta
Copy link
Member

Ok, I'll open a quick PR just now, just in case we plan to release a bugfix version.

@elacuesta
Copy link
Member

@kmike Done, please see #3896

@marsam
Copy link
Author

marsam commented Jul 23, 2019

addressed by #3896

@marsam marsam closed this Jul 23, 2019
@marsam marsam deleted the fix-python2-configparser branch July 23, 2019 10:53
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.

Installing on CentOS7 (Python 2.7.5)
3 participants