BUG: fixes #7572, percent in path

[ssbarnea]

Fixes #7572 -- the failure to install numpy in virtual environments that are created in paths that do contain percent sign in them. Bug that occurs only with Python 3 because the changed behaviour of ConfigParser between py2 and py3.

Introduction of interpolation support in py3 introduce this bug, as now the code fails to load ini files with percent sign in them.

[ssbarnea]

@pv @charris please check this new PR which and let's home.

BTW, You mentioned something regarding line length but I was not able to find any usage of flake8 or pylint in numpy. Even the contribution documentation does not mention a thing about linting.

[pv]

It would be more backward compatible to escape percent signs.

[ssbarnea]

@pv Percent signs were never escaped in INI files. The ini/config RFC does not allow it. Python 2 did not allow them unless you used the SafeConfigParser (which was not used). So talking about being backwards compatible and not breaking things, it would mean not to try to do any interpolation at all.

This bug was harder to spot because Python team decided to change implementation of the ConfigParser class. Luckily they kept both others, allowing people to migrate to one that has a predictable / consistent behaviour. People that encountered this bug in the wild are usually just switching to RawConfigParser or SafeConfigParser. Probably a FancyConfigParser name would have been more appropriate name ;)

This would not have happened if they would have improved the parsing of the strings, but that's something for the Python issue tracker. Instead of throwing an exception when it finds a percent it would have just treat it as a normal percent instead of requiring you to escape it. The only blocks that were supposed to be interpolated were the ones with parentheses after them %(...). Anyway, we do know that we cannot fix Python now, but at least we can fix NumPy.

[charris]

Could you squash the commits and redo the commit messages as explained in doc/source/dev/gitwash/development_workflow.rst, especially with regards to 72 character line length.

@pv This makes sense to me, do you still have an objection?

[ssbarnea]

I am not sure on how to perform the squashing but somehow I managed to do something that put all the changes in a single commit, uses the 72 char limit, and uses the documented commit format. The only problem is that this commit is now on my master.

https://github.com/ssbarnea/numpy/commit/d73e877d72b012b4b0117980590579e516904c8f

[pv]

ConfigParser also on Python 2 does do interpolation, and this is the documented behavior. Change to RawConfigParser breaks this. Probably nothing in the wild depends on interpolation in Numpy's site.cfg files, but I note that a fully backward compatible fix would also exist.

[ssbarnea]

Only SafeConfigParser does interpolation in Python 2 and that's documented. And yes, we have reasons to believe that nobody is using these in the wild for numpy site.cfg files. If they would have used it would have worked only on py3 and not py2.

Believe me that if it was possible to use the SafeConfigParser I would have picked that approach. Still, changing the way the paths are injected into the site.cfg is much harder to fix.

My personal view is that is that enabling expansion in a ini file caused more problems than solves. Any occurrence of an percent sign will break existing configurations. File that used to load will stop loading because the percent is no double escaped.

As a personal note, I plan to propose a change in Python SafeConfigParser that would enable it to load files with percent in them instead of failing to load them with the current exception. If people want a warning would be raised but not an exception, which breaks existing code.

[charris]

@ssbarnea The way to do it is run git rebase -i HEAD~n in your branch, where n is the number of commits you have made. That will bring up a screen that will enable you to squash commits and edit commit messages, just read the documentation that comes up with it and replace the picks with the relevant letter. When that is done do a force push to origin git push --force origin <branch-name>. You probably want to clean up your master, just do git reset --hard <sha of last commit you want to keep>

[charris]

Only SafeConfigParser does interpolation in Python 2

The documentation says that interpolation is supported by ConfigParser, although SafeConfigParser is preferred when available. As I understand it, the problem is that python3 requires % to be escaped when it is not being used for interpolation whereas that was not the case in python2. Correct me if I am wrong.

[charris]

Just to be clear, the problem is with file paths generated by git-flow, correct? Is that on windows or linux or both? Also, does SafeConfigParser require escaping %?

[ssbarnea]

@charris Python documentation on ConfigParser module is confusing.

Here is an executive summary, regarding abiliy to load a value that contained a singe percent in it (unescaped):
py2.ConfigParser - true
py2.RawConfigParser - true
py2.SafeConfigParser - false
py3.ConfigParser - false
py3.RawConfigParser - true
py3.SafeConfigParser - false

Mainly Python 3 ditched the version that was able to load unescaped percent signs. I guess they were able to do this because almost nobody used this feature without explicitly using the SafeConfigParser.

The problem is that most of the users didn't need it and they just used the default. Now, if they are unlucky to get a perfect in their config file they will have to escape it which is against the RFC.

Based on current experience I would say that Python 3 ConfigParser class is no longer compliant with RFC 822. RawConfigParser still is.

If you want to see this for yourself you can check https://travis-ci.org/ssbarnea/test-configparser -- which is a project that I made specially for testing the behaviour of ConfigParser classes across different versions of Python. If you check the logs you will see the differences.

Yes, SafeConfigParser is the one that requires escaping and that's why we should not use it, as it would require us to expect escaped values, which is not supported by the RFC 822.

[dejlek]

I have encountered the same problem. We had to temporarily change the naming scheme and not use / in branch names...

[steveholden]

I'd really like a solution to this, as it's biting my team, though obviously it needs to be one that causes minimal (preferably no) disruption to existing projects. Given that RawConfigParser appears to be the only option with consistent behaviour across both Python major versions, its use would seem to be desirable. Unfortunately I'm not sufficiently familiar with the range of configurations used by Numpy users to determine how much breakage this might be expected to cause.

[charris]

It looks like there is no good solution, but this looks like the least bad ;) Let's give it a shot. Long term, if this gets fixed in Python 3 at some point we might want to copy the implementation in numpy for backwards compatibility. We might want to roll our own in any case.

It's a bit risky, but I think I'll backport this to 1.11.1. Thanks @ssbarnea .

[ssbarnea]

Thank you! My team will be happy not to have to use a patched version anymore :)