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

BLD: rewrite np.distutils.exec_command.exec_command() #7862

Merged
merged 1 commit into from
Mar 11, 2017

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jul 21, 2016

exec_command() is currently a mess of several implementations using outdated Python APIs and various hacks. This rewrites it to use the standard subprocess module. See PR #7614 for previous discussion.

I've tested this on Python 2.7 and 3.5, Windows and Linux.

@pitrou
Copy link
Member Author

pitrou commented Jul 21, 2016

There's wacky failure on one of the Travis builders that I can't really understand...

@rgommers
Copy link
Member

There's wacky failure on one of the Travis builders that I can't really understand...

Looks odd indeed, I restarted that build to make sure it's a real issue.

@rgommers
Copy link
Member

Hmm, it fails reproducibly. Many times /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8).

@rgommers
Copy link
Member

Will be a nice cleanup, thanks @pitrou.

@rgommers
Copy link
Member

Scary piece of untested code to touch though:)

@pitrou
Copy link
Member Author

pitrou commented Jul 21, 2016

It does have tests in numpy/distutils/test_exec_command. As for the failure, it seems it's counting the number of warnings and for a reason fails if they are > 32? Perhaps the previous implementation didn't let the warnings through (it's difficult to make out, as the old code is so convoluted...).

@rgommers
Copy link
Member

Would be good to check also a Scipy build on Windows, to make sure we don't make the "too many open files" errors worse there. I recently had subprocess issues with that in another context, because it has a pretty bad bug on Python 2.7 with close_fds not working, so hard to work around.

@rgommers
Copy link
Member

It does have tests in numpy/distutils/test_exec_command.

Indeed it does, forgot about that. The redirect_stdout context manager there brings back some unpleasant memories:)

pitrou added a commit to pitrou/numba that referenced this pull request Jul 21, 2016
See numpy/numpy#7614 for context,
and numpy/numpy#7862 for the patch submitted upstream.
This should fix random failures on our Windows CI builders.
exec_command() is currently a mess of several implementations using outdated
Python APIs and various hacks.  This rewrites it to use the standard
subprocess module.  See PR numpy#7614 for previous discussion.
@pitrou
Copy link
Member Author

pitrou commented Jul 25, 2016

Hmm, it fails reproducibly. Many times /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8).

I've solved the issue by setting the locale to something existing in the build script. This was really a bug in the build script.

@pitrou
Copy link
Member Author

pitrou commented Jul 27, 2016

Would be good to check also a Scipy build on Windows

I've tried too, but I end up with "numpy.distutils.system_info.NotFoundError: no lapack/blas resources found". I guess someone more expert than me will have to try it out.

@juliantaylor
Copy link
Contributor

I guess there are no known problems on windows?
Its probably best to just merge it and see what happens then. Can you maybe rebase it again to get an up to date ci suite?

@juliantaylor juliantaylor merged commit f819cc1 into numpy:master Mar 11, 2017
@juliantaylor
Copy link
Contributor

thanks this was long overdue refactoring

@minrk
Copy link

minrk commented Mar 13, 2017

Bonus comment from #8767 for posterity: this patch allows parallel builds to actually run in parallel on macOS.

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

Successfully merging this pull request may close these issues.

None yet

4 participants