API: Default to hidden visibility for API tables #26103
Conversation
This adds a new ``NPY_API_SYMBOL_ATTRIBUTE`` but defaults to using hidden visibility (always the case on windows!). That actually makes the situation "worse" for ``eigenpy`` in some degree, since it forces them to adapt, but it also allows them to decide to just roll with it (by actually also exporting it on windows which). Since it aligns windows and linux it seems like a good idea? OTOH, who knows how many projects get away with just not caring about windows... I tried to reorganize the docs a bit on how to import the array API...
|
This should be backported? |
|
Maybe? It only really matters if we think it helps clarify the situation for 2.0 and I am not certain. Maybe @mattip has an opinion on this? Otherwise, I think this may be nice but if it causes breakage it is probably niche enough that we can get away with it in a 2.1 release. |
|
Sounds like it needs more experimentation, so 2.1 is probably the better option. |
|
This seems like a good idea to me. It prevents potential problems I think, and having Windows and other OSes behave consistently here has to be better. Regarding symbol visibility, I'll note that:
|
|
Ah nice point, maybe another reason that adding it explicitly may be good to make downstream setups not accidentally rely on somtehing that is brittle (by build setup/system rules).
I guess we don't actually use it? Would be a nice cleanup (not now!) to remove all the |
|
We discussed it and landed on trying this for 2.1 (which means that this should be fine but the release notes/migration changes need to be moved around a bit). |
Cleanup would be nice indeed. We do use it, it's automatic. Try this (after you've built with $ meson introspect build --targets -i > targets.json
# now open targets.json and search for `hidden` |
|
OK, finally moved the notes in the last commit. This should be squash merged to avoid editing the release notes back and forth. |
|
Rebased (hopefully correctly), it would be good to check the docs reorg (the technical change is pretty small). |
doc/source/reference/c-api/array.rst
Outdated
|
|
||
|
|
||
| .. versionchanged:: | ||
| NumPy 2.0 exports the headers to avoid sharing the table outside of a |
There was a problem hiding this comment.
Now that this PR is targeting numpy 2.1 it's a bit of a shame that these docs won't be available for the 2.0 release. Maybe consider splitting off into a separate PR if that's not obnoxious or manually backporting after this is merged?
Also now that this is targeting 2.1, I think the versionchanged here is 2.1. The other versionadded above is 2.0.
doc/source/reference/c-api/array.rst
Outdated
|
|
||
| .. c:macro:: PY_ARRAY_UNIQUE_SYMBOL | ||
|
|
||
| .. c:macro:: NPY_API_SYMBOL_ATTRIBUTE | ||
|
|
||
| .. versionadded:: 2.0 |
|
In principle we could backport just the docs or even more (even if it says 2.1, I guess). I guess we could even backport the whole thing, so long we change the default to do nothing. Or just backport and delete all the new things, shouldn't be hard after all. |
|
Sorry for letting this one drop, let's bring this in. |
This adds a new
NPY_API_SYMBOL_ATTRIBUTEbut defaults to using hidden visibility (always the case on windows!).That actually makes the situation "worse" for
eigenpyin some degree, since it forces them to adapt, but it also allows them to decide to just roll with it (by actually also exporting it on windows which).Since it aligns windows and linux it seems like a good idea? OTOH, who knows how many projects get away with just not caring about windows...
I tried to reorganize the docs a bit on how to import the array API...
Should be ready, but for discussion: we have to make a decision that this is better and ideally improves theeigenpysituation (if just be clarifying it). EDIT: See gh-26098 and gh-26091.