FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

BUG: don't mutate list of fake libraries while iterating over it by mckib2 · Pull Request #18295 · numpy/numpy · GitHub

/ numpy Public

BUG: don't mutate list of fake libraries while iterating over it - #18295

Merged
charris merged 5 commits into
numpy:masterfrom
mckib2:bug-fix-fake-libs
Feb 4, 2021
ConversationCommits (5)ChecksFiles changed
Merged

BUG: don't mutate list of fake libraries while iterating over it#18295
charris merged 5 commits into
numpy:masterfrom
mckib2:bug-fix-fake-libs

Conversation

mckib2 commented Feb 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

seberg reviewed Feb 2, 2021
View reviewed changes
Comment thread numpy/distutils/command/build_ext.py Outdated

# Expand possible fake static libraries to objects
to_remove = []
for lib in libraries:

seberg Feb 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

mckib2 Feb 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

seberg Feb 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

libraries[:] will mean it iterates a copy of the list, which should be safe, basically undoing the PR you linked.

mckib2 Feb 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Changed to libraries[:] in latest push

seberg Feb 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would it be possible to create a short regression test for this (I am just fishing here, but every test helps).

mckib2 Feb 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mckib2 Feb 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Test added to protect against regression

seberg added this to the 1.20.1 release milestone Feb 2, 2021
seberg added the 09 - Backport-Candidate PRs tagged should be backported label Feb 2, 2021
This was referenced Feb 2, 2021
Merged
Closed
TST: add build test for build_ext fix (#1)
13603d4 
* TST: add build test for build_ext fix

* TST: clearer test name

* STY: use triple quotes instead of lists of strings

mckib2 commented Feb 3, 2021

Copy link
Copy Markdown
Contributor Author

Build errors appear not from this PR, see #18312

rgommers reviewed Feb 3, 2021
View reviewed changes
Comment thread numpy/distutils/command/build_ext.py

# Expand possible fake static libraries to objects
for lib in libraries:
for lib in libraries[:]:

rgommers Feb 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mckib2 Feb 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added an explanation is latest push, feel free to resolve or suggest different wording

charris commented Feb 3, 2021

Copy link
Copy Markdown
Member

The test failures look legitimate.

charris commented Feb 3, 2021

Copy link
Copy Markdown
Member

If the test becomes to convoluted we might want to omit it.

mckib2 commented Feb 4, 2021

Copy link
Copy Markdown
Contributor Author

The test failures look legitimate.

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.

mckib2 commented Feb 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

So I've reproduced the issue in a Docker container and there seems to be a spurious extra -c compile option given to the gfortran command (extra line breaks added for clarity):

------------------------------------------------------------ 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 127

Has 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.

charris merged commit d376319 into numpy:master Feb 4, 2021

charris commented Feb 4, 2021

Copy link
Copy Markdown
Member

Thanks @mckib2

charris pushed a commit to charris/numpy that referenced this pull request Feb 4, 2021
BUG: don't mutate list of fake libraries while iterating over it (num…
ac98bfb 
…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 mentioned this pull request Feb 4, 2021
Merged
charris added component: numpy.distutils and removed 09 - Backport-Candidate PRs tagged should be backported labels Feb 4, 2021
charris removed this from the 1.20.1 release milestone Feb 4, 2021
mckib2 deleted the bug-fix-fake-libs branch February 4, 2021 19:26
ilayn mentioned this pull request May 22, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

seberg seberg left review comments
rgommers rgommers left review comments

Assignees

No one assigned

Labels

00 - Bug 06 - Regression component: numpy.distutils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL