From 3fddbe19a69ba98a5603e6cffe7d888151bbf24d Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Thu, 19 May 2022 06:14:30 -0700 Subject: [PATCH] [workspace] Improve qhull hidden visibility When building C++ code, we should not use -fvisibility=hidden because it infects all #included code, even the standard library. This causes a lot of console spam during macOS builds, and is a typeinfo (vtable) problem even on Ubuntu, potentially leading to ODR violation. Instead, we should wrap the third-party C++ code in a inline hidden namespace. That way, only the third-party code itself is hidden, not anything that it #includes. --- geometry/optimization/vpolytope.cc | 4 +-- tools/workspace/BUILD.bazel | 1 + tools/workspace/qhull/package.BUILD.bazel | 30 +++++++++++++++++-- .../workspace/qhull/patches/vendor_cxx.patch | 27 +++++++++++++++++ tools/workspace/qhull/repository.bzl | 1 + 5 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 tools/workspace/qhull/patches/vendor_cxx.patch diff --git a/geometry/optimization/vpolytope.cc b/geometry/optimization/vpolytope.cc index 18834aa6adf7..71567978e253 100644 --- a/geometry/optimization/vpolytope.cc +++ b/geometry/optimization/vpolytope.cc @@ -6,9 +6,9 @@ #include #include +#include +#include #include -#include -#include #include "drake/common/is_approx_equal_abstol.h" #include "drake/geometry/read_obj.h" diff --git a/tools/workspace/BUILD.bazel b/tools/workspace/BUILD.bazel index c12609a9a3d0..d1e145cc3da8 100644 --- a/tools/workspace/BUILD.bazel +++ b/tools/workspace/BUILD.bazel @@ -47,6 +47,7 @@ drake_py_binary( name = "vendor_cxx", srcs = ["vendor_cxx.py"], visibility = [ + "@qhull//:__pkg__", "@yaml_cpp_internal//:__pkg__", ], deps = [":module_py"], diff --git a/tools/workspace/qhull/package.BUILD.bazel b/tools/workspace/qhull/package.BUILD.bazel index ab3fae4a22a9..47ab55ee0352 100644 --- a/tools/workspace/qhull/package.BUILD.bazel +++ b/tools/workspace/qhull/package.BUILD.bazel @@ -4,6 +4,10 @@ load( "@drake//tools/install:install.bzl", "install", ) +load( + "@drake//tools/workspace:vendor_cxx.bzl", + "cc_library_vendored", +) licenses(["notice"]) # Qhull @@ -53,15 +57,35 @@ _SRCS_CPP = [ ] cc_library( - name = "qhull", - hdrs = _HDRS_C + _HDRS_CPP, + name = "qhull_r", + hdrs = _HDRS_C, copts = [ "-fvisibility=hidden", ], includes = ["src"], - srcs = _SRCS_C + _SRCS_CPP, + srcs = _SRCS_C, + linkstatic = 1, +) + +cc_library_vendored( + name = "qhull", + hdrs = _HDRS_CPP, + hdrs_vendored = [ + x.replace("src/libqhullcpp/", "drake_src/drake_vendor/libqhullcpp/") + for x in _HDRS_CPP + ], + includes = ["drake_src"], + edit_include = { + "libqhullcpp/": "drake_vendor/libqhullcpp/", + }, + srcs = _SRCS_CPP, + srcs_vendored = [ + x.replace("src/", "drake_src/drake_vendor/") + for x in _SRCS_CPP + ], linkstatic = 1, visibility = ["//visibility:public"], + deps = [":qhull_r"], ) # Install the license file. diff --git a/tools/workspace/qhull/patches/vendor_cxx.patch b/tools/workspace/qhull/patches/vendor_cxx.patch new file mode 100644 index 000000000000..ea3b6cfb71e6 --- /dev/null +++ b/tools/workspace/qhull/patches/vendor_cxx.patch @@ -0,0 +1,27 @@ +For private access within the module, don't use out-of-namespace friendship with +an `extern "C"` function. This doesn't work with drake's namespace vendoring. + +Instead, just make the fields public. + +--- src/libqhullcpp/QhullQh.h.orig 2022-03-11 15:34:46.350243223 -0800 ++++ src/libqhullcpp/QhullQh.h 2022-03-11 15:34:33.294070367 -0800 +@@ -58,7 +58,7 @@ + #//!\name Constants + + #//!\name Fields +-private: ++public: + int qhull_status; //!< qh_ERRnone if valid + std::string qhull_message; //!< Returned messages from libqhull_r + std::ostream * error_stream; //!< overrides errorMessage, use appendQhullMessage() +@@ -66,8 +66,9 @@ + double factor_epsilon; //!< Factor to increase ANGLEround and DISTround for hyperplane equality + bool use_output_stream; //!< True if using output_stream + ++private: + //! modified by qh_fprintf in QhullUser.cpp +- friend void ::qh_fprintf(qhT *qh, FILE *fp, int msgcode, const char *fmt, ... ); ++ //friend void ::qh_fprintf(qhT *qh, FILE *fp, int msgcode, const char *fmt, ... ); + + static const double default_factor_epsilon; //!< Default factor_epsilon is 1.0, never updated + diff --git a/tools/workspace/qhull/repository.bzl b/tools/workspace/qhull/repository.bzl index 5454cda3fd87..5d70f13d9495 100644 --- a/tools/workspace/qhull/repository.bzl +++ b/tools/workspace/qhull/repository.bzl @@ -13,6 +13,7 @@ def qhull_repository( build_file = "@drake//tools/workspace/qhull:package.BUILD.bazel", patches = [ "@drake//tools/workspace/qhull:patches/disable_dead_code.patch", + "@drake//tools/workspace/qhull:patches/vendor_cxx.patch", ], mirrors = mirrors, )