BLD: Merge the npysort library into multiarray #17167
Conversation
|
So to be clear, the |
|
Well, perhaps unused would be better. It was being built as a library that was then linked into multiarray. This wasn't used anywhere else and wasn't installed for other people to link against. Doing it this way did force the code in I can't speak to why it was done this way, but perhaps @charris can. |
|
I think the goal is similar to the intent behind the What's the argument for making this change? |
|
In #16942, from where this change was extracted, this is needed so that the dtype transfer mechanism can be used in |
| @@ -941,7 +931,7 @@ def generate_umath_c(ext, build_dir): | |||
| ], | |||
| depends=deps + multiarray_deps + umath_deps + | |||
| common_deps, | |||
| libraries=['npymath', 'npysort'], | |||
| libraries=['npymath'], | |||
There was a problem hiding this comment.
#17127 shows that npymath is also not needed here, and in fact contains symbols that are already in the other object files which leads to build failures with strict linker settings.
There was a problem hiding this comment.
I don't know that we can just get rid of npymath, though, it gets installed to numpy/core/lib/ for other projects to link against. I'm not sure how common that is, but at least scipy is using it.
There was a problem hiding this comment.
This doesn't stop npymath from being built, it just stops it being passed to the linker since it does not contain any symbols that are not already available through the object files created from the compiled C files (and in fact contains many identically named functions).
|
@charris any thoughts? |
|
I don't have a problem with merging this. The original idea was to remove the original sort module that initialized the sort function pointers when it was loaded, so this just completes that removal. The reason it was made a library was the thought of making it available downstream as already mentioned here, but as there have been no requests for that functionality it is probably better to merge it with the |
This will no longer build npysort as library that is linked into multiarray. This library was not installed and was not linked to anything else.
765747f to
32ceb0b
Compare
|
Updated to mark things with |
|
Looks good to me. Does this need a release note? |
|
I don't think it needs a release note. The |
|
Thanks @ewmoore . Hmm, looks like |
This will no longer build npysort as library that is linked into
multiarray. This library was not installed and was not linked to
anything else.
This PR was extracted from #16942 as suggested there to make reviewing easier.