Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[2.7] bpo-11566: Extension build errors on Windows for _hypot #880

Merged
merged 1 commit into from
Dec 22, 2018

Conversation

thewtex
Copy link
Contributor

@thewtex thewtex commented Mar 28, 2017

This addresses C extension build errors related to an undefined _hypot
symbol when building with the Microsoft Visual C++ Compiler for Python
2.7 [1] or MinGWPy [2]. It also addresses errors when building a C++
extension with MinGWPy and C++11 from cmath, 'error "::hypot' has not
been declared'

On line 71 of PC/pyconfig.h, HAVE_HYPOT is defined, indicating that
hypot is expected to be available in current Windows toolchains.

[1] https://www.microsoft.com/en-us/download/details.aspx?id=44266

[2] https://mingwpy.github.io/

https://bugs.python.org/issue11566

@the-knights-who-say-ni

This comment has been minimized.

@thewtex

This comment has been minimized.

@zware zware changed the title bgo-11566: Extension build errors on Windows for _hypot bpo-11566: Extension build errors on Windows for _hypot Mar 28, 2017
This addresses C extension build errors related to an undefined _hypot
symbol when building with the Microsoft Visual C++ Compiler for Python
2.7 [1] or MinGWPy [2]. It also addresses errors when building a C++
extension with MinGWPy and C++11 from cmath, 'error "::hypot' has not
been declared'

On line 71 of PC/pyconfig.h, HAVE_HYPOT is defined, indicating that
hypot is expected to be available in current Windows toolchains.

[1] https://www.microsoft.com/en-us/download/details.aspx?id=44266

[2] https://mingwpy.github.io/
@rgommers
Copy link
Contributor

IIRC the MSVC hypot function is broken. I suggest you search the numpy codebase for HAVE_HYPOT; you'll see it's disabled for _MSC_VER < 1900 and reimplemented. That fix is well tested by now so possibly appropriate to just copy.

@thewtex
Copy link
Contributor Author

thewtex commented Apr 29, 2017

@rgommers Thanks for the pointer.

I was able to find the NumPy _MSC_VER check:

https://github.com/numpy/numpy/blob/8d01ee40eb00160068cce3cb02c930cebf013230/numpy/core/src/private/npy_config.h#L36

and a discrepancy between the NumPy backup implementation and the CPython one:

https://github.com/numpy/numpy/blob/8d01ee40eb00160068cce3cb02c930cebf013230/numpy/core/src/npymath/npy_math_internal.h.src#L196-L202

double hypot(double x, double y)

but the existing MSVC hypot is used prior to this patch and is still used by this patch. It passes tests:

def testHypot(self):
self.assertRaises(TypeError, math.hypot)
self.ftest('hypot(0,0)', math.hypot(0,0), 0)
self.ftest('hypot(3,4)', math.hypot(3,4), 5)
self.assertEqual(math.hypot(NAN, INF), INF)
self.assertEqual(math.hypot(INF, NAN), INF)
self.assertEqual(math.hypot(NAN, NINF), INF)
self.assertEqual(math.hypot(NINF, NAN), INF)
self.assertRaises(OverflowError, math.hypot, FLOAT_MAX, FLOAT_MAX)
self.assertTrue(math.isnan(math.hypot(1.0, NAN)))
self.assertTrue(math.isnan(math.hypot(NAN, -2.0)))

When looking into replacing it, does not looks trivial -- Py_INFINITY needs to be defined. We would have to namespace the name to not conflict with hypot from math.h, ... Since that seems to be a different issue, I would rather just get this one in.

I was able to dig up an old VS 2008 to test this patch.

@c0yote
Copy link

c0yote commented Dec 20, 2017

I'm experiencing this problem with regard to mingw-w64 builds. Is this likely to be accepted?

@jdemeyer
Copy link
Contributor

Also hitting this issue at sagemath/cysignals#76

@jdemeyer
Copy link
Contributor

IIRC the MSVC hypot function is broken.

It doesn't matter whether hypot is broken is not. The problem here is that the #define hypot _hypot is breaking code which doesn't involve hypot at all. Simply including the C+ header <cmath> on Mingw64 causes compiler errors.

