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

Remove npymath library from cython extensions #527

Merged
merged 9 commits into from Dec 29, 2014

Conversation

Projects
None yet
3 participants
@omarocegueda
Contributor

omarocegueda commented Dec 29, 2014

When numpy is built with mingw, the npy_* functions exported from libnpymath.a reference some runtime math functions whose name has a leading underscore, like:
...
000008E5 REL32 00000000 86 _asinf
000008F5 REL32 00000000 85 _acosf
00000905 REL32 00000000 84 _atanf
...
If we try to link this library with visual c by renaming it to npymath.lib it will fail because the corresponding functions in the msvc runtime don't contain that leading underscore:
...
00000281 REL32 00000000 4F asinf
00000291 REL32 00000000 51 acosf
000002A1 REL32 00000000 53 atanf
...
This means that the library compiled with mingw is not compatible with visual c, and we get these errors:
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-34/builds/76/steps/shell_6/logs/stdio

This PR removes the npymath library from the linking step of the cython extensions, and defines the required subset of math functions in dpy_math.h (by borrowing some code directly from numpy).

With the proposed modification, the conflicting buildbots now pass:
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-33/builds/78
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-34/builds/80

#define DPY_PI NPY_PI
/* From numpy npy_math.c.src */

This comment has been minimized.

@matthew-brett

matthew-brett Dec 29, 2014

Member

Maybe specify which version (commit hash?) of numpy, for future updaters.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

Looks good to me. Omar - did you already try_branch these?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

I guess we should also remove the 'check_npymath' code from setup_helpers.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 29, 2014

Looks good to me. Omar - did you already try_branch these?

Yes, I tried yesterday on the two conflicting buildbots, and all the green ones, just to be sure we didn't break anything (we didn't). Now I'm trying with the new modifications.

I guess we should also remove the 'check_npymath' code from setup_helpers.
Sure!, I just removed it and added the commit info to dpy_math.h

What about explicitly requesting manifest creation to msvc? to prevent the checker from failing with versions 10.0+:
omarocegueda@a1068cd#diff-e16ea2bffa27db146eb98051d7eb346aR122

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

Omar - sorry - I didn't see this until just now - I had done another fix - what do you think of this one?

#530

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 29, 2014

Omar - sorry - I didn't see this until just now - I had done another fix - what do you think of this one #530

Much better!, can I merge #530 and rebase on top of it now? (I mean, do you need to add anything else to #530?)

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

Please go ahead and merge / rebase if you like #530 .

@omarocegueda omarocegueda force-pushed the omarocegueda:remove_npymath branch from a1068cd to 0fcd460 Dec 29, 2014

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 29, 2014

Rebased, thanks!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 29, 2014

Hey this is +1 here too. Don't wait for me to merge this. Go ahead and merge it when Travis is done. It's just getting a bit too late here. So, it seems that tomorrow if we are lucky we might be able to release... Thx for the hard work.

setup.py Outdated
ext_kwargs = get_info('npymath')
# We use some defs from npymath, but we don't want to link against npymath lib
ext_kwargs = {'include_dirs':get_info('npymath')['include_dirs']}

This comment has been minimized.

@matthew-brett

matthew-brett Dec 29, 2014

Member

Now we can go back to include_dirs=[np.get_include()] - I think - and not call get_info('npymath')

This comment has been minimized.

@omarocegueda

omarocegueda Dec 29, 2014

Contributor

We still need some defs from numpy, like this:
https://github.com/nipy/dipy/pull/527/files#diff-e4efbbc259c0bf121fb79b7f40ed1cc1R65

Right now we just need a couple defs but if later on we need other npymath functions we may need more.

This comment has been minimized.

@matthew-brett

matthew-brett Dec 29, 2014

Member

Do you mean defs from the header files? Then won't include_dirs=[np.get_include()] cover that?

This comment has been minimized.

@omarocegueda

omarocegueda Dec 29, 2014

Contributor

Nice!, I didn't know about that =)

}

This comment has been minimized.

@matthew-brett

matthew-brett Dec 29, 2014

Member

Extra blank line?

This comment has been minimized.

@omarocegueda

omarocegueda Dec 29, 2014

Contributor

removed

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

If travis-ci tests pass, let's merge this and see what the buildbots make of it. Thanks for your patience working through the comments.

@omarocegueda omarocegueda force-pushed the omarocegueda:remove_npymath branch from 7bcdffd to bef4785 Dec 29, 2014

matthew-brett added a commit that referenced this pull request Dec 29, 2014

Merge pull request #527 from omarocegueda/remove_npymath
MRG: Remove npymath library from cython extensions

npymath lib does not work for MSVC and numpy compiled with mingw.  Rather than work round this, copy the functions we need into `dpy_math.h` and use those.

@matthew-brett matthew-brett merged commit b048445 into nipy:master Dec 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment