-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[BUG] pybind11_add_module forcing -fvisiblity=hidden is a bit too aggressive? #2479
Comments
Hm... Thinking about this more, option (3) may be a bit hypocritical on my part given that we don't explicitly annotate public visibility on those templates 😅 So perhaps (2) is the best for us (though I guess it's really to enable laziness), and (1) is still the simplest. |
We used to enforce "-fvisibility=hidden" globally via the pybind11_add_module(). Then, at some point, we added it to the pybind11 namespace, which basically resolved the binary size problem and removed the need for a global flag. I don't remember all the historical context why it had to be added back to pybind11_add_module, but it sounds like there were some tricky edge cases related to user code rather than pybind11 code bloat. (2) would be fine with me. |
Can't you just as easily unset it?
Then you don't need a new flag. And this is one of the benefits of using properties instead of piling flags on. As a minor improvement, we could respect |
Also, "advanced" users should be able to use the targets and helper function(s) - you have a lot more control that way. |
I'd also argue to try and keep the default way of using pybind11 (which we describe in the docs) the one that's most likely to be correct for the standard user. (It also would not be a backwards compatible change, right?) @henryiii's suggestion seems to suggest that adding an option to |
This issue also affects cythonized extension modules and breaks importing the built module with the following error:
If I change the flag to When I finally figured out why that ^^ error was happening, I did try the At the least I think it really needs a well-documented flag to un-force setting |
@sarnold Something else must be wrong. pybind11's
So I'm not sure what you're doing that somehow hides this symbol? If it's a Cython library that produces Meanwhile, not hiding these symbols could be quite tricky when it comes to libraries interacting, and ODR. |
It's a fork of the original converted to cmake, originally based on the cmake_example repo, and it uses pybind11 for the cmake python/extension/linker helpers on all 3 OS platforms (mainly trying to make it cleaner/compatible with all 3). Maybe not the primary use case, but if you're telling me not to do that, then that should probably also be clearly documented up front. Are you saying I should not use pybind11 at all? The branch I'm working on is here: https://github.com/freepn/py-re2/tree/pybind11 Sorry I can't really answer your question since I am not the author of the extension code, or else I would file a bug with more detail... |
@sarnold What you're doing is fine. The only problem is that, by circumbenting the |
Well, I don't think it's that weird that pybind11's build system and instructions are crafted to build pybind11 extensions? As far as I can see, that project is not using pybind11 anywhere (the library, which is the core of the project, not the build system). |
Just to second that, FindPython is very reasonable, and very similar to our tools (in fact, we pretty much can "just use it" and don't do any workarounds to speak of when you FindPython first). You need at least 3.18.2+, but with pyproject.toml, you can ensure this, or for a non-proper Python project, you can copy in FindPython* from CMake, it's designed to work standalone with CMake 3.7 (at least last I checked). Cython should probably fix their visibility issues, as setting this is not unreasonable at all. Finally, I don't think we ever added the ability to ignore adding this flag if
Do you have an example PR or something that shows what you tried? This really should work? |
Hi, Anyone knows how to change the pybind11 visibility from the CmakeFile ? I cannot read a DLL because it says "dllexport has default visibility, while pybind11:... has been declared with another visibiity". I guess by changing the the visibility to default will fix the issue Thank you in advance |
Issue description
In 97aa54fefa,
-fvisibility=hidden
was introduced topybind11_add_module
as a means to effectively avoid warnings (by means of settingCXX_VISIBILITY_PRESET
to"hidden"
).I ran into this on our downstream project (which uses a
pybind11
fork) here:RobotLocomotion/drake#14047
In the fork, I had previously stripped out that CMake code, then restored it when merging in the changes from more active development. (In this issue, I've provided the workaround, which is to re-set the visibility to "default".)
The long and short is, we (unfortunately) have very template-heavy public API and bindings, and setting
-fvisibility=hidden
in downstream units mean that some of the bound symbols may get new RTTI information, which we don't want in our project.From reading the comments in the commit (and the surround commit on current master), it looks like this preset isn't really necessary; it's really only to suppress errors in user's code who aren't sufficiently careful to curate their symbol visibility.
That's fine, but I would still love to not have to reverse that in code and try to track changes.
Reproducible example code
See downstream issue.
Possible solutions
From most conservative (keep non-expert users happy) to least conservative (require users be cognizant):
pybind11_add_module
talking about templates, RTTI, and visibility with bindings, and mention the workaround (which is now coupled to internal implementation details, albeit publicly documented).pybind11_add_module
to not foist hidden visibility on downstream code (instantiations).@rwgk @YannickJadoul @bstaletic @wjakob Thoughts?
The text was updated successfully, but these errors were encountered: