Skip to content

Commit

Permalink
[workspace] Improve qhull hidden visibility (#17229)
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.

We are substantially patching and subsetting this library to weave it into
Drake. We shouldn't give the illusion to users that it's available for reuse
downstream; therefore, we'll add an "_internal" suffix and deprecate the
original repository name.
  • Loading branch information
jwnimmer-tri authored Jun 2, 2022
1 parent 9c9e575 commit b7c0b58
Show file tree
Hide file tree
Showing 10 changed files with 119 additions and 12 deletions.
2 changes: 1 addition & 1 deletion geometry/optimization/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ drake_cc_library(
"//solvers:mosek_solver",
"//solvers:scs_solver",
"//solvers:solve",
"@qhull",
"@qhull_internal//:qhull",
],
)

Expand Down
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
4 changes: 3 additions & 1 deletion tools/workspace/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ drake_py_binary(
name = "vendor_cxx",
srcs = ["vendor_cxx.py"],
visibility = [
# These should all be of the form "@foo_internal//:__pkg__".
"@qhull_internal//:__pkg__",
"@yaml_cpp_internal//:__pkg__",
],
deps = [":module_py"],
Expand Down Expand Up @@ -86,7 +88,7 @@ _DRAKE_EXTERNAL_PACKAGE_INSTALLS = ["@%s//:install" % p for p in [
"org_apache_xmlgraphics_commons",
"petsc",
"pybind11",
"qhull",
"qhull_internal",
"sdformat",
"spdlog",
"tinyobjloader",
Expand Down
11 changes: 9 additions & 2 deletions tools/workspace/default.bzl
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# -*- python -*-

load("@drake//tools/workspace:deprecation.bzl", "add_deprecation")
load("@drake//tools/workspace:mirrors.bzl", "DEFAULT_MIRRORS")
load("@drake//tools/workspace:os.bzl", "os_repository")
load("@drake//tools/workspace/abseil_cpp_internal:repository.bzl", "abseil_cpp_internal_repository") # noqa
Expand Down Expand Up @@ -74,7 +75,7 @@ load("@drake//tools/workspace/pycodestyle:repository.bzl", "pycodestyle_reposito
load("@drake//tools/workspace/pygame_py:repository.bzl", "pygame_py_repository") # noqa
load("@drake//tools/workspace/python:repository.bzl", "python_repository")
load("@drake//tools/workspace/qdldl:repository.bzl", "qdldl_repository")
load("@drake//tools/workspace/qhull:repository.bzl", "qhull_repository")
load("@drake//tools/workspace/qhull_internal:repository.bzl", "qhull_internal_repository") # noqa
load("@drake//tools/workspace/ros_xacro:repository.bzl", "ros_xacro_repository") # noqa
load("@drake//tools/workspace/rules_pkg:repository.bzl", "rules_pkg_repository") # noqa
load("@drake//tools/workspace/rules_python:repository.bzl", "rules_python_repository") # noqa
Expand Down Expand Up @@ -255,7 +256,13 @@ def add_default_repositories(excludes = [], mirrors = DEFAULT_MIRRORS):
if "qdldl" not in excludes:
qdldl_repository(name = "qdldl", mirrors = mirrors)
if "qhull" not in excludes:
qhull_repository(name = "qhull", mirrors = mirrors)
add_deprecation(
name = "qhull",
date = "2022-10-01",
cc_aliases = {"qhull": "@qhull_internal//:qhull"},
)
if "qhull_internal" not in excludes:
qhull_internal_repository(name = "qhull_internal", mirrors = mirrors)
if "ros_xacro" not in excludes:
ros_xacro_repository(name = "ros_xacro", mirrors = mirrors)
if "rules_pkg" not in excludes:
Expand Down
46 changes: 46 additions & 0 deletions tools/workspace/deprecation.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# -*- python -*-

def _impl(repo_ctx):
name = repo_ctx.attr.name
date = repo_ctx.attr.date
cc_aliases = repo_ctx.attr.cc_aliases

build = "package(default_visibility = [\"//visibility:public\"])\n"
deprecation = "".join([
"DRAKE DEPRECATED: The @{} external is deprecated".format(name),
" and will be removed from Drake on or after {}.".format(date),
])
for label, actual in cc_aliases.items():
build += "cc_library({})\n".format(", ".join([
"name = " + repr(label),
"deps = [" + repr(actual) + "]",
"deprecation = " + repr(deprecation),
]))

repo_ctx.file("BUILD.bazel", build)

add_deprecation = repository_rule(
doc = """Adds a repository rule with deprecated aliases to other targets.
This is particularly useful when renaming an external repository.
Example:
add_deprecation(
name = "qhull",
date = "2038-01-19",
cc_aliases = {"qhull": "@qhull_internal//:qhull"},
)
""",
attrs = {
"date": attr.string(
doc = "Scheduled removal date of the deprecated target(s).",
mandatory = True,
),
"cc_aliases": attr.string_dict(
doc = """
Optional mapping for cc_library deprecations. The keys are
deprecated target names, the values are the non-deprecated labels.
""",
),
},
implementation = _impl,
)
File renamed without changes.
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_internal/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

Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@

load("@drake//tools/workspace:github.bzl", "github_archive")

def qhull_repository(
def qhull_internal_repository(
name,
mirrors = None):
github_archive(
name = name,
repository = "qhull/qhull",
commit = "2020.2",
sha256 = "59356b229b768e6e2b09a701448bfa222c37b797a84f87f864f97462d8dbc7c5", # noqa
build_file = "@drake//tools/workspace/qhull:package.BUILD.bazel",
build_file = "@drake//tools/workspace/qhull_internal:package.BUILD.bazel", # noqa
patches = [
"@drake//tools/workspace/qhull:patches/disable_dead_code.patch",
"@drake//tools/workspace/qhull_internal:patches/disable_dead_code.patch", # noqa
"@drake//tools/workspace/qhull_internal:patches/vendor_cxx.patch",
],
mirrors = mirrors,
)

0 comments on commit b7c0b58

Please sign in to comment.