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

VTK Dependency Audit #17963

Closed
4 tasks
svenevs opened this issue Sep 21, 2022 · 3 comments
Closed
4 tasks

VTK Dependency Audit #17963

svenevs opened this issue Sep 21, 2022 · 3 comments
Assignees
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters type: feature request

Comments

@svenevs
Copy link
Contributor

svenevs commented Sep 21, 2022

External dependencies for the VTK builds need to be inspected. From #16502 (comment)

Relates: #17962

There are three external dependencies shared between drake and the VTK tarball build:

  • eigen
  • png
  • zlib

There are also third party dependencies that the VTK build vendors that we can reduce:

  • freetype
    # extract from: tools/workspace/vtk/iamge/vtk_cmake_configure_args.py

    # Dependencies other parts of drake depend on directly get shared.  For the
    # .tar.gz builds these come from apt-get / brew.  Files involved:
    # - macOS
    #     - setup/mac/binary_distribution/Brewfile
    #     - tools/wheel/image/packages-macos
    # - ubuntu
    #     - tools/workspace/vtk/image/prereqs (for the docker build)
    #     - setup/ubuntu/binary_distribution/packages-focal.txt
    #     - setup/ubuntu/binary_distribution/packages-jammy.txt
    #     - tools/wheel/image/packages-focal
    #
    # These packages are the only ones we should require -dev from in apt.
    shared_external_dependencies = [
        "eigen",
        "png",
        "zlib",
    ]

Eigen

Drake installs this via the package manager (apt / brew). However, there are additional considerations.

https://gitlab.kitware.com/vtk/vtk/-/blob/fcc6af2c07bffcd918e89917f9aad592424dc846/ThirdParty/eigen/vtk_eigen.h.in#L26-L30

The implications:

  • Forces EIGEN_MPL2_ONLY (for the vtk build).

  • Does not affect VTK headers, only source files.

    $ git grep VTK_EIGEN
    Filters/FlowPaths/vtkLinearTransformCellLocator.cxx:#include VTK_EIGEN(Geometry)
    Filters/FlowPaths/vtkParallelVectors.cxx:#include VTK_EIGEN(Eigenvalues)
    Filters/FlowPaths/vtkParallelVectors.cxx:#include VTK_EIGEN(Geometry)
    Filters/FlowPaths/vtkVectorFieldTopology.cxx:#include VTK_EIGEN(Eigenvalues)
    Filters/FlowPaths/vtkVortexCore.cxx:#include VTK_EIGEN(Eigenvalues)
    Filters/FlowPaths/vtkVortexCore.cxx:#include VTK_EIGEN(Geometry)
    Filters/Statistics/vtkPCAStatistics.cxx:#include VTK_EIGEN(Dense)
    ThirdParty/eigen/vtk_eigen.h.in:# define VTK_EIGEN(x) <Eigen/x>
    ThirdParty/eigen/vtk_eigen.h.in:# define VTK_EIGEN(x) <vtkeigen/eigen/x>
  • If the user does "normal" drake,

    • The linux apt build will have used the same thing.
    • The macOS brew install might have been the same (depends on when we build vs user installed things).
  • If th user supplies their own Eigen, all bets are off

  • Switching to VTK vendored Eigen might be the right call?

png

Considerations for removing:

  • @jwnimmer-tri mentioned we might be able to update this file to use vtkPNGReader instead:

zlib

I don't think we should be doing anything about this one. There is, as far as I know, no way for the user to override the zlib choice. On Ubuntu it comes from apt-get, on macOS it comes from the system (xcode bundled and linked in statically).

If we did switch to the VTK one, it would be mangled, and therefore we could stop building it in the wheel: https://github.com/RobotLocomotion/drake/blob/894ac55aa493ecca4bf43379b61447fcc99fbe0c/tools/wheel/image/dependencies/projects/zlib.cmake

Freetype

While developing, we noticed that the vtkIOExport module forces a lot of dependencies on us that we do not want, e.g., vtkRenderingFreetype:

https://gitlab.kitware.com/vtk/vtk/-/blob/fcc6af2c07bffcd918e89917f9aad592424dc846/IO/Export/vtk.module#L15

We need that module for vtkGLTFExporter, and nothing else.

  1. (@svenevs) For drake, revisit creating a patch to strip out everything else from vtkIOExport to only build the exporter we need. This should be simple to create and maintain, but the original investigation took place while sorting out all of the other dependencies and I made a mistake.
  2. (@svenevs) The drake-specific hack will not be acceptable for VTK, in order to get something upstreamed and not do this we will want to.
    • Create a discourse post describing the desired change and why it is helpful.
    • Decide on how the modules will change (likely solution, e.g., is vtkGLTFExporter becomes its own module).
    • Decide on how deprecations for the module moves are going to work.
    • The deprecations are needed because we're changing the VTK build system interface.
    • Sticky: this may make the python bindings difficult during the transition process.
      • from vtkmodules.vtkA import vtkX now needs to be from vtkmodules.vtkB import vtkX when a class moves around
      • VTK would need some compat bridge to make vtkA still have vtkX (preferably with a deprecation warning)

Showing people how (1) works is the easiest way to get the discussion started, in our case it lets us get rid of the freetype dependency.

@svenevs svenevs added the component: build system Bazel, CMake, dependencies, memory checkers, linters label Sep 21, 2022
@EricCousineau-TRI
Copy link
Contributor

Per platform checklist, assigning @jwnimmer-tri as lead for delegation etc.

@jwnimmer-tri
Copy link
Collaborator

Updates from f2f today:

Most likely the only remaining work is to deal with these three libraries:

  • libpng
  • libtiff
  • libjpeg

See TODOs in @vtk_internal:

"deps_extra": [
# TODO(jwnimmer-tri) VTK is the only user of this library.
# We should write our own WORKSPACE rule to build it sensibly,
# or switch to VTK's vendored version.
"@libjpeg",
],
},

"deps_extra": [
# TODO(jwnimmer-tri) VTK is the only user of this library.
# We should write our own WORKSPACE rule to build it sensibly,
# or switch to VTK's vendored version.
"@libpng",
],

"deps_extra": [
# TODO(jwnimmer-tri) VTK is the only user of this library.
# We should write our own WORKSPACE rule to build it sensibly,
# or switch to VTK's vendored version.
"@libtiff",
],

The goal is to make sure all three of those are properly vendored, ideally with drake_vendor_foo prefix not just vtkfoo.

Remember that libtiff got angry last time we tried this. Figure out why.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Mar 16, 2024

@libpng was fixed in #20276
@libtiff was fixed in #20331.
@libjpeg was fixed in #20343.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters type: feature request
Development

No branches or pull requests

3 participants