From fed060d7a7d12bad6d6a9b7feb7c6d136c7654f0 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Wed, 1 Jun 2022 07:59:36 -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. 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. --- geometry/optimization/BUILD.bazel | 2 +- geometry/optimization/vpolytope.cc | 4 +- tools/workspace/BUILD.bazel | 4 +- tools/workspace/default.bzl | 11 ++++- tools/workspace/deprecation.bzl | 44 +++++++++++++++++++ .../{qhull => qhull_internal}/BUILD.bazel | 0 .../package.BUILD.bazel | 30 +++++++++++-- .../patches/disable_dead_code.patch | 0 .../qhull_internal/patches/vendor_cxx.patch | 27 ++++++++++++ .../{qhull => qhull_internal}/repository.bzl | 7 +-- 10 files changed, 117 insertions(+), 12 deletions(-) create mode 100644 tools/workspace/deprecation.bzl rename tools/workspace/{qhull => qhull_internal}/BUILD.bazel (100%) rename tools/workspace/{qhull => qhull_internal}/package.BUILD.bazel (72%) rename tools/workspace/{qhull => qhull_internal}/patches/disable_dead_code.patch (100%) create mode 100644 tools/workspace/qhull_internal/patches/vendor_cxx.patch rename tools/workspace/{qhull => qhull_internal}/repository.bzl (56%) diff --git a/geometry/optimization/BUILD.bazel b/geometry/optimization/BUILD.bazel index 1bacf48a800c..a7698e3f46af 100644 --- 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 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 4dfdd3954521..a98e935e13c7 100644 --- 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 index eee39dbd6340..818e6747e333 100644 --- 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 new file mode 100644 index 000000000000..d0bdf583906f --- /dev/null +++ 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 b/tools/workspace/qhull_internal/BUILD.bazel similarity index 100% rename from tools/workspace/qhull/BUILD.bazel rename to tools/workspace/qhull_internal/BUILD.bazel diff --git a/tools/workspace/qhull/package.BUILD.bazel b/tools/workspace/qhull_internal/package.BUILD.bazel similarity index 72% rename from tools/workspace/qhull/package.BUILD.bazel rename to tools/workspace/qhull_internal/package.BUILD.bazel index ab3fae4a22a9..47ab55ee0352 100644 --- a/tools/workspace/qhull/package.BUILD.bazel +++ b/tools/workspace/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/tools/workspace/qhull/patches/disable_dead_code.patch b/tools/workspace/qhull_internal/patches/disable_dead_code.patch similarity index 100% rename from tools/workspace/qhull/patches/disable_dead_code.patch rename to tools/workspace/qhull_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 new file mode 100644 index 000000000000..34b16016c5c2 --- /dev/null +++ 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 b/tools/workspace/qhull_internal/repository.bzl similarity index 56% rename from tools/workspace/qhull/repository.bzl rename to tools/workspace/qhull_internal/repository.bzl index 5454cda3fd87..8fa92a3786c2 100644 --- a/tools/workspace/qhull/repository.bzl +++ b/tools/workspace/qhull_internal/repository.bzl @@ -2,7 +2,7 @@ load("@drake//tools/workspace:github.bzl", "github_archive") -def qhull_repository( +def qhull_internal_repository( name, mirrors = None): github_archive( @@ -10,9 +10,10 @@ def qhull_repository( 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, )