[workspace] Use VTK inline namespace #17962
Conversation
There was a problem hiding this comment.
Some assorted results, probably missed some (there's a lot of repeated errors):
vtksys / vtkloguru
Issues finding dlclose, dlopen, dlerror, etc.
[8:09:18 PM] external/vtk/lib/libvtkloguru-9.2.a(loguru.cpp.o):loguru.cpp:function vtkloguru::stacktrace_as_stdstring[abi:cxx11](int): error: undefined reference to 'dladdr'
[8:09:18 PM] external/vtk/lib/libvtksys-9.2.a(DynamicLoader.cxx.o):DynamicLoader.cxx:function vtksys::DynamicLoader::CloseLibrary(void*): error: undefined reference to 'dlclose'
vtkRenderingOpenGL2
Issues finding the X windowing functions:
[8:09:22 PM] external/vtk/lib/libvtkRenderingOpenGL2-9.2.a(vtkXOpenGLRenderWindow.cxx.o):vtkXOpenGLRenderWindow.cxx:function drake_vtk::vtkXOpenGLRenderWindow::GetDesiredVisual(): error: undefined reference to 'XFree'
[8:09:22 PM] external/vtk/lib/libvtkRenderingOpenGL2-9.2.a(vtkXOpenGLRenderWindow.cxx.o):vtkXOpenGLRenderWindow.cxx:function drake_vtk::vtkXOpenGLRenderWindow::SetShowWindow(bool): error: undefined reference to 'XUnmapWindow'
Missing functions
[8:09:22 PM] external/vtk/lib/libvtkRenderingHyperTreeGrid-9.2.a(vtkHyperTreeGridMapper.cxx.o):vtkHyperTreeGridMapper.cxx:function drake_vtk::vtkHyperTreeGridMapper::UpdateWithDecimation(drake_vtk::vtkCompositeDataSet*, drake_vtk::vtkRenderer*): error: undefined reference to 'drake_vtk::vtkHyperTreeGridGeometry::New()'
[8:09:22 PM] clang: error: linker command failed with exit code 1 (use -v to see invocation)
# ...
[8:12:35 PM] external/vtk/lib/libvtkRenderingHyperTreeGrid-9.2.a(vtkHyperTreeGridMapper.cxx.o):vtkHyperTreeGridMapper.cxx:function drake_vtk::vtkHyperTreeGridMapper::UpdateWithDecimation(drake_vtk::vtkCompositeDataSet*, drake_vtk::vtkRenderer*): error: undefined reference to 'drake_vtk::vtkAdaptiveDataSetSurfaceFilter::New()'
[8:12:35 PM] external/vtk/lib/libvtkRenderingHyperTreeGrid-9.2.a(vtkHyperTreeGridMapper.cxx.o):vtkHyperTreeGridMapper.cxx:function drake_vtk::vtkHyperTreeGridMapper::UpdateWithDecimation(drake_vtk::vtkCompositeDataSet*, drake_vtk::vtkRenderer*): error: undefined reference to 'drake_vtk::vtkHyperTreeGridGeometry::New()'
[8:12:35 PM] external/vtk/lib/libvtkRenderingHyperTreeGrid-9.2.a(vtkHyperTreeGridMapper.cxx.o):vtkHyperTreeGridMapper.cxx:function drake_vtk::vtkHyperTreeGridMapper::UpdateWithDecimation(drake_vtk::vtkCompositeDataSet*, drake_vtk::vtkRenderer*): error: undefined reference to 'drake_vtk::vtkAdaptiveDataSetSurfaceFilter::SetRenderer(drake_vtk::vtkRenderer*)'
vtkJPEGReader
[8:09:38 PM] external/vtk/lib/libvtkRenderingOpenGL2-9.2.a(vtkOpenGLRenderWindow.cxx.o):vtkOpenGLRenderWindow.cxx:function drake_vtk::vtkOpenGLRenderWindow::GetNoiseTextureUnit(): error: undefined reference to 'drake_vtk::vtkJPEGReader::New()'
render gltf update
[8:09:45 PM] geometry/render_gltf_client/internal_render_engine_gltf_client.cc:183:2: error: "UpdateViewpoint can be removed, modified transform no longer needed."
[8:09:45 PM] #error "UpdateViewpoint can be removed, modified transform no longer needed."
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/image/vtk-args line 14 at r1 (raw file):
CMAKE_EXE_LINKER_FLAGS:STRING='-Wl,-Bsymbolic-functions -Wl,-z,now -Wl,-z,relro' CMAKE_MODULE_LINKER_FLAGS:STRING='-Wl,-Bsymbolic-functions -Wl,-z,now -Wl,-z,relro' CMAKE_SHARED_LINKER_FLAGS:STRING='-Wl,-Bsymbolic-functions -Wl,-z,now -Wl,-z,relro'
Working, PIC and static lib requirements thread.
I may have found the source of these flags, debian wiki on hardening. If you apt-get install devscripts you can get some more info:
$ hardening-check lib/libvtkRenderingOpenGL2-9.2.a
lib/libvtkRenderingOpenGL2-9.2.a:
Position Independent Executable: no, object archive (ignored)
Stack protected: yes
Fortify Source functions: yes (some protected functions found)
Read-only relocations: no, non-ELF (ignored)
Immediate binding: no, non-ELF (ignored)
Stack clash protection: yes
Control flow integrity: yesI don't know much about what's needed here for flags / verification, we've had CMAKE_POSITION_INDEPENDENT_CODE on the entire time.
We will probably have some similar questions to answer taking the macOS build outside of brew (likely a handful of flags injected by brew for building distributable binaries)?
Current archive URL if helpful: https://drake-packages.csail.mit.edu/vtk/vtk-9.2.0-1-focal-x86_64.tar.gz
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
946108b to
c465794
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
tools/workspace/vtk/build_mac_binary line 65 at r2 (raw file):
-DCMAKE_BUILD_TYPE:STRING=Release \ -DCMAKE_CXX_STANDARD:STRING=20 \ "-DCMAKE_OSX_ARCHITECTURES:STRING='x86_64;arm64'" \
Working, fastest route to success here will be to do two separate mac binaries -- x86_64 and arm64 separately. Our dependencies coming from brew are not multi-arch / fat binaries (building on arm64)
"_jpeg_stdio_src", referenced from:
drake_vtk::vtkJPEGReader::ExecuteInformation() in libvtkIOImage-9.2.a(vtkJPEGReader.cxx.o)
...
ld: symbol(s) not found for architecture x86_64
AFAIU that'll never change Homebrew/brew#10307
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
c465794 to
896847e
Compare
|
@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-everything-release please. |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
vtk_rendering_freetype_linkopts = ["-lfreetype"] if os_result.is_macos: vtk_rendering_freetype_linkopts = ["-L/opt/homebrew/lib", "-lfreetype"]
@jwnimmer-tri freetype is a hard dependency for us.
- I should add it to
setup/mac/binary_distribution/Brewfile? - The above is a hack that worked locally, is the right thing to do here make
tools/workspace/freetypeand usepkg-configon linux and brew on macOS?
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
tools/workspace/vtk/image/vtk-args line 254 at r3 (raw file):
VTK_MODULE_USE_EXTERNAL_VTK_glew:BOOL=ON VTK_MODULE_USE_EXTERNAL_VTK_hdf5:BOOL=ON VTK_MODULE_USE_EXTERNAL_VTK_jpeg:BOOL=ON
Working, macOS arm64 jpeg issues
//systems/sensors:lcm_image_array_to_images_test:
...
[ RUN ] LcmImageArrayToImagesTest.JpegTest
2022-11-01 17:37:02.948 ( 0.002s) [ A57B7B] vtkJPEGReader.cxx:183 ERR| vtkJPEGReader (0x131e06090): libjpeg could not read file from memory buffer: <ptr
I could get it to work if I linked to jpeg-turbo, need to inspect which one VTK is finding during its build. Regardless, jpeg or jpeg-turbo need to get promoted to a drake dependency.
|
@drake-jenkins-bot mac-x86-big-sur-clang-bazel-experimental-release please. |
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/build_mac_binary line 65 at r2 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Working, fastest route to success here will be to do two separate mac binaries -- x86_64 and arm64 separately. Our dependencies coming from
breware not multi-arch / fat binaries (building on arm64)"_jpeg_stdio_src", referenced from: drake_vtk::vtkJPEGReader::ExecuteInformation() in libvtkIOImage-9.2.a(vtkJPEGReader.cxx.o) ... ld: symbol(s) not found for architecture x86_64AFAIU that'll never change Homebrew/brew#10307
Done, we're building em separately.
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
@jwnimmer-tri
freetypeis a hard dependency for us.
- I should add it to
setup/mac/binary_distribution/Brewfile?- The above is a hack that worked locally, is the right thing to do here make
tools/workspace/freetypeand usepkg-configon linux and brew on macOS?
Working, I cannot for the life of me figure out why CI is not OK with adding -lfreetype. Works locally, we can use pkg-config for freetype2 on macOS via brew, but on focal and jammy we only install libfreetype6 binary package so the pkg_config_repository attempt broke.
Not sure what to look at other than that locally on focal -lfreetype for linkopts seems fine.
tools/workspace/vtk/image/vtk-args line 254 at r3 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Working, macOS arm64 jpeg issues
//systems/sensors:lcm_image_array_to_images_test: ... [ RUN ] LcmImageArrayToImagesTest.JpegTest 2022-11-01 17:37:02.948 ( 0.002s) [ A57B7B] vtkJPEGReader.cxx:183 ERR| vtkJPEGReader (0x131e06090): libjpeg could not read file from memory buffer: <ptrI could get it to work if I linked to
jpeg-turbo, need to inspect which one VTK is finding during its build. Regardless,jpegorjpeg-turboneed to get promoted to a drake dependency.
Done, this was challenging to diagnose but ultimately find_package(JPEG) in VTK was getting the arbitrarily defined {homebrew_prefix}/lib/libjpeg -- that will either come from jpeg or jpeg-turbo in brew depending on a couple of things.
The build script now forces VTK's CMake to find {homebrew_prefix}/opt/jpeg/ explicitly so that when jpeg comes back into the mix upon build it finds the same one as @libjpeg in drake. Previously the libjpeg installed in the lib directory was coming from jpeg-turbo, so the errors came up because at runtime it was trying to use a different library.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
setup/mac/binary_distribution/Brewfile line 19 at r5 (raw file):
brew 'graphviz' brew 'ipopt' brew 'jpeg-turbo'
Working, we can choose to use either jpeg-turbo or jpeg, however it should probably be jpeg since the @libjpeg dependency in drake is "regular jpeg".
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Working, I cannot for the life of me figure out why CI is not OK with adding
-lfreetype. Works locally, we can usepkg-configforfreetype2on macOS viabrew, but on focal and jammy we only installlibfreetype6binary package so thepkg_config_repositoryattempt broke.Not sure what to look at other than that locally on focal
-lfreetypeforlinkoptsseems fine.
Wow, ok I "figured it out" -- seems to be some kind of packaging issue.
# fresh docker jammy image, apt-get update/upgrade and install `libfreetype6`
$ root@8bf8b2160ab5:/# ls /usr/lib/x86_64-linux-gnu/libfreetype<TAB>
libfreetype.so.6 libfreetype.so.6.18.1
# install libfreetype6-dev
$ ls /usr/lib/x86_64-linux-gnu/libfreetype<TAB>
libfreetype.a libfreetype.so libfreetype.so.6 libfreetype.so.6.18.1When you just install libfreetype6 you don't get libfreetype.so, only libfreetype.so.6 and libfreetype.so.6.18.1. My vote: we install libfreetype6-dev, this is a dumb problem to try and solve in drake (some kind of magic bazel symlink?)
|
Previously, svenevs (Stephen McDowell) wrote…
That's actually how library packages always work on Debian/Ubuntu. To link against |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
That's actually how library packages always work on Debian/Ubuntu. To link against
-lfreetypewe would need thelibfreetype6-devinstalled so we have the.so=>.so.6symlink.
I see. Do you have a preference? I just pushed a "fix" that worked locally, you just give the linker the library path entirely.
Adding libfreetype-dev does add a bit to the footprint, but if we do that then I can resurrect the pkg-config version which is much cleaner.
As long as it works I don't have a very strong opinion, will pick this back up tomorrow 👍
|
@drake-jenkins-bot mac-x86-big-sur-clang-bazel-experimental-everything-debug please. |
There was a problem hiding this comment.
Reviewed 1 of 5 files at r3, 2 of 6 files at r4.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
I see. Do you have a preference? I just pushed a "fix" that worked locally, you just give the linker the library path entirely.
Adding
libfreetype-devdoes add a bit to the footprint, but if we do that then I can resurrect thepkg-configversion which is much cleaner.As long as it works I don't have a very strong opinion, will pick this back up tomorrow 👍
I think pkg-config will be better.
(Ideally of course, we'd disable the VTK modules that require freetype in the first place. It doesn't seem to me like Drake needs font support at all.)
|
@drake-jenkins-bot mac-x86-big-sur-clang-bazel-experimental-everything-debug please. |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I think pkg-config will be better.
(Ideally of course, we'd disable the VTK modules that require freetype in the first place. It doesn't seem to me like Drake needs font support at all.)
OK, @freetype repo created. I added an item to #17963 to investigate if / how we can remove freetype (requires digging around VTK a bit, we're currently actively depending on it via homebrew-director anyway).
Leaving this unresolved until we are ready to add freetype-dev to the AMIs.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
... we're currently actively depending on it via homebrew-director anyway.
We don't have director (drake-visualizer) enabled on macOS anymore, though?
tools/workspace/vtk/image/vtk-args line 251 at r8 (raw file):
VTK_MODULE_USE_EXTERNAL_VTK_exprtk:BOOL=OFF VTK_MODULE_USE_EXTERNAL_VTK_fmt:BOOL=OFF VTK_MODULE_USE_EXTERNAL_VTK_freetype:BOOL=ON
It seems like VTK has a freetype disablement knob. Have tried setting it OFF and seeing what happens?
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/image/vtk-args line 251 at r8 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It seems like VTK has a freetype disablement knob. Have tried setting it OFF and seeing what happens?
Then you cannot have vtkIOExport (render_gltf needs this) or vtkRenderingOpenGL2 (render_vtk needs this). It's probably not actually necessary for these dependencies in VTK, but currently the module system requires it. I can look into removing the freetype dependencies in VTK if we want, and we'll be able to update to that commit after it merges (and the dependency can be removed).
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
tools/workspace/vtk/repository.bzl line 849 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... we're currently actively depending on it via homebrew-director anyway.
We don't have director (drake-visualizer) enabled on macOS anymore, though?
Sorry that was not clear, the vtk@9.1.0 formula coming from homebrew-director internally depends on freetype
That's why macOS provisioned images can be used to test this PR (currently).
There was a problem hiding this comment.
Reviewed 1 of 13 files at r8.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/image/vtk-args line 251 at r8 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Then you cannot have
vtkIOExport(render_gltfneeds this) orvtkRenderingOpenGL2(render_vtkneeds this). It's probably not actually necessary for these dependencies in VTK, but currently the module system requires it. I can look into removing the freetype dependencies in VTK if we want, and we'll be able to update to that commit after it merges (and the dependency can be removed).
Whether for freetype or anything else, I fully expect us to need VTK source code patchfile(s) local to Drake to make VTK work for us. Cycling those patches through VTK upstream as a precondition for using them here might slow us down too much.
If's really just a few spurious lines in some CMakeLists, possibly nuking it with a Drake-local patchfile is not too bad?
Mostly, I'm somewhat worried about adding a new dynamic wheel dependency on freetype, whereas we didn't have one before (it was statically linked). On the other side, I don't want to statically link it for non-Wheel builds since it might conflict with the host system freetype. Yet we'd like to use the same vtk binaries in both wheel and non-wheel builds.
There are several ways to square that circle, but "print VTK with slash-and-burn patchfiles for spurious dependencies" seems like one of the better ones. Second best is probably to have a different VTK archive for wheel vs non-wheel. Third best is add freetype as an actual dependency and cross our fingers it doesn't blow anyone up.
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
de16f9b to
7e14974
Compare
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. |
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
7e14974 to
7e787af
Compare
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. @drake-jenkins-bot linux-focal-unprovisioned-clang-bazel-experimental-everything-debug please. |
There was a problem hiding this comment.
Reviewed 2 of 9 files at r26, 2 of 2 files at r27, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/BUILD.bazel line 7 at r27 (raw file):
name = "package", srcs = [ "package.py",
nit All of the image/foo.py should be listed as srcs, since we import them.
tools/workspace/vtk/BUILD.bazel line 21 at r27 (raw file):
"find_vtk_headers.py", "package.py", ] + glob(["image/*.py"]) + glob(["test/*.py"]),
nit I don't think we need anything here more than find_vtk_headers.py, do we? All of the other stuff is covered by drake_py_test and drake_py_binary, which themselves always induce linting. The only thing we should need to manually add to the linters are codes that are not mentioned by the BUILD file anywhere else.
tools/workspace/vtk/README.md line 10 at r25 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, DOCS
Can I at least get a quick dump here of the commands I'm supposed to use to re-create the images? I can't review more without that.
We can still defer the full docs for another push.
tools/workspace/vtk/repository.bzl line 933 at r27 (raw file):
# Install all files. files_to_install = [":vtk"]
nit We might as well just inline this constant into the literal string below. Keeping it separate is needlessly indirect.
tools/workspace/vtk/test/vtk_cxx_std_matches_drake_test.py line 48 at r27 (raw file):
drake_cxx_std = parse_cxx_std_from_bazelrc(bazelrc_path) vtk_cxx_std = cxx_std(codename) assert vtk_cxx_std == drake_cxx_std, (
Working
I'll push a rewrite of this file to use import unittest. No need to reinvent the wheel.
There was a problem hiding this comment.
Reviewed 1 of 9 files at r26.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 24 at r27 (raw file):
def fortify_flags() -> list[str]: fortify_compile_flags = " ".join(
Why are we adding these "fortify" compiler flags? We don't use them for any other externals in Drake.
For clarity: I can buy -Wno-deprecated-declarations to reduce build spam, and we do use that one in other externals. It's the FORTIFY... and relro... stuff that is dubious.
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 74 at r27 (raw file):
# VTK installs license files to `share/licenses/PROJECT_NAME`, we want # the license files to end up in `share/doc/PROJECT_NAME`. "-DCMAKE_INSTALL_LICENSEDIR:STRING=share/doc/vtk-9.2",
It seems overly brittle to hard-code "9.2" here.
Maybe we don't need a version number suffix here at all?
Or if we do need it, it will need to be automatically passed or configured from somewhere else so that it doesn't bitrot.
Code quote:
9.2
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
b857842 to
c78cdd1
Compare
- Build drake's copy of VTK as a static library, using the
VTK_INLINE_NAMESPACE=drake_vtk in a hidden namespace. This hides
all the VTK symbols behind namespace `drake_vtk::`.
- This fixes the issue where the copy of drake's VTK and the
system VTK have any collisions. For example, users using PCL
from Ubuntu apt (which depends on VTK from Ubuntu apt) will now
be able to link against both drake and PCL.
- NOTE: only one copy of VTK can initialize things like the
OpenGL context per process (users of PCL and drake must coerce).
- NOTE: third party libraries vendored by the VTK build are
properly mangled (in C) or hidden in a nested namespace (in C++).
However, the inline namespace does not apply, if your application
links against a VTK with an inline namespace that vendored the
same third party library as drake (e.g., vtktiff) there *WILL*
be multiple symbol definition issues. There is no easy way to
prevent this.
- Rewrite how the VTK .tar.gz archives build. Maintainers can use
`bazel run //tools/workspace/vtk:package` to build the archive.
- Linux: the build is in Docker, and builds all distributions for
the native processor architecture.
- macOS: you must build x86_64 natively on an x86_64 machine, same
for arm64.
- Dependency changes:
- Linux:
- TODO(svenevs): can't we delete some linux dependencies now?
- Wheel:
- lz4, jpeg, tiff, and double-conversion all now come from the
.tar.gz archive via the VTK vendored libraries.
- The wheel build no longer needs to build its own copy of VTK
since we are linking everything statically now.
- macOS:
- Stop using the now deprecated RobotLocomotion/homebrew-director
VTK bottle in favor of shipping a macOS .tar.gz archive.
- `libpng` is *NOT* new, drake has always dependended on this
indirectly from the RobotLocomotion/homebrew-director. This is
now explicitly marked as a dependency in drake's setup.
- `double-conversion`, `glew`, and `lz4` all now come from the
macOS .tar.gz archive via the VTK vendored libraries.
svenevs
force-pushed
the
feat/vtk-inline-namespace
branch
from
c78cdd1 to
5faab60
Compare
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
tools/workspace/vtk/BUILD.bazel line 7 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit All of the
image/foo.pyshould be listed as srcs, since we import them.
done, added, good catch
tools/workspace/vtk/BUILD.bazel line 21 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit I don't think we need anything here more than
find_vtk_headers.py, do we? All of the other stuff is covered bydrake_py_testanddrake_py_binary, which themselves always induce linting. The only thing we should need to manually add to the linters are codes that are not mentioned by the BUILD file anywhere else.
done, removed
tools/workspace/vtk/find_vtk_headers.py line 151 at r22 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, restore this, there are new vtk versions after what we are currently on
done, I'm the only person using this script, this script and it works exactly as I need it to. this will come up again as soon as a new vtk git reference is used.
tools/workspace/vtk/README.md line 10 at r25 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Can I at least get a quick dump here of the commands I'm supposed to use to re-create the images? I can't review more without that.
We can still defer the full docs for another push.
working, bazel run //tools/workspace/vtk:package -- -k, on linux builds both focal and jammy, on macOS builds the given arch of the machine building it.
while working on updating the documentation here, I've found some issues in terms of debugging and retracing steps needed to discover hidden dependencies like -ldl. For now, these are not changing in the PR, we're keeping them. Instead of implementing the fix now, can I file a follow-up issue and stub it in here on the readme as a TODO? We don't need these instructions now, it's just something we may need in the future.
tools/workspace/vtk/repository.bzl line 840 at r26 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working,
$ ls -1 wheelz/x_linux/drake.libs/ libblas-41474fda.so.3.9.0 libbz2-4488544d.so.1.0.4 libcrypt-ede83cdd.so.1.1.0* libdmumps_seq-5-295dcecb.2.1.so libesmumps-6-e3042930.so libgfortran-1488f340.so.5.0.0 libGLdispatch-80f42924.so.0.0.0 libGLX-74c0d009.so.0.0.0 libgomp-b6de5f8b.so.1.0.0 liblapack-86ce423d.so.3.9.0 liblzma-79a1dbc7.so.5.2.4 libmosek64-e462faa1.so.10.0* libmumps_common_seq-5-db5a68bc.2.1.so libOpenGL-9502a6a8.so.0.0.0 libpord_seq-5-7cc28ce8.2.1.so libquadmath-ab8ef433.so.0.0.0 libscotch-6-5e4d7497.so libscotcherr-6-43c7946f.so libtbb-fb00472a.so.12.6* libXt-f585519f.so.6.0.0Is forcing link against
@x11appropriate for wheel build?
still working on this part, will be discussing internally soon
tools/workspace/vtk/repository.bzl line 933 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit We might as well just inline this constant into the literal string below. Keeping it separate is needlessly indirect.
done, good call 👍
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 24 at r27 (raw file):
These are inherited:
drake/tools/workspace/vtk/image/vtk-args
Lines 10 to 14 in c3fee1d
When I looked into it originally after encountering them in the first VTK update, this ultimately came up: https://manpages.debian.org/testing/devscripts/hardening-check.1.en.html
And miscellaneous debian packaging guides ultimately discussing the flags being used for things that you intend to distribute to a lot of other people (like we're doing here). Beyond that though, I don't have much knowledge about how these work. There is a vestigial comment in the README:
- The
CMAKE_CXX_FLAGSfound inimage/vtk-argsin this directory, for example-D_FORTIFY_SOURCE=2, are excluded as they introduce additional complexity for the wheel build that is not desirable. Those flags are used inimage/vtk-argsin this directory to mirror whatbazelcompiles the rest of drake with, which does not apply to the wheel builds.
so I think the answer is
For clarity: I can buy
-Wno-deprecated-declarationsto reduce build spam, and we do use that one in other externals.
What's the right way for me to get access to the flags that are used for the other drake externals? A bazel file I can parse from python?
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 74 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It seems overly brittle to hard-code "9.2" here.
Maybe we don't need a version number suffix here at all?
Or if we do need it, it will need to be automatically passed or configured from somewhere else so that it doesn't bitrot.
done, share/doc/vtk is perfectly acceptable. extracted tarball layout will have share/doc/vtk/Copyright.txt and all the third party libraries are in folders beneath that.
I ran into a source of confusion, running bazel run //:install -- /tmp/drake with a newly packaged /tmp/drake/share/doc/vtk-9.2. This seems suspicious, but I copied the archive to a new computer and ran it and it shows up in /tmp/drake/share/doc/vtk.
tools/workspace/vtk/test/vtk_cxx_std_matches_drake_test.py line 48 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
I'll push a rewrite of this file to use
import unittest. No need to reinvent the wheel.
ok, thank you 🙂
There was a problem hiding this comment.
Reviewed 3 of 5 files at r28, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/README.md line 10 at r25 (raw file):
... I've found some issues in terms of debugging ...
I'm not sure what you're asking, exactly.
Maybe you're proposing writing tips for "how to debug build errors during a VTK upgrade". I don't think that kind of doc is necessary. Each upgrade will have its own tranche of errors, very likely completely different than what we hit in the prior upgrade. Investing time in writing up those kind of docs does not seem like it would ever pan out. At most, I would buy a google doc "lab notebook" style logbook that explains what you did during a certain upgrade. If you don't already have that, I don't think it's worth trying to recreate it from scratch. We can just make a new logbook next time.
tools/workspace/vtk/image/build_and_package_vtk.py line 25 at r27 (raw file):
), f"Expected {package_tree.source_dir} to be a directory." else: if not package_tree.source_dir.is_dir():
Doesn't this risk the wrong version being accidentally built on macOS?
Cloning does not actually take a long time, so we could do it repeatedly. At minimum, on macOS we need to switch the existing checkout to the correct tag or sha (or fail if we can't). Ideally we should also fail if the clone is not pristine, though that could be a TODO. Simplest is probably just to do the clone every time, from scratch. The few seconds is nothing compared to going off in the weeds because the source version wasn't what we thought it was.
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 24 at r27 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
These are inherited:
drake/tools/workspace/vtk/image/vtk-args
Lines 10 to 14 in c3fee1d
When I looked into it originally after encountering them in the first VTK update, this ultimately came up: https://manpages.debian.org/testing/devscripts/hardening-check.1.en.html
And miscellaneous debian packaging guides ultimately discussing the flags being used for things that you intend to distribute to a lot of other people (like we're doing here). Beyond that though, I don't have much knowledge about how these work. There is a vestigial comment in the README:
- The
CMAKE_CXX_FLAGSfound inimage/vtk-argsin this directory, for example-D_FORTIFY_SOURCE=2, are excluded as they introduce additional complexity for the wheel build that is not desirable. Those flags are used inimage/vtk-argsin this directory to mirror whatbazelcompiles the rest of drake with, which does not apply to the wheel builds.so I think the answer is
For clarity: I can buy
-Wno-deprecated-declarationsto reduce build spam, and we do use that one in other externals.What's the right way for me to get access to the flags that are used for the other drake externals? A bazel file I can parse from python?
Ah, okay. It wasn't clear to me that we were trying to mimic Bazel's default compile flags. We should put a comment that explains the source / motivation of these flags, in that case.
I think it's fine for now to just have a literal string copy of the flags Bazel is using, maybe with a TODO to try to cross-check them automatically. I don't think we need to automate anything in this PR.
I'll also note that the Bazel flags are not these anymore. For myself on Jammy (using bazel build -s to see the flags), we have -fstack-protector (no "strong") and -D_FORTIFY_SOURCE=1 (not 2).
Also incorrect is to call "no deprecated" a "fortify flag". It's just a regular flag.
This also begs the question of what the "Bsymbolic" etc stuff is doing. I don't think those are fortify flags, nor are they a default part of Bazel (TTMOBK).
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 74 at r27 (raw file):
I ran into a source of confusion, ...
Note that :install never removes any old files. Maybe you forgot to rm -rf /tmp/drake prior to running the install? I never run :install on its own, I'm always do the two-in-one rm -rf /tmp/drake && bazel run //:install -- /tmp/drake.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r28, 1 of 2 files at r29, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/repository.bzl line 81 at r27 (raw file):
fail(os_result.error) if os_result.is_macos and os_result.macos_arch_result == "x86_64":
I thought is_macos is false for macOS wheel builds. I'd assume they would currently fail to build now, because we're not checking or is_macos_wheel here?
tools/workspace/vtk/test/vtk_cxx_std_matches_drake_test.py line 48 at r27 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
ok, thank you 🙂
Done.
Free free to squash and force-push back to a single commit at any time.
There was a problem hiding this comment.
Reviewed 1 of 30 files at r24, 1 of 2 files at r29.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/repository.bzl line 923 at r29 (raw file):
) # Glob all files for the data dependency of //tools:drake_visualizer.
This doesn't smell right. The drake_visualizer build is not using this VTK build anymore.
tools/workspace/vtk/repository.bzl line 932 at r29 (raw file):
""" # Install all files.
This is a private build of VTK. We should not be installing its headers nor static libraries.
The only thing we should be installing are any required copyright license notices, including any required license notices of VTK's vendored libraries -- expat, double-conversion, etc.
os_result.is_macos does not return True for mac wheels
There was a problem hiding this comment.
Reviewed 2 of 5 files at r28, 2 of 2 files at r29, 2 of 2 files at r30.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
tools/workspace/vtk/README.md line 10 at r25 (raw file):
Just added this:
TODO(svenevs): in the existing infrastructure, change
BUILD_SHARED_LIBS
toONin vtk_cmake_configure_args.py
and the below section can still be used to identify the dependencies. To
help automate the process (this being a manual task is time consuming and easy
to make mistakes on) would be something like paraview-config.
Now that we've switched to static builds, it's harder to identify the external deps like dlopen etc. We know that the existing libraries' dependencies have not changed currently, but it was advised to me that the most robust way to actually identify what our link dependencies are is to do something like what paraview does for testing. I'd like to make this into an issue and tackle it after this PR, since for this PR, we don't need to change anything. It's more about "in the future, nobody knows how to do it correctly". The switching to shared libraries so that you can ldd it is OK-ish, but the process is extremely tedious (I always do it).
tools/workspace/vtk/repository.bzl line 81 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I thought
is_macosis false for macOS wheel builds. I'd assume they would currently fail to build now, because we're not checkingor is_macos_wheelhere?
done, yes, I will re-run macos wheels against this when I have new artifacts pending tiff investigation
tools/workspace/vtk/repository.bzl line 83 at r28 (raw file):
if os_result.is_macos and os_result.macos_arch_result == "x86_64": archive = "vtk-v9.2.0.rc2-1001-gd706250a14-mac-x86_64-1.tar.gz" sha256 = "0e7378fc5858313abd9f0ba53df4bf8977cdb4d101c617044ebfa15b90b0f46f" # noqa
working, single blocking thread: when everything else is resolved, a final repackaging (and upload to s3) should be done.
tools/workspace/vtk/repository.bzl line 923 at r29 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This doesn't smell right. The
drake_visualizerbuild is not using this VTK build anymore.
This predates this PR. My understanding was that the drake_visualizer folder depends on this, but I am OK to remove it if you think we should.
tools/workspace/vtk/repository.bzl line 932 at r29 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This is a private build of VTK. We should not be installing its headers nor static libraries.
The only thing we should be installing are any required copyright license notices, including any required license notices of VTK's vendored libraries -- expat, double-conversion, etc.
I think you are correct here. I don't know how to change the install logic to do that. I have concerns about what happens on the wheel front, I think the vtkmodules might still need the libs. Can investigate pending how to fix the bazel code here.
tools/workspace/vtk/image/build_and_package_vtk.py line 25 at r27 (raw file):
working
Simplest is probably just to do the clone every time, from scratch
I'm happy with this. I'm done doing core development so it's less painful now.
tools/workspace/vtk/image/vtk_cmake_configure_args.py line 24 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, okay. It wasn't clear to me that we were trying to mimic Bazel's default compile flags. We should put a comment that explains the source / motivation of these flags, in that case.
I think it's fine for now to just have a literal string copy of the flags Bazel is using, maybe with a TODO to try to cross-check them automatically. I don't think we need to automate anything in this PR.
I'll also note that the Bazel flags are not these anymore. For myself on Jammy (using
bazel build -sto see the flags), we have-fstack-protector(no "strong") and-D_FORTIFY_SOURCE=1(not 2).Also incorrect is to call "no deprecated" a "fortify flag". It's just a regular flag.
This also begs the question of what the "Bsymbolic" etc stuff is doing. I don't think those are fortify flags, nor are they a default part of Bazel (TTMOBK).
done, I think!
tools/workspace/vtk/test/vtk_cxx_std_matches_drake_test.py line 48 at r27 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done.
Free free to squash and force-push back to a single commit at any time.
LGTM thanks!
There was a problem hiding this comment.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
-- commits line 38 at r31:
working, per f2f lets use the vtk vendored eigen
There was a problem hiding this comment.
Reviewed 1 of 4 files at r32.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
geometry/render_gltf_client/test/internal_render_client_test.cc line 573 at r22 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, I commented out our only failing test for now to see if I can get green check marks
[12:53:39 AM] [ RUN ] RenderClientTest.LoadDepth16UTiffGood [12:53:39 AM] 2023-06-23 04:53:39.529 ( 0.024s) [ FB47780] vtkTIFFReader.cxx:1131 ERR| vtkTIFFReader (0x56301ad3f360): Problem reading slice of volume in TIFF file. [12:53:39 AM] geometry/render_gltf_client/test/internal_render_client_test.cc:577: Failure [12:53:39 AM] Expected equality of these values: [12:53:39 AM] depth [12:53:39 AM] Which is: [12:53:39 AM] Channel 0: [12:53:39 AM] 11.23200035095215 4.052000045776367 32.69800186157227 [12:53:39 AM] 0 11.23200035095215 4.052000045776367 [12:53:39 AM] [12:53:39 AM] CreateTestDepthImage() [12:53:39 AM] Which is: [12:53:39 AM] Channel 0: [12:53:39 AM] 0 1 2 [12:53:39 AM] 3 4 5
working, as expected, if we use the system @libtiff and compile VTK expecting to link that in later the test passes again. Resolving this may a bit involved, likely involving patching VTK and how it builds libtiff. Will need to finalize this PR up and revive the wheel building tiff / redo the macOS side and then new artifacts to see what CI does. If that is all successful then I will be updating the dependency audit #17963
tools/workspace/vtk/image/build_and_package_vtk.py line 25 at r27 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working
Simplest is probably just to do the clone every time, from scratch
I'm happy with this. I'm done doing core development so it's less painful now.
done, macOS and linux both blindly re-clone. the clone_vtk method now deletes input dest_dir if it exists to make the call-sites a little cleaner.
There was a problem hiding this comment.
Reviewed 3 of 4 files at r32, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @svenevs)
tools/workspace/vtk/README.md line 10 at r25 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Just added this:
TODO(svenevs): in the existing infrastructure, change
BUILD_SHARED_LIBS
toONin vtk_cmake_configure_args.py
and the below section can still be used to identify the dependencies. To
help automate the process (this being a manual task is time consuming and easy
to make mistakes on) would be something like paraview-config.Now that we've switched to static builds, it's harder to identify the external deps like
dlopenetc. We know that the existing libraries' dependencies have not changed currently, but it was advised to me that the most robust way to actually identify what our link dependencies are is to do something like what paraview does for testing. I'd like to make this into an issue and tackle it after this PR, since for this PR, we don't need to change anything. It's more about "in the future, nobody knows how to do it correctly". The switching to shared libraries so that you canlddit is OK-ish, but the process is extremely tedious (I always do it).
Ah. I would have thought the process to find any missing linking is bazel test //... in Drake. If that passes, it should be good. If it yells about missing things, the name of the missing symbol usually implies pretty well which library was omitted from the deps line(s).
tools/workspace/vtk/repository.bzl line 923 at r29 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
This predates this PR. My understanding was that the
drake_visualizerfolder depends on this, but I am OK to remove it if you think we should.
drake_visualizer uses VTK 8.
I do think we need to remove it here.
tools/workspace/vtk/repository.bzl line 932 at r29 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
I think you are correct here. I don't know how to change the install logic to do that. I have concerns about what happens on the wheel front, I think the
vtkmodulesmight still need the libs. Can investigate pending how to fix the bazel code here.
It's something like this:
load("@drake//tools/install:install.bzl", "install_files")
install(
name = "install",
docs = ["BSD-LICENSE"],
visibility = ["//visibility:public"],
)
Possibly you'll want something like a glob(["share/doc/**"]) for the docs = ..., instead of listing them all out. I imagine there a pretty good handful of licenses.
|
I'd like to pause this PR for now. Coordinating the dependencies between the VTK archive builder, wheel builder, and WORKSPACE is getting too complicated and doesn't really have any good fail-safes or cross-checks. It's all manual work, and requires painfully slow Dockerfile rebuilds while iterating. Because of that difficulty, I'd like to try moving VTK to the Bazel build wholesale, instead (see #19945). Bazel offers careful, interlocking checks about what depends on what, and can quickly incrementally rebuild only what's necessary as we evolve our choices of what modules we use and/or where the dependencies come from. We wanted to do this eventually anyway, to make everything easier (gdb debugging, version pin upgrades, hotfix patches to VTK source code, etc). Now I think the Dockerfile approach has become so difficult to be worth exploring the full Bazel rebuild end-game immediately. I've carried the Bazel experiment far enough that I'm pretty sure it will be tractable. There is still some bug with offscreen rendering stuff where I'm sure I have some build options wrong (causing unit test failures), but it seems like it will be surmountable. (I think the second-hand Kitware reports that using Bazel was intractable here were overly conservative, at least w.r.t. the subset of VTK that Drake needs.) @svenevs I'll continue cleaning up #19945 to see if I can get it passing CI (and on macOS, Wheel, etc). If so, we'll plan to take that route. If it hits a roadblock, we can circle back here and try to get the Dockerfile builds working, instead. |
|
Ok. While I agree with almost everything here in terms of the assessment of the infrastructure, I'll point out that getting the libtiff dependency back in (~30 minutes of work, plus repackaging which I just set and forget while working on other things) was the last thing. So this can be finished off fairly quickly if we need to
To my recollection, the bottleneck was that the rules foreign_cc didn't allow multi-core builds. We've got about 2500 objects to compile from VTK, so that adds quite a bit. But it could easily amortize out on a full rebuild given the rest of drake that needs to compile. I did not take a very close look at the other PR, one thing I will say is in the VTK build tree there is a Will wait for your direction before doing anything here. |
Not quite. It was still taking a bunch of my time to finish reviewing and polishing the code for maintainability, and to cross-check all of the library dependencies and their visibility. In the middle of doing that, I finally decided it was going to be pretty brittle overall and it's probably time to accelerate the push up to Bazel.
Yes, that was under the supposition that the CMake logic was complicated enough that we need to shell out to it, instead of writing BUILD files. Note that my other PR isn't using foreign_cc at all, not even just for the configure step. It creates BUILD rules files on the fly for each module we need (following their transitive closure of
Aha! Thanks, I hadn't seen that. I ended up re-implementing the same thing from scratch: That upstream json file seems to be generated a configure-time though, not committed into At minimum, I'll at least cross-check my implementation against the CMake code that makes that file. Possibly I'll also add a manual linter program that can run the CMake configure step and cross-check it against the Bazel rules, though maybe as a TODO. |
|
Got it, yeah the dependency coordination has been very cumbersome and I think if we go with your PR that self-resolves. Yes, |
Yeah, we might want manually-run linter cross-check during VTK pin upgrades. Currently any missing |
Closes #16502.
Work in progress.
This change is