BUG: don't mutate list of fake libraries while iterating over it #18295
Conversation
mckib2
commented
Feb 2, 2021
•
mckib2
commented
- Fixes regression causing Fortran static libraries in SciPy to not be found and linked on Windows 10 using MinGW-W64 (see BLD: Quadpack undefined references scipy/scipy#13072)
- not sure why this is causing an issue now (worked with previous versions), but with v1.20.0 it's not playing nicely with multiple Fortran libraries (EDIT: might be due to MAINT: Delete obsolete conversion to list #17052, this patch is good anyways because it makes it obvious what needs to happen rather than a list copy that looks unnecessary)
- simple bug reproducer
numpy/distutils/command/build_ext.py
Outdated
| @@ -570,12 +570,13 @@ def _process_unlinkable_fobjects(self, objects, libraries, | |||
| unlinkable_fobjects = list(unlinkable_fobjects) | |||
|
|
|||
| # Expand possible fake static libraries to objects | |||
| to_remove = [] | |||
| for lib in libraries: | |||
There was a problem hiding this comment.
If we know that libraries is a list, could also do libraries[:], but not sure that we do, so this is probably good. Should it even mutate the libraries that were passed in?
There was a problem hiding this comment.
The list does need to change or else the linker will try to link nonexistent or empty .lib files and crash. libraries is converted to a list at the top of this function
There was a problem hiding this comment.
libraries[:] will mean it iterates a copy of the list, which should be safe, basically undoing the PR you linked.
There was a problem hiding this comment.
Changed to libraries[:] in latest push
There was a problem hiding this comment.
Would it be possible to create a short regression test for this (I am just fishing here, but every test helps).
There was a problem hiding this comment.
A simple build test (exactly like the one linked in description) could do the job. I'm not familiar with the numpy conventions of how to do this. What I have in mind is building an extension and then testing to see if the extension exists (i.e., was built successfully). Is there a simple example you can point me to?
EDIT: looking now at tests for random extending and cooking up something similar to that, don't have a lot more time to look at this tonight, will probably have something tomorrow evening
There was a problem hiding this comment.
Test added to protect against regression
* TST: add build test for build_ext fix * TST: clearer test name * STY: use triple quotes instead of lists of strings
|
Build errors appear not from this PR, see #18312 |
| @@ -570,7 +570,7 @@ def _process_unlinkable_fobjects(self, objects, libraries, | |||
| unlinkable_fobjects = list(unlinkable_fobjects) | |||
|
|
|||
| # Expand possible fake static libraries to objects | |||
| for lib in libraries: | |||
| for lib in libraries[:]: | |||
There was a problem hiding this comment.
Can you add a comment above this line that [:] is on purpose, to not modify the original list? Otherwise the next over-eager code cleanup may undo the fix again.
There was a problem hiding this comment.
Added an explanation is latest push, feel free to resolve or suggest different wording
|
The test failures look legitimate. |
|
If the test becomes to convoluted we might want to omit it. |
Yes, was looking on mobile and didn't read correctly. I'll start looking at this right now. If it does indeed seem like a headache, will omit the tests. Fingers crossed for an easy fix. |
|
So I've reproduced the issue in a Docker container and there seems to be a spurious extra ------------------------------------------------------------ Captured stderr call ------------------------------------------------------------
error: Command "gfortran-5 -Wall -g -ffixed-form -fno-second-underscore -fPIC -O3 -funroll-loops
-I/home/numpy/venv/lib/python3.8/site-packages/numpy/distutils/tests/../../../numpy/core/include
-Ibuild/src.linux-i686-3.8/numpy/distutils/include -c -c ./_dummy1.f # <--- spurious "-c" argument here
-o build/temp.linux-i686-3.8/_dummy1.o" failed with exit status 127Has anyone encountered this before? This almost looks like another bug to me EDIT: this does not appear to be what's causing the error and might not be interesting at all EDIT EDIT: I've identified the issue: there is no Fortran 77 compiler available. This f2py test check prevents most of the tests from running in these Docker containers. The test in this PR needs a similar check. |
|
Thanks @mckib2 |
…py#18295) * BUG: don't mutate list of fake libraries while iterating over it * BUG: iterate over copy of list * TST: add build test for build_ext fix (#1) * TST: add build test for build_ext fix * TST: clearer test name * STY: use triple quotes instead of lists of strings * FIX: check for f77 compiler before test is run * DOC: add comment explaining that a list copy is necessary
charris
added
component: numpy.distutils
and removed
09 - Backport-Candidate