dweindl added a commit to AMICI-dev/AMICI that referenced this pull request Jun 22, 2018
…ore Python.h is discouraged

This is an open python issue and should be fixed there (python/cpython#880). Windows workarounds exist.
dweindl added a commit to AMICI-dev/AMICI that referenced this pull request Jul 3, 2018
…rch, python ci in venv (#337)

* fix(python) Rename dsigmaydp -> dsigma_ydp also in extern declaration

* ci(python) Include python-sbml import and simulation in CI (Closes #338)

* fix(*) Replace sigma_* by sigma* in all C++ code to have more uniform naming in python- and matlab-generated code

* Add missing newly created model files

* Fix typo

* ci(python) Set exit status according to test outcome (Fixes #339)

* ci(python) Disable building python package with cmake-generated libraries until they are repaired to work with current setup.py. Use plain setuptools package.

* ci(python) Don't run tests/testSBML.py twice. Is already run as part of scripts/run-codecov.sh -> tests/testCoverage.py

* debug(python) Print info if hdf5 was found

* feature(python) Check for HDF5 in standard location if pkgconfig is unsuccesful

* Fix variable name

* added standard osx include and library dir hints

* Fix hdf5 library path

* Adapt hdf5 library path, be verbose

* ci(): List hdf5 files

* ci() Maybe we can find the static hdf5 library

* ci() List brew hdf5 files

* Adapt hdf5 library path

* c() Add travis macOS hdf5 directories, remove debug output

* fix(cmake) Don't use PYTHON_INCLUDE_PATH which was already deprecated in cmake 3.0.2 or earlier

* cmake() VERSION_GREATER_EQUAL only introduced in cmake 3.7, making this check mostly useless

* cmake() VERSION_GREATER_EQUAL only introduced in cmake 3.7, making this check mostly useless

* Recreate example model code

* Revert mingw ::hypot hypot _hypot fix, since ncluding STL headers before Python.h is discouraged

This is an open python issue and should be fixed there (python/cpython#880). Windows workarounds exist.

* ci(python) Install amici package to virtual environment and run tests there (Closes #342)

* cmake() Add target for python source distribution

* ci() install h5py

* Revert "ci() install h5py"

This reverts commit ea79054.

* Install h5py in venv

* Squashed commit of the following:

commit 985646a
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:52:18 2018 +0200

    ci(python) no --user in venv; install wheel

commit f991ce6
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:51:35 2018 +0200

    ci(python) no --user in venv; install wheel

commit 62f828b
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:30:57 2018 +0200

    ci(python) install pkgconfig

commit b3d37c9
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:21:41 2018 +0200

    ci (python) correct package name

commit 039ebb0
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:11:55 2018 +0200

    ci (python) venv activate

commit 49d5412
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 13:05:03 2018 +0200

    ci (python) venv activate

commit d1f5824
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 12:57:45 2018 +0200

    ci (python) venv activate

commit 1446015
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 12:49:14 2018 +0200

    ci (python) h5py

commit b88701d
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 12:42:56 2018 +0200

    ci(python) install sdist

commit 220323b
Author: Daniel Weindl <daniel.weindl@helmholtz-muenchen.de>
Date:   Tue Jul 3 12:15:40 2018 +0200

    ci(python) install --user

commit 190a270
Merge: 7adcc83 2e2e445
Author: Fabian Fröhlich <fabian@schaluck.com>
Date:   Tue Jul 3 10:02:21 2018 +0200

    Merge branch 'master' into feature_venv

* ci(python) python 3.6 venv to find proper symengine wheel on osx?

* ci(python) install python3.6

* ci(python) install python3.6
@jdemeyer
Copy link
Contributor

@embray Do you happen to have the powers to merge this (or know a friend who does)?

vinklein pushed a commit to vinklein/cysignals that referenced this pull request Aug 30, 2018
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Aug 30, 2018
- Add a reference to python/cpython#880.
- Remove some commented code.
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Aug 30, 2018
- Remove linebreak changes/
- Add a reference to python/cpython#880.
- Remove some commented code.
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Aug 30, 2018
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Aug 30, 2018
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
@terryjreedy terryjreedy changed the title bpo-11566: Extension build errors on Windows for _hypot [2.7] bpo-11566: Extension build errors on Windows for _hypot Dec 21, 2018
@terryjreedy
Copy link
Member

Closing and re-opening to trigger bedevere/news and appveyor CI

@terryjreedy terryjreedy reopened this Dec 21, 2018
@terryjreedy
Copy link
Member

If this is an issue for 3.x, we would normally patch master first and then backport.

@thewtex
Copy link
Contributor Author

thewtex commented Dec 21, 2018

If this is an issue for 3.x, we would normally patch master first and then backport.

This is a 2.7-only issue.

@methane
Copy link
Member

methane commented Dec 22, 2018

This is a 2.7-only issue.

Are you sure? master branch has same defines.
For MSVC, it is just a junk because we don't use VS2008 in the branch.
But for GCC, why this makes trouble for Python 2.7 but not 3.7?

Anyway, I prefer removing them in master branch.

@thewtex
Copy link
Contributor Author

thewtex commented Dec 22, 2018

Here is the PR for master: #11283

This patch is significant for people that want to create Python C extensions with binary compatibility for Python 2.7 distributed by Python.org or many other sources. This requires Visual Studio 2008, the Microsoft Visual C++ Compiler for Python 2.7, or MinGWPy. MinGWPy is currently only available for Python 2.7. The patch for masterwill help MinGW-w64, but this has far less use compared to the official Python binaries.

@methane methane added type-bug An unexpected behavior, bug, or error skip news labels Dec 22, 2018
@methane methane merged commit 000b809 into python:2.7 Dec 22, 2018
@methane
Copy link
Member

methane commented Dec 22, 2018

Thanks.

@thewtex
Copy link
Contributor Author

thewtex commented Dec 22, 2018

@methane thanks for merging!!

@thewtex thewtex deleted the hypot-definition-extension-clash branch December 22, 2018 04:16
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 4, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 4, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 11, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
Modify cysignals for windows

- (windows) Use POSIX macro in macro.h
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
- Remove some unecessary "#ifndef POSIX" in implementation.c and macro.h.
- In macro.h replace abort() and raise(SIGFPE) by raise(SIGABRT).
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
Modify cysignals for windows

- (windows) Use POSIX macro in macro.h
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
- Remove some unecessary "#ifndef POSIX" in implementation.c and macro.h.
- In macro.h replace abort() and raise(SIGFPE) by raise(SIGABRT).
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
Modify cysignals for windows

- (windows) Use POSIX macro in macro.h
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
- Remove some unecessary "#ifndef POSIX" in implementation.c and macro.h.
- In macro.h replace abort() and raise(SIGFPE) by raise(SIGABRT).
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 15, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 17, 2019
Modify cysignals for windows

- (windows) Use POSIX macro in macro.h
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
- Remove some unecessary "#ifndef POSIX" in implementation.c and macro.h.
- In macro.h replace abort() and raise(SIGFPE) by raise(SIGABRT).
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 17, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 17, 2019
Modify cysignals for windows

- (windows) Use POSIX macro in macro.h
- Remove linebreak changes.
- Add a reference to python/cpython#880.
- Remove some commented code.
- define SIGHUP SIGBUS SIGQUIT and SIGALRM for windows
- Implementation.c: reorder functions declaration to stick
with HEAD current order.
- Reorder code in many place to minimize git diff with HEAD.
- Remove some unecessary "#ifndef POSIX" in implementation.c and macro.h.
- In macro.h replace abort() and raise(SIGFPE) by raise(SIGABRT).
vinklein pushed a commit to vinklein/cysignals that referenced this pull request Jan 17, 2019
- Work around for hypot problem python/cpython#880
- Rebase on 582dbf6.
- Fix indentation and whitespace errors.
- Remove #define PTHREAD_STACK_MIN 65536
- Remove unecessary #include <float.h>
- simplify "int mapped_sig =" with "sig ="
- define sigjmp, sigsetjmp and siglongjmp in implementation.c
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
OS-windows skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.