Skip to content
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

pybind11: Ensure all modules are compiled with hidden symbols #8435

Closed

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Mar 22, 2018

Closes #8433


This change is Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

+@jamiesnape for feature review, please.
+@jwnimmer-tri for platform review, please.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

:lgtm:

This is great to see.

Be sure to test in CI for macOS, to cover the new Werror.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

An existing defect, but I saw the following with GCC

cc1plus: warning: unrecognized command line option '-Wno-unknown-warning-option'
cc1plus: warning: unrecognized command line option '-Wno-#warnings'

@jamiesnape
Copy link
Contributor

@drake-jenkins-bot mac-sierra-clang-bazel-experimental please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please

@jamiesnape
Copy link
Contributor

:lgtm: (pending Mac CI)


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Mar 22, 2018

An existing defect, but I saw the following with GCC

Tinkered with it, but will defer fixing that for now:
#8437

@EricCousineau-TRI
Copy link
Contributor Author

@drake-jenkins-bot mac-sierra-clang-bazel-experimental please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please

@jwnimmer-tri
Copy link
Collaborator

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


tools/skylark/pybind.bzl, line 44 at r2 (raw file):

        copts = [
            "-fvisibility=hidden",
            # The following copts are per pybind11 deficiencies.

FYI I wonder if this would be slightly clearer if it returned to be above copts, but rephrased as "The warning disables below are per py..." or something. It's not 100% obvious that it covers both groups now.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

jamiesnape commented Mar 23, 2018

AbstractValue: a request to extract a value of type 'drake::systems::BasicVector<double>'
failed because the actual type was 'drake::systems::BasicVector<double>'.

@jwnimmer-tri
Copy link
Collaborator

That's consistent with having more than one typeinfo in play. The typeinfo requirement for process-wide uniqueness vs mach linker on macOS has been challenging in the past for us.


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tools/skylark/drake_cc.bzl, line 16 at r2 (raw file):

CXX_FLAGS = [
    "-Werror=all",
    "-Werror=attributes",

Also add to CMake build (see the comment just above this code block)


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Apr 5, 2018

Hm... From my understanding, this PR itself is a great improvement for link size, but not yet necessary at this point (since Pytohn on the platforms we care about use RTLD_LAZY).

If we are going to encounter run-time linking errors such as duplicate (and mismatched) typeids for the same types, I'd like to defer making this change until we refactor our Python modules to better handle this (i.e. better curation of cross-module dependencies for templated types). A potential solution would be to have the Python modules be linked statically, which is possible, but I'd prefer to have more time to develop something that is more cohesive to development (e.g. not waiting forever when focusing on a specific unittest, and not having to hack a manifest file everytime I wish to prune unneeded dependencies).

@jwnimmer-tri
Copy link
Collaborator

If this PR is stalled, maybe you could pull the last two commits into their own PR, since I think they are a strict improvement?

@EricCousineau-TRI
Copy link
Contributor Author

If this PR is stalled, maybe you could pull the last two commits into their own PR, since I think they are a strict improvement?

Done. Submitted #8537 (which resolves #8437 - gotta love those offsets)

@EricCousineau-TRI
Copy link
Contributor Author

Seems that I just got bit by the above symbol visibility mismatch with Mac in the following PR:
RobotLocomotion/drake-external-examples#98
Specifically failing in this Mac build:
https://travis-ci.org/RobotLocomotion/drake-shambhala/builds/363828263#L2747-L2756

Traceback (most recent call last):
  File ".../example_module_test.py", line 68, in <module>
    main()
  File ".../example_module_test.py", line 60, in main
    simulator.StepTo(1)
RuntimeError: LeafOutputPort::Calc(): Expected a vector output type for output port 0 of shambhala::(anonymous)::SimpleAdder<double> System ::_::shambhala/(anonymous)/SimpleAdder@00007fc640e0bf90 but got a drake::systems::Value<drake::systems::BasicVector<double>> instead.

Seems that this is a problem that would not be fixed by resolve this PR, nor the above solutions (compacting pydrake into a single statically-linked library).

Will post a separate issue.

@EricCousineau-TRI
Copy link
Contributor Author

Closing this for now. Can re-open later if need be.

@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pydrake: Modules should be compiled with hidden symbols
3 participants