[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. 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.
RobotLocomotion · Jun 1, 2022 · fed060d · fed060d
1 parent e80d001
commit fed060d
Show file tree

Hide file tree

Showing 10 changed files with 117 additions and 12 deletions.
diff --git a/geometry/optimization/BUILD.bazel b/geometry/optimization/BUILD.bazel
@@ -55,7 +55,7 @@ drake_cc_library(
         "//solvers:mosek_solver",
         "//solvers:scs_solver",
         "//solvers:solve",
-        "@qhull",
+        "@qhull_internal//:qhull",
     ],
 )
 

diff --git a/geometry/optimization/vpolytope.cc b/geometry/optimization/vpolytope.cc
@@ -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"

diff --git a/tools/workspace/BUILD.bazel b/tools/workspace/BUILD.bazel
@@ -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"],
@@ -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",

diff --git a/tools/workspace/default.bzl b/tools/workspace/default.bzl
@@ -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
@@ -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
@@ -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:

diff --git a/tools/workspace/deprecation.bzl b/tools/workspace/deprecation.bzl
@@ -0,0 +1,44 @@
+# -*- 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),
+            "deprecation = " + repr(deprecation),
+        ]))
+
+    repo_ctx.file("BUILD.bazel", build)
+
+add_deprecation = repository_rule(
+    doc = """Adds a repository rule with deprecated aliases to other targets.
+
+    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,
+)
diff --git a/tools/workspace/qhull/BUILD.bazel → tools/workspace/qhull_internal/BUILD.bazel b/tools/workspace/qhull/BUILD.bazel → tools/workspace/qhull_internal/BUILD.bazel
diff --git a/tools/workspace/qhull/package.BUILD.bazel → ...kspace/qhull_internal/package.BUILD.bazel b/tools/workspace/qhull/package.BUILD.bazel → ...kspace/qhull_internal/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/...ace/qhull/patches/disable_dead_code.patch → ..._internal/patches/disable_dead_code.patch b/...ace/qhull/patches/disable_dead_code.patch → ..._internal/patches/disable_dead_code.patch
diff --git a/tools/workspace/qhull_internal/patches/vendor_cxx.patch b/tools/workspace/qhull_internal/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
++++ 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
+
diff --git a/tools/workspace/qhull/repository.bzl → ...s/workspace/qhull_internal/repository.bzl b/tools/workspace/qhull/repository.bzl → ...s/workspace/qhull_internal/repository.bzl
@@ -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,
     )