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

Monkeypatch distutils.filelist.findall for Python 3.4.6 #971

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jcfr

jcfr commented Feb 14, 2017

Since the latest version of python 3.4.x series is 3.4.6 and it
does not include an fixed version of "distutils.filelist.findall",
this commit makes sure the patch is also applied.

For sake of comparison, this link highlights the "still broken"
implementation associated with v3.4.6:

https://github.com/python/cpython/blob/v3.4.6/Lib/distutils/filelist.py#L245-L273

And this link highlights the correct version found in Python 3.5.3:

https://github.com/python/cpython/blob/v3.5.3/Lib/distutils/filelist.py#L245-L273

Finally, simply using sys.version_info <= (3, 4, 6) would not work because
sys.version_info returns a tuple like (3, 4, 6, 'final', 0) which is in
fact greater than the other one:

>>> (3, 4, 6, 'final', 0) <= (3, 4, 6)
False

>>> (3, 4, 6, 'final', 0) <= (3, 4, 7)
True

>>> (3, 4, 6, 'final', 0) <= (3, 4, 6)
False

>>> (3, 4, 6) <= (3, 4, 6, 'final', 0)
True

>>> (3, 4, 6) < (3, 4, 6, 'final', 0)
True

It may be reasonable to revisit the monkey patch check to use
sys.version_info[:3] instead.

Monkeypatch distutils.filelist.findall for Python 3.4.6
Since the latest version of python 3.4.x series is 3.4.6 and it
does not include an fixed version of "distutils.filelist.findall",
this commit makes sure the patch is also applied.

For sake of comparison, this link highlights the "still broken"
implementation associated with v3.4.6:

  https://github.com/python/cpython/blob/v3.4.6/Lib/distutils/filelist.py#L245-L273

And this link highlights the correct version found in Python 3.5.3:

  https://github.com/python/cpython/blob/v3.5.3/Lib/distutils/filelist.py#L245-L273

Finally, simply using ``sys.version_info <= (3, 4, 6)`` would not work because
``sys.version_info`` returns a tuple like ``(3, 4, 6, 'final', 0)`` which is in
fact greater than the other one:

```
>>> (3, 4, 6, 'final', 0) <= (3, 4, 6)
False

>>> (3, 4, 6, 'final', 0) <= (3, 4, 7)
True

>>> (3, 4, 6, 'final', 0) <= (3, 4, 6)
False

>>> (3, 4, 6) <= (3, 4, 6, 'final', 0)
True

>>> (3, 4, 6) < (3, 4, 6, 'final', 0)
True

```

It may be reasonable to revisit the monkey patch check to use
``sys.version_info[:3]`` instead.
@jcfr

This comment has been minimized.

jcfr commented Feb 14, 2017

@jcfr jcfr referenced this pull request Feb 14, 2017

Merged

Add python3 support #196

jcfr added a commit to Radiomics/pyradiomics that referenced this pull request Feb 14, 2017

ci: manylinux: Fix sdist failure workaround distutils/setuptools issue
Since:
 (1) CircleCI automatically adds a symlink from /home/ubuntu/pyradiomics/venv
     to /home/ubuntu/virtualenvs/venv-system, (indeed it detects it is a
     python project)

 (2) the symlink is indeed invalid when "seen" from within the docker
     container building the project (because it mount the current folder
     into the dockcross/manylinux-x64 image),

 (3) distutils available in python 3.4.6 has an issue causing sdist to fail
     when it find an invalid symlink (http://bugs.python.org/issue12885)
     Setuptools is also failing to monkey patch for that version of python.
     A patch has been submitted upstream: pypa/setuptools#971

To workaround the issue, we simply remove the symbolic link and avoid the
problem completely.

jcfr added a commit to Radiomics/pyradiomics that referenced this pull request Feb 14, 2017

ci: manylinux: Avoid sdist failure with workaround for distutils/setu…
…ptools issue

Since:
 (1) CircleCI automatically adds a symlink from /home/ubuntu/pyradiomics/venv
     to /home/ubuntu/virtualenvs/venv-system, (indeed it detects it is a
     python project)

 (2) the symlink is indeed invalid when "seen" from within the docker
     container building the project (because it mount the current folder
     into the dockcross/manylinux-x64 image),

 (3) distutils available in python 3.4.6 has an issue causing sdist to fail
     when it find an invalid symlink (http://bugs.python.org/issue12885)
     Setuptools is also failing to monkey patch for that version of python.
     A patch has been submitted upstream: pypa/setuptools#971

To workaround the issue, we simply remove the symbolic link and avoid the
problem completely.
@jaraco

This comment has been minimized.

Member

jaraco commented Feb 23, 2017

Aha. That issue probably arose because I originally intended for the changes to be present in 3.4.6, but then backed them out when I realized I'd violated the configuration management policy. The right fix is just to make that a < (3, 5). Thanks for bringing this to my attention. I'll make the appropriate change.

@jaraco jaraco closed this Feb 23, 2017

jaraco added a commit that referenced this pull request Feb 23, 2017

@jcfr jcfr deleted the jcfr:fix-monkeypatch-findall branch Feb 23, 2017

@jcfr

This comment has been minimized.

jcfr commented Feb 23, 2017

Thanks for fix the issue 👍

Out of curiosity, is there any files where you acknowledge contributors ?

@jaraco

This comment has been minimized.

Member

jaraco commented Feb 23, 2017

Not a file. I hate duplicating metadata in source. Contributions are apparent from the commit graph and tracker (here).

@jcfr

This comment has been minimized.

jcfr commented Feb 23, 2017

Makes sense.

(Having spent quite some time to track this down ... I was then hoping to fix my PR and make it to the contributor graph 😄 . Next time.)

@jaraco

This comment has been minimized.

Member

jaraco commented Feb 23, 2017

Indeed, and I usually pull in the changes and overwrite them (as necessary). This change seemed small and trivial, hence the route I took. For sure next time!

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