Link VTK privately

[jwnimmer-tri]

Per #7451, we don't allow #include <vtk...> in any headers, because we want to insulate our end-users from Drake's choice of VTK.

However, this doesn't fully protect users, because we dynamically link to VTK with public symbols and VTK does not version it's ABI symbols. A user can only load one copy of VTK into their process, and because Drake loads it's own build of VTK (currently VTK 9), that limits our compatibility choices vs our users.

I'd like to see Drake's binaries shipped in a way where our choices w.r.t. VTK to not constrain what other version(s) of VTK our users are allowed to use for their other code.

[jwnimmer-tri]

One way to accomplish this would be to add a new feature to a future major version of VTK (e.g., VTK 10) so that all of its code is placed within an inline namespace, e.g., inline namespace VTK10 { /* ... */ }. This should be source-compatible with all existing code, but would add the namespace into the binary symbol names, which means that the user could load two different major versions of VTK independently.

Possibly it should be done with a preprocessor macro and CMake configuration, in case there was some need for opt-out or for the user to choose a different namespace (for their local fork to get a new ABI). Or at worst, Drake could configure it to use inline namespace VTKdrake so that we're always unique.

This is what abseil does, and I think it works well.

[jwnimmer-tri]

@BetsyMcPhail or @svenevs or anyone else from Kitware here ... I think the next step here might be to submit this feature request into the VTK issue tracker, for broader circulation / discussion among the VTK development team?

If you agree, could I ask one of you to work on submitting that? I assume there will be a lead time to make this happen, so getting the ball rolling now seems useful.

[svenevs]

I have a meeting with somebody tomorrow to get that going, there are a number of facets per internal discussions. As a followup, there were two specific tension points. Are we seeking

1. Linking two potential VTK instances in the same process?
2. Linking two potential VTK instances in the same translation unit?

We believe (1) could be achievable, but there was significant doubt about (2). Beyond just deprecation policies and whatnot, there is an additional concern over things like OpenGL context creation that need to be investigated a bit more before opening up the discussion to the VTK discussion boards.

The plan at least for now is to make this an "opt-in" feature, with a macro default expanding to no namespace configurable via CMake. There are some additional considerations about third party libraries that get vendored but it may just result in a list of "this list of libraries cannot be vendored if you do opt-in".

My understanding was we're after (1) here, so that a user can link against e.g., drake as well as PCL without needing to recompile one or the other with the same version.

[jwnimmer-tri]

My understanding was we're after (1) here, so that a user can link against e.g., drake as well as PCL without needing to recompile one or the other with the same version.

Yes, precisely. Since no Drake headers ever include VTK, the option (2) "mixing different VTK within the same TU" is not relevant for Drake's use case. We only need (1), so that any VTK library loading choices that Drake makes will not limit the choice-of-VTK for our users' independent code.

[jwnimmer-tri]

... and just to complete the landscape, the other path forward would be to have an option to compile VTK statically with PIC (which is already possible), but also with all symbols hidden (which I don't think is possible yet?). That way, we could link it statically and privately, in which case the symbols' namespace doesn't matter.

[svenevs]

FYI related discussion on the VTK discourse here.

[jwnimmer-tri]

@svenevs Can you link to the VTK pull request for this feature?

[mathstuf]

https://gitlab.kitware.com/vtk/vtk/-/merge_requests/8993

[DamrongGuoy]

Recently Drake added VTK file reader into geometry/proximity #17725. It triggered a problem in Anzu.

Drake wants VTK 9, but another depending library (pcl://io) of Anzu wants VTK 7. Sammy did a workaround in Anzu to fix it.

CC: @sammy-tri @EricCousineau-TRI .

[jwnimmer-tri]

@svenevs It's difficult for me to look at https://gitlab.kitware.com/vtk/vtk/-/merge_requests/8993 and understand the status of this feature.

Could I ask you (or your teammates) to post a status update either to the 8993 MR, or to this issue? In particular:

• what tasks/bugs still remain,
• what is the timeline for completing the work?

For our own plan, we should also know whether it will make it into VTK 9.3 (coming in the next month or two, I expect?) or whether we'll plan to switch Drake's docker build of VTK to an unreleased pin of VTK in the interim.

[svenevs]

Inline namespaces are now merged into VTK master, additional information here! I believe the next steps here are:

1. Choose the inline namespace (drake_vtk, ... ?).

2. Examine our third party dependencies for native builds (and possibly wheel). I believe where possible we want externally required (apt, brew) C dependencies like libtiff, libpng. @jwnimmer-tri I recall at some point you were doing a workspace dependency audit? The following are all getting compiled / vendored internally by the VTK build:

   • exprtk
   • fmt
   • libharu
   • pugixml
   • utf8

3. Change the vtk/repository.bzl to use a recent commit rather than a released version of VTK, repackage artifacts.

[jwnimmer-tri]

For (1), the name drake_vtk is fine with me. Ideally it would be inline namespace drake_vtk __attribute__ ((visibility ("hidden"))) { ... } so that:

• the symbols are mangled for linking but not in our source code (the "inline" part); and
• the symbols are private in our archive (the "hidden" part).

(2) I am OK to keep this issue only about the VTK stuff, and roll that part out ASAP. We can open a separate ticket to improve our choices for VTK's externals, those are not as acutely painful as VTK itself. (If it ends up that we already need to play with them as part of rebuilding VTK binaries, that's fine, but we don't need to seek out changes there yet.)

(3) Yes. In particular, change macOS to use a downloaded archive (similar to Ubuntu), not a bottle. And for both macOS and Ubuntu, change the VTK rebuild scripts to output a pic static archive, not a dynamic library.

I think it's worth preparing a PR with all of these changes promptly. We might decide to hold off on landing it until a VTK tagged version with this feature is officially released, but probably we won't wait and in any case we need to de-risk that change by prototyping it now.

[mathstuf]

Ideally it would be inline namespace drake_vtk __attribute__ ((visibility ("hidden"))) { ... } so that:

I don't believe that visibility attributes apply to namespaces. Even if they did, there are attributes on the classes themselves to export them. Also, this would only work with a 100% static VTK anyways (as the shared libraries need to export their symbols).

We can open a separate ticket to improve our choices for VTK's externals, those are not as acutely painful as VTK itself. (If it ends up that we already need to play with them as part of rebuilding VTK binaries, that's fine, but we don't need to seek out changes there yet.)

Note that some external packages may require patches to VTK 8 to work with newer versions. These can probably be backported from more recent VTK development as needed.

Yes. In particular, change macOS to use a downloaded archive (similar to Ubuntu), not a bottle. And for both macOS and Ubuntu, change the VTK rebuild scripts to output a pic static archive, not a dynamic library.

What do you mean "a pic static archive"? VTK is dozens of libraries, not just one.

We might decide to hold off on landing it until a VTK tagged version with this feature is officially released

That's months away. Since this project requested the feature, it would be great if it were tested to be up-to-snuff here rather than waiting until an rc is made and we have to rush to fix things. If problems are found now, we can fix them and add tests to ensure they stay working into the future.

but probably we won't wait and in any case we need to de-risk that change by prototyping it now.

Agreed. :)

[jwnimmer-tri]

I don't believe that visibility attributes apply to namespaces

They do (with modern compilers).

Even if they did, there are attributes on the classes themselves to export them.

Ah right, of course. The namespace attribute is irrelevant when the class overrides it. We could define the macros like VTKIOCORE_EXPORT to hidden visibility, or maybe we could define it to be empty and use the namespace attribute.

In any case, the goal is to end up with hidden symbols, one way or another. That doesn't need to be in the first pass; certainly having drake_vtk in front is a good first step. Hiding the symbols is just an optimization.

What do you mean "a pic static archive"? VTK is dozens of libraries, not just one.

Right. I meant "pic static archives" plural.

Also, this would only work with a 100% static VTK anyways (as the shared libraries need to export their symbols).

There shouldn't be any shared libraries. Hopefully the subset of VTK that we need doesn't have any. I guess @svenevs will find out.

[svenevs]

We can open a separate ticket to improve our choices for VTK's externals

=> #17963

Yes. In particular, change macOS to use a downloaded archive (similar to Ubuntu), not a bottle. And for both macOS and Ubuntu, change the VTK rebuild scripts to output a pic static archive, not a dynamic library.

The games begin in #17962, it appears we're able to get away with inline namespace drake_vtk __attribute__ ((visibility ("hidden"))) but the builds for that are running locally. Mac may be a more challenging story, investigating.

[svenevs]

drake/tools/workspace/vtk/image/vtk-args

Line 5 in 8e2f4b0

CMAKE_CXX_STANDARD:STRING=17

It was mentioned that C++20 is inbound for jammy and macOS, but focal will stay 17. How should the different vtk archives be compiled? Currently they are all 17, but in the future will we want to have the mac and jammy ones be 20? I'm currently reworking other parts of the build steps and want to bake in the right C++ standard logic now.

Edit: and for the wheel? Stays 17 until we are building them on a later platform?

[jwnimmer-tri]

Good question. Probably getting ahead of the game with C++20 in your VTK branch now will be best:

• VTK on focal and manylinux: C++17
• VTK on jammy and macos and macos_wheel: C++20

Later today, I'll update #17943 with a final proposal for a planned set of flags. Before testing your new C++20 VTK linked into Drake on a C++20 platform, you'll l want to cherry-pick #17943 so that Drake matches the C++ version as well.

[jwnimmer-tri]

This has taken on a new urgency now that I realize OpenCV on Jammy pulls in VTK-9.1 and so directly fights with us on that platform.

I would like to have this issue fixed prior to the Thanksgiving break (at least on Ubuntu). If that means I pitch myself to help pair program, I'll do that.

[jwnimmer-tri]

From f2f:

We'll stick with prebuild downloaded binary archives for now... just to keep this moving.

For macOS, it's OK to have a single archive with form x86_64 and arm64 code. (Larger archive, but easier for us to build.)

The priority here is for Ubuntu releases (and in particular Ubuntu 22.04 non-wheel releases).

[EricCousineau-TRI]

Per Anzu issue 9729 (upgrade to Jammy), we are hitting symbol leakage issues most likely due to this issue.

From Anzu opencv_pydrake_test.py:

"""
This fails as follows when cv2 is imported *after*
pydrake:
    ImportError: /lib/x86_64-linux-gnu/libvtkParallelDIY-9.1.so.1: undefined symbol: _ZN3vtk6detail3smp21vtkSMPToolsImplForTBBExxxPFvPvxxxES2_
The above symbol translates to
    vtk::detail::smp::vtkSMPToolsImplForTBB(long long, long long, long long, void (*)(void*, long long, long long, long long), void*)
"""  # noqa
import pydrake
import cv2

Above reproducible on Ubuntu Jammy 22.04.1 (Apptainer image), in Anzu using:

• drake from source, 333ede3
• deb python-opencv:amd64, 4.5.4+dfsg-9ubuntu4 (and its dependencies)

Is it possible to update this to priority: high?
Is there anything that I can do to help resolving this?

[jwnimmer-tri]

Yes, this has been one of the top priorities since #16502 (comment), I just forgot to update the label.

[jwnimmer-tri]

I've confirmed that #18340 is a valid work-around, so I've re-lowered the priority here again.

Repeating from f2f: @svenevs please do keep working on this issue, but we probably won't review and land the PR until January.

[jwnimmer-tri]

I realized today that the #13514 people might be unhappy switching VTK to be a pre-compiled download. Possibly we should consider shipping an arm64 build of VTK10 for that use case? Alternatively, maybe we could keep 9.1 around (Ubuntu shared libraries) for Ubuntu arm64. To be discussed.

[jwnimmer-tri]

Re: keeping Ubuntu arm64 working... From discussion in today's meeting, we think the easiest way to do this would be the boring thing -- provide an arm64 binary build of our VTK, same as all of our other architectures. We don't have local machines to run this build, but we could manually launch an EC2 amd64 machine and build there.