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

numpy.distutils and distutils._msvccompiler compatibility #739

Merged
merged 6 commits into from
Aug 19, 2016
Merged

numpy.distutils and distutils._msvccompiler compatibility #739

merged 6 commits into from
Aug 19, 2016

Conversation

JGoutin
Copy link
Contributor

@JGoutin JGoutin commented Aug 16, 2016

  • Fix compatibility between numpy.distutils and distutils._msvccompiler. See Windows regression: wrong LIBPATH quoting on command-line (with numpy.distutils) #728 : Setuptools 24 msvc.py improvement import distutils._msvccompiler (New Python 3.5 C compiler for MSVC >= 14), but this one is not compatible with numpy.distutils (because not patched with numpy.distutils.ccompiler.gen_lib_options) and return unquoted libpaths when linking. The problem was patched in Numpy, but need to be patched also in Setuptools for compatibility between older versions of Numpy and distutils._msvccompiler (and indirectly Setuptools > 24).

Also some minor changes :

  • Replace some residuals except Exception.
  • PEP8.
  • Better history of the problem in Change.rst.

- Fix compatibility between `numpy.distutils` and `distutils._msvccompiler`. See #728 : Setuptools 24 `msvc.py` improvement import `distutils._msvccompiler` (New Python 3.5 C compiler for MSVC >= 14), but this one is not compatible with `numpy.distutils` (because not patched with `numpy.distutils.ccompiler.gen_lib_options`) and return unquoted libpaths when linking. The problem was patched in Numpy, but need to be patched also in Setuptools for compatibility between older versions of Numpy and `distutils._msvccompiler` (and indirectly Setuptools > 24).
- Replace some residuals `except Exception`.
@vallsv
Copy link
Contributor

vallsv commented Aug 16, 2016

Hi. I think there is a problem with import numpy.distutils as np_distutils. You dont check if the project is using numpy or not. Then it will force to use numpy.distutils anyway the project dotn want it. Maybe there is no problem with that?

Another way would be to check if CCompile.spawn is patched.

Here is an IPython check

> import distutils.ccompiler
> distutils.ccompiler.CCompiler.spawn.__module__
Out: 'distutils.ccompiler'
> import numpy.distutils 
> distutils.ccompiler.CCompiler.spawn.__module__
Out: 'numpy.distutils.ccompiler'

Then something like that:

if "numpy" in distutils.ccompiler.CCompiler.spawn.__module__:
        # apply the patch
        # you also can check that msvc14compiler.gen_lib_options.__module__ is not already patched

But it means numpy must be loaded before distutils. Or you have to check it that at execution time.

@JGoutin
Copy link
Contributor Author

JGoutin commented Aug 16, 2016

True, but setuptools is often imported first.

Fix on execution time is a good idea, I changed the code to try this. 👍

Please review it with your project.

After test without project and compilation, the good gen_lib_options is called if numpy.distutils was imported before running the patched function or not.

compatibility between "numpy.distutils" and "distutils._msvccompiler"
(for Numpy < 1.11.2)
"""
if "numpy" in distutils.ccompiler.CCompiler.spawn.__module__:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very specific. Testing for "numpy.distutils" in sys.modules sounds less hackish, no?

@vallsv
Copy link
Contributor

vallsv commented Aug 18, 2016

Hi, i will try that now.

BTW i think what you write on the comment about the numpy version is good.
Maybe it would be nice to check it programatically. It can avoid further problems, if numpy.distutils API is changed.

if distutils.version.StrictVersion(numpy.__version__) < distutils.version.StrictVersion("1.11.2"):
    # your patch

@vallsv
Copy link
Contributor

vallsv commented Aug 18, 2016

Hi again.

Nice job. Here it is working in all cases.

I test with:

  • from numpy.distutils.core import Extension
  • from setuptools import Extension
  • import first numpy.distutils
  • import first setuptools

Everything was fine on Windows x64, Python 3.5. I dont try other platforms.

@JGoutin
Copy link
Contributor Author

JGoutin commented Aug 18, 2016

Hi,

Thanks for the review.

👍 for the version check, I'll add this. Please, review it.

"""
if "numpy.distutils" in sys.modules:
import numpy as np
if StrictVersion(np.__version__) < StrictVersion('1.11.2'):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot use StrictVersion here, that's a broken braindead thing. You will get something like

ValueError: invalid version number '1.11.1rc1+fa248ad'

for any development version. This is true for many projects, not just numpy. From numpy 1.9.0 on numpy.lib.NumpyVersion is available, which does proper version comparisons. IIRC something similar that does the right thing for any PEP 440 compliant version string is present in distlib or another PyPA project.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry I'm late with testing/reviewing - just moved and without internet at home)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to send a fix for this if a setuptools dev can say which is the preferred version comparison function here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want pkg_resources.extern.packaging.version.LegacyVersion.

Copy link
Contributor

@vallsv vallsv Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Sorry for that, i ask for StrictVersion. But you can use LooseVersion without problem i guess.

In [6]: distutils.version.LooseVersion("1.11.1rc1+fa248ad") < distutils.version.LooseVersion('1.11.2')
Out[6]: True

Here is the patch #774 in case you like the idea.
Regards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont find pkg_resources.extern.packaging.version. But pkg_resources.parse_version, if you prefer

In [20]: pkg_resources.parse_version('1.11.1rc1+fa248ad') < pkg_resources.parse_version('1.11.2')
Out[20]: True
In [21]: pkg_resources.parse_version('1.11.3rc1+fa248ad') < pkg_resources.parse_version('1.11.2')
Out[21]: False

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with parse version is it will return a LegacyVersion when a proper Version can't be constructed and a LegacyVersion always compares less than a Version.

The reason for extern.packaging is because Setuptools vendors the packaging package, and extern exposes the package whether it is vendored or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more patch #775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants