Skip to content

Commit

Permalink
[workspace] Improve qhull hidden visibility
Browse files Browse the repository at this point in the history
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.

TODO deprecation
  • Loading branch information
jwnimmer-tri committed May 27, 2022
1 parent fbf7bc6 commit f947fd1
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 5 deletions.
4 changes: 2 additions & 2 deletions geometry/optimization/vpolytope.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
#include <memory>
#include <numeric>

#include <drake_vendor/libqhullcpp/Qhull.h>
#include <drake_vendor/libqhullcpp/QhullVertexSet.h>
#include <fmt/format.h>
#include <libqhullcpp/Qhull.h>
#include <libqhullcpp/QhullVertexSet.h>

#include "drake/common/is_approx_equal_abstol.h"
#include "drake/geometry/read_obj.h"
Expand Down
1 change: 1 addition & 0 deletions tools/workspace/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ drake_py_binary(
name = "vendor_cxx",
srcs = ["vendor_cxx.py"],
visibility = [
"@qhull//:__pkg__",
"@yaml_cpp_internal//:__pkg__",
],
deps = [":module_py"],
Expand Down
30 changes: 27 additions & 3 deletions tools/workspace/qhull/package.BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ load(
"@drake//tools/install:install.bzl",
"install",
)
load(
"@drake//tools/workspace:vendor_cxx.bzl",
"cc_library_vendored",
)

licenses(["notice"]) # Qhull

Expand Down Expand Up @@ -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.
Expand Down
27 changes: 27 additions & 0 deletions tools/workspace/qhull/patches/vendor_cxx.patch
Original file line number Diff line number Diff line change
@@ -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
+++ src/libqhullcpp/QhullQh.h
@@ -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

1 change: 1 addition & 0 deletions tools/workspace/qhull/repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

0 comments on commit f947fd1

Please sign in to comment.