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

Windows regression: wrong LIBPATH quoting on command-line (with numpy.distutils) #728

Closed
pitrou opened this Issue Aug 8, 2016 · 20 comments

Comments

Projects
None yet
6 participants
@pitrou

pitrou commented Aug 8, 2016

During the upgrade from setuptools 23.0.0 to 25.1.6, it seems quoting of arguments in Windows command lines got broken (specifically when linking C extensions), which you can witness on those two AppVeyor builds:

This seems to be a setuptools issue since it's the only piece of the environment that changed between the two builds.

@anthrotype

This comment has been minimized.

Contributor

anthrotype commented Aug 8, 2016

Oh no, again? :(
Supposedly the behaviour was reverted with #725

@pitrou

This comment has been minimized.

pitrou commented Aug 8, 2016

Well, if you look at the failing build, the quoting is missing, which the reverted patch was supposed to fix (?). But we also use numpy.distutils, so perhaps there's an interaction with that?

@anthrotype

This comment has been minimized.

Contributor

anthrotype commented Aug 8, 2016

But we also use numpy.distutils, so perhaps there's an interaction with that?

Yes, I think that's it. In the google/brotli project, reverting the patch fixed the issue. Brotli does not use numpy.distutils.

https://ci.appveyor.com/project/quixdb/brotli/build/1.0.26

@pitrou

This comment has been minimized.

pitrou commented Aug 8, 2016

Well, we build against the Numpy C API, so it's not possible for us to disable numpy.distutils (which seems to monkeypatch itself into distutils as soon as it's imported...).

@rgommers

This comment has been minimized.

rgommers commented Aug 8, 2016

The only relevant recent change is to add msvc9 to the list of compiler classes that use a patchedgen_lib_options, in numpy/numpy@e149fac (in numpy 1.11.1). EDIT: it's also present in numpy 1.9.3 (last 1.9.x bugfix release) but missing from 1.10.x and 1.11.0.

For other compiler classes nothing has changed in ~10 years: https://github.com/numpy/numpy/blame/master/numpy/distutils/ccompiler.py#L602

@pitrou pitrou changed the title from Windows regression: wrong LIBPATH quoting on command-line to Windows regression: wrong LIBPATH quoting on command-line (with numpy.distutils) Aug 9, 2016

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 9, 2016

Here we had the same talk #694.
I think the problem is on numpy. They dont need anymore to monkey-patch some function to quote things.

@pitrou

This comment has been minimized.

pitrou commented Aug 9, 2016

I think the problem is on numpy. They dont need anymore to monkey-patch some function to quote things.

That doesn't sound like a plausible explanation. If you look at the logs, the builds fail because the arguments are not quoted anymore.

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 9, 2016

If you read the thread 694, you can see that the first issue come from 75282e5 Is it the same for you?

After that, we tried to fix the problem on setuptools and it was worst for people who was not using numpy.distutils. In fact setuptools does not monkey-patch anything relative to quotes.

I think numpy monkey-patch two function which change the quoting.

  • distutils.ccompile.spawn (numpy do not quote anymore things here, while the original does)
  • gen_lib_options (numpy quote things here)

I dont reach deeply on numpy then i can be wrong. But possibly the issue depend on the order of the import.

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 9, 2016

Oh then that's maybe it. setuptools does not use anymore msvc9 with the patched (by numpy) gen_lib_options, but the global spawn is still patched by numpy. Then nothing is anymore quoted.

@rgommers

This comment has been minimized.

rgommers commented Aug 9, 2016

The history is getting a bit hard to understand here, but it looks like commit 75282e5 was identified as having introduced the problem, and that commit was not properly reverted. Instead, there was a new fix for the reported issue (gh-694) tried (PR gh-715), which resulted in issue gh-722, a fix for that was then attempted in gh-723, and finally there was a partial revert in gh-725.

It looks to me like the thing to do is revert everything, then split commit 75282e5 (it's way too large) and re-commit only the parts that don't touch anything related to quoting.

Note that any change in either distutils or numpy.distutils is unlikely to give a satisfactory result, because there are a lot of released Python and Numpy versions that won't receive that fix. And, unlike setuptools, Python and Numpy cannot be simply/quickly upgraded to the latest version by users.

The other option that may work was this suggestion to patch gen_lib_options in setuptools in the same way as numpy does.

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 10, 2016

The problem is distutils should be the normal API. distutils choose to quote things on the spawn method. If setuptools choose to quote things on gen_lib_options they should force to not quote things on spawn (then 2 monkey-patches instead of nothing). And it will anyway change the semantic of the API.

What if numpy only patch spawn function on patched classes?

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 10, 2016

Here is another proposal for numpy. Just patch _msvccompile like any other compilers, while you also patch it's spawn function.

numpy/numpy#7925

It is working for my project.

@rgommers

This comment has been minimized.

rgommers commented Aug 10, 2016

The problem is distutils should be the normal API.

distutils doesn't have an API really:)

If setuptools choose to quote things on gen_lib_options they should force to not quote things on spawn (then 2 monkey-patches instead of nothing).

Need to be careful here that this works for both import orders of setuptools and numpy.distutils. It looks like asking for trouble, since the spawn functions in numpy have diverged quite a bit from distutils.

Here is another proposal for numpy. Just patch _msvccompile like any other compilers, while you also patch it's spawn function.

I'm happy to review/merge your numpy PR. But it's not a good alternative to a fix in setuptools I think, because it doesn't fix anything for released numpy versions. Note by the way that numpy doesn't directly touch or even import _msvccompile (it's private after all), so the patching of its spawn function is only indirect and due to distutils implementation details that may or may not change (likely not, given glacial speed of development there).

@JGoutin

This comment has been minimized.

Contributor

JGoutin commented Aug 16, 2016

In fact, the problem is not directly setuptools related but appear with setuptools 24 because it import distutils._msvccompiler (Which is the new C compiler for Python 3.5 and MSVC >= 14, msvc9compiler is now only available for compatibility.) and numpy.distutils was not compatible with it before the vallsv's PR.

It's good to fix it directly in Numpy, with this Numpy is really compatible with distutils._msvccompiler.

Like suggested by rgommers, I created a PR (#739) to fix this problem in setuptools also and avoid problems with older versions of Numpy. Please, review this patch on yours projects and Numpy (Not with the Numpy patch from vallsv) to see if that work correctly.

I hope this time this libpath quoting problem is solved, it was a little tricky to fix because not really related to Setuptools changes...

@pitrou

This comment has been minimized.

pitrou commented Aug 16, 2016

The problem, as @rgommers points out, is that the bug will still be there for older Numpy versions (for example the 1.9.x and 1.10.x branches).

@vallsv

This comment has been minimized.

Contributor

vallsv commented Aug 16, 2016

If numpy 1.11 is patched, it is already very nice no? I mean, do you really need to use the last version of setuptools when you use an older version of numpy? Sure it is better, but there is for sure a lot of combinations of libraries which are not working together.

@pitrou

This comment has been minimized.

pitrou commented Aug 16, 2016

Well, the PyPA will probably argue that any old version of setuptools is obsolete and shouldn't be used. setuptools is routinely upgraded to the latest version, while Numpy isn't: this is the core of the argument.

In the meantime, sure, we are pinning setuptools to 23.x in all our CI builds.

@JGoutin

This comment has been minimized.

Contributor

JGoutin commented Aug 16, 2016

We are finding a solution for this compatible with all previous Numpy versions, and also non numpy based projects (#739). You can test it with your projects for see if that work as intended.

@rgommers This is just by curiosity, but why not move distutils fixes and additions from numpy.distutils to setuptools ? Your fixes and new feature may be useful also for non Numpy users. And this may also avoid this kind of problem if future.

jabooth added a commit to menpo/menpo that referenced this issue Aug 18, 2016

Pin setuptools to 23.x
This is needed to avoid bugs with setuptools 25.x:
pypa/setuptools#728
@JGoutin

This comment has been minimized.

Contributor

JGoutin commented Aug 22, 2016

@jaraco Fixed in setuptools 25.3.0 (#739).

@rgommers

This comment has been minimized.

rgommers commented Sep 3, 2016

Fixed in setuptools 25.3.0

Thanks @JGoutin !

@rgommers This is just by curiosity, but why not move distutils fixes and additions from numpy.distutils to setuptools? Your fixes and new feature may be useful also for non Numpy users. And this may also avoid this kind of problem if future.

That would be a significant amount of work, but I can see some advantages. It would mean that setuptools then does/knows a lot more about compilers (incl C++ and Fortran), which would be good imho but it's up to setuptools devs to set a direction for that.

It's actually a similar question as "why don't we put all of distutils in setuptools? distutils is badly maintained, and it really doesn't belong in the stdlib (except the parts needed to build Python itself).

@jaraco jaraco closed this Sep 5, 2016

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