From 4cc50ddddc16f3ac3a3fc1430256317dc49ac4e6 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Fri, 27 May 2022 07:49:03 -0700 Subject: [PATCH] [workspace] Use sdformat hidden visibility When building C++ code, we should not use -fvisibility=hidden because it infects all #included code, even the standard library. Instead, we should wrap the third-party C++ code in a inline hidden namespace. That way, only the third-party code itself is hidden. TODO deprecation --- multibody/parsing/detail_common.h | 2 +- multibody/parsing/detail_sdf_diagnostic.h | 2 +- multibody/parsing/detail_sdf_geometry.cc | 14 +- multibody/parsing/detail_sdf_geometry.h | 6 +- multibody/parsing/detail_sdf_parser.cc | 18 +- .../parsing/test/detail_sdf_geometry_test.cc | 4 +- .../parsing/test/detail_sdf_parser_test.cc | 2 +- tools/workspace/BUILD.bazel | 1 + tools/workspace/sdformat/package.BUILD.bazel | 233 ++++++++++-------- tools/workspace/vendor_cxx.py | 5 +- 10 files changed, 164 insertions(+), 123 deletions(-) diff --git a/multibody/parsing/detail_common.h b/multibody/parsing/detail_common.h index a2581266c486..da8616947ef2 100644 --- a/multibody/parsing/detail_common.h +++ b/multibody/parsing/detail_common.h @@ -6,7 +6,7 @@ #include #include -#include +#include #include #include "drake/common/diagnostic_policy.h" diff --git a/multibody/parsing/detail_sdf_diagnostic.h b/multibody/parsing/detail_sdf_diagnostic.h index f121df9a63db..7b8f8176c2fb 100644 --- a/multibody/parsing/detail_sdf_diagnostic.h +++ b/multibody/parsing/detail_sdf_diagnostic.h @@ -3,7 +3,7 @@ #include #include -#include +#include #include "drake/common/diagnostic_policy.h" diff --git a/multibody/parsing/detail_sdf_geometry.cc b/multibody/parsing/detail_sdf_geometry.cc index 4fef6353d4f7..9c56c03aa569 100644 --- a/multibody/parsing/detail_sdf_geometry.cc +++ b/multibody/parsing/detail_sdf_geometry.cc @@ -7,13 +7,13 @@ #include #include -#include -#include -#include -#include -#include -#include -#include +#include +#include +#include +#include +#include +#include +#include #include "drake/geometry/geometry_instance.h" #include "drake/geometry/proximity_properties.h" diff --git a/multibody/parsing/detail_sdf_geometry.h b/multibody/parsing/detail_sdf_geometry.h index 23d39946483e..36495c511e89 100644 --- a/multibody/parsing/detail_sdf_geometry.h +++ b/multibody/parsing/detail_sdf_geometry.h @@ -3,9 +3,9 @@ #include #include -#include -#include -#include +#include +#include +#include #include "drake/common/diagnostic_policy.h" #include "drake/geometry/geometry_instance.h" diff --git a/multibody/parsing/detail_sdf_parser.cc b/multibody/parsing/detail_sdf_parser.cc index f79dc6c3bc44..30ecbb9a00f4 100644 --- a/multibody/parsing/detail_sdf_parser.cc +++ b/multibody/parsing/detail_sdf_parser.cc @@ -10,15 +10,15 @@ #include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include "drake/geometry/geometry_instance.h" #include "drake/math/rigid_transform.h" diff --git a/multibody/parsing/test/detail_sdf_geometry_test.cc b/multibody/parsing/test/detail_sdf_geometry_test.cc index 62998f9dac46..13896469db6c 100644 --- a/multibody/parsing/test/detail_sdf_geometry_test.cc +++ b/multibody/parsing/test/detail_sdf_geometry_test.cc @@ -7,10 +7,10 @@ #include #include "fmt/ostream.h" +#include +#include #include #include -#include -#include #include "drake/common/filesystem.h" #include "drake/common/find_resource.h" diff --git a/multibody/parsing/test/detail_sdf_parser_test.cc b/multibody/parsing/test/detail_sdf_parser_test.cc index f0c5da1e0b2d..ce710a5b2a2d 100644 --- a/multibody/parsing/test/detail_sdf_parser_test.cc +++ b/multibody/parsing/test/detail_sdf_parser_test.cc @@ -5,9 +5,9 @@ #include #include +#include #include #include -#include #include "drake/common/filesystem.h" #include "drake/common/find_resource.h" diff --git a/tools/workspace/BUILD.bazel b/tools/workspace/BUILD.bazel index 4dfdd3954521..2a97892d7ed8 100644 --- a/tools/workspace/BUILD.bazel +++ b/tools/workspace/BUILD.bazel @@ -48,6 +48,7 @@ drake_py_binary( name = "vendor_cxx", srcs = ["vendor_cxx.py"], visibility = [ + "@sdformat//:__pkg__", "@yaml_cpp_internal//:__pkg__", ], deps = [":module_py"], diff --git a/tools/workspace/sdformat/package.BUILD.bazel b/tools/workspace/sdformat/package.BUILD.bazel index 8c9aadc7fd56..0c6f7ce15f4d 100644 --- a/tools/workspace/sdformat/package.BUILD.bazel +++ b/tools/workspace/sdformat/package.BUILD.bazel @@ -16,11 +16,15 @@ load( "@drake//tools/install:install.bzl", "install", ) +load( + "@drake//tools/workspace:vendor_cxx.bzl", + "cc_library_vendored", +) load("@drake//tools/workspace:generate_file.bzl", "generate_file") licenses(["notice"]) # Apache-2.0 AND BSD-3-Clause AND BSL-1.0 -package(default_visibility = ["//visibility:public"]) +package(default_visibility = ["//visibility:private"]) # Generates config.h based on the version numbers in CMake code. cmake_configure_file( @@ -55,7 +59,7 @@ generate_file( ) # Public headers are indicated in sdformat's `include/sdf/CMakeLists.txt`. -SDFORMAT_MOST_PUBLIC_HDRS = [ +_MOST_PUBLIC_HDRS = [ "include/sdf/Actor.hh", "include/sdf/AirPressure.hh", "include/sdf/Altimeter.hh", @@ -139,107 +143,119 @@ py_binary( srcs_version = "PY3", ) -SDFORMAT_ALL_PUBLIC_HDRS = SDFORMAT_MOST_PUBLIC_HDRS + [ +_HDRS = _MOST_PUBLIC_HDRS + [ "include/sdf/config.hh", # from cmake_configure_file above "include/sdf/sdf_config.h", # alias to config.hh "include/sdf/Export.hh", # from generate_file above ] -# Generates the library exported to users. The explicitly listed srcs= matches -# upstream's explicitly listed sources (sdformat/src/CMakeLists.txt), with two -# exceptions: ign.hh and ign.cc are not incorporated in this library, but are -# incorporated into the `ign_sdf_cmdline` library defined below; and the -# parser_urdf.hh and parser_urdf.cc are excluded because we don't use them. -cc_library( +# This list of sources matches upstream's explicitly listed sources +# (sdformat/src/CMakeLists.txt), with two exceptions: ign.hh and ign.cc are not +# incorporated in this library, but are incorporated into the `ign_sdf_cmdline` +# library defined below; and the parser_urdf.hh and parser_urdf.cc are excluded +# because we don't use them. +_SRCS = [ + "src/Actor.cc", + "src/AirPressure.cc", + "src/Altimeter.cc", + "src/Atmosphere.cc", + "src/Box.cc", + "src/Camera.cc", + "src/Capsule.cc", + "src/Collision.cc", + "src/Console.cc", + "src/Converter.cc", + "src/Converter.hh", + "src/Cylinder.cc", + "src/Element.cc", + "src/Ellipsoid.cc", + "src/EmbeddedSdf.hh", + "src/Error.cc", + "src/Exception.cc", + "src/Filesystem.cc", + "src/ForceTorque.cc", + "src/Frame.cc", + "src/FrameSemantics.cc", + "src/FrameSemantics.hh", + "src/Geometry.cc", + "src/Gui.cc", + "src/Heightmap.cc", + "src/Imu.cc", + "src/InterfaceElements.cc", + "src/InterfaceFrame.cc", + "src/InterfaceJoint.cc", + "src/InterfaceLink.cc", + "src/InterfaceModel.cc", + "src/InterfaceModelPoseGraph.cc", + "src/Joint.cc", + "src/JointAxis.cc", + "src/Lidar.cc", + "src/Light.cc", + "src/Link.cc", + "src/Magnetometer.cc", + "src/Material.cc", + "src/Mesh.cc", + "src/Model.cc", + "src/NavSat.cc", + "src/Noise.cc", + "src/OutputConfig.cc", + "src/Param.cc", + "src/ParamPassing.cc", + "src/ParamPassing.hh", + "src/ParserConfig.cc", + "src/ParticleEmitter.cc", + "src/Pbr.cc", + "src/Physics.cc", + "src/Plane.cc", + "src/Plugin.cc", + "src/Polyline.cc", + "src/PrintConfig.cc", + "src/Root.cc", + "src/SDF.cc", + "src/SDFExtension.cc", + "src/SDFExtension.hh", + "src/SDFImplPrivate.hh", + "src/Scene.cc", + "src/ScopedGraph.hh", + "src/SemanticPose.cc", + "src/Sensor.cc", + "src/Sky.cc", + "src/Sphere.cc", + "src/Surface.cc", + "src/Types.cc", + "src/Utils.cc", + "src/Utils.hh", + "src/Visual.cc", + "src/World.cc", + "src/XmlUtils.cc", + "src/XmlUtils.hh", + "src/parser.cc", + "src/parser_private.hh", + ":src/EmbeddedSdf.cc", +] + + +# Generates the library exported to users. +cc_library_vendored( name = "sdformat", - srcs = [ - "src/Actor.cc", - "src/AirPressure.cc", - "src/Altimeter.cc", - "src/Atmosphere.cc", - "src/Box.cc", - "src/Camera.cc", - "src/Capsule.cc", - "src/Collision.cc", - "src/Console.cc", - "src/Converter.cc", - "src/Converter.hh", - "src/Cylinder.cc", - "src/Element.cc", - "src/Ellipsoid.cc", - "src/EmbeddedSdf.hh", - "src/Error.cc", - "src/Exception.cc", - "src/Filesystem.cc", - "src/ForceTorque.cc", - "src/Frame.cc", - "src/FrameSemantics.cc", - "src/FrameSemantics.hh", - "src/Geometry.cc", - "src/Gui.cc", - "src/Heightmap.cc", - "src/Imu.cc", - "src/InterfaceElements.cc", - "src/InterfaceFrame.cc", - "src/InterfaceJoint.cc", - "src/InterfaceLink.cc", - "src/InterfaceModel.cc", - "src/InterfaceModelPoseGraph.cc", - "src/Joint.cc", - "src/JointAxis.cc", - "src/Lidar.cc", - "src/Light.cc", - "src/Link.cc", - "src/Magnetometer.cc", - "src/Material.cc", - "src/Mesh.cc", - "src/Model.cc", - "src/NavSat.cc", - "src/Noise.cc", - "src/OutputConfig.cc", - "src/Param.cc", - "src/ParamPassing.cc", - "src/ParamPassing.hh", - "src/ParserConfig.cc", - "src/ParticleEmitter.cc", - "src/Pbr.cc", - "src/Physics.cc", - "src/Plane.cc", - "src/Plugin.cc", - "src/Polyline.cc", - "src/PrintConfig.cc", - "src/Root.cc", - "src/SDF.cc", - "src/SDFExtension.cc", - "src/SDFExtension.hh", - "src/SDFImplPrivate.hh", - "src/Scene.cc", - "src/ScopedGraph.hh", - "src/SemanticPose.cc", - "src/Sensor.cc", - "src/Sky.cc", - "src/Sphere.cc", - "src/Surface.cc", - "src/Types.cc", - "src/Utils.cc", - "src/Utils.hh", - "src/Visual.cc", - "src/World.cc", - "src/XmlUtils.cc", - "src/XmlUtils.hh", - "src/parser.cc", - "src/parser_private.hh", - ":src/EmbeddedSdf.cc", + srcs = _SRCS, + srcs_vendored = [ + x.replace("src/", "drake_src/src/") + for x in _SRCS ], - hdrs = SDFORMAT_ALL_PUBLIC_HDRS, - # TODO(jamiesnape): Enable visibility after resolving warnings such as - # "Direct access in function means the weak symbol cannot be overridden at - # runtime. This was likely caused by different translation units being - # compiled with different visibility settings." + hdrs = _HDRS, + hdrs_vendored = [ + x.replace("include/sdf/", "drake_src/drake_vendor/sdf/") + for x in _HDRS + ], + edit_include = { + "sdf/": "drake_vendor/sdf/", + }, + includes = ["drake_src"], copts = ["-w"], defines = ["SDFORMAT_STATIC_DEFINE", "SDFORMAT_DISABLE_CONSOLE_LOGFILE"], - includes = ["include"], linkstatic = 1, + visibility = ["//visibility:public"], deps = [ "@ignition_math", "@ignition_utils", @@ -251,14 +267,36 @@ cc_library( ], ) +_IGN_SRCS = [ + "src/ign.cc", + "src/ign.hh", +] + +_VENDORED_IGN_SRCS = [ + x.replace("src/", "drake_src/src/") + for x in _IGN_SRCS +] + +genrule( + name = "vendoring_ign", + srcs = _IGN_SRCS, + outs = _VENDORED_IGN_SRCS, + cmd = " ".join([ + "$(execpath @drake//tools/workspace:vendor_cxx)", + "--edit-include=sdf/:drake_vendor/sdf/", + ] + [ + "$(execpath {}):$(execpath {})".format(old, new) + for old, new in zip(_IGN_SRCS, _VENDORED_IGN_SRCS) + ]), + tools = ["@drake//tools/workspace:vendor_cxx"], +) + cc_library( name = "ign_sdf_cmdline", - srcs = [ - "src/ign.cc", - "src/ign.hh", - ], + srcs = _VENDORED_IGN_SRCS, copts = ["-w"], linkstatic = 1, + visibility = ["@drake//tools/workspace/sdformat:__pkg__"], deps = [ ":sdformat", ], @@ -266,6 +304,7 @@ cc_library( install( name = "install", + visibility = ["//visibility:public"], docs = [ "COPYING", "LICENSE", diff --git a/tools/workspace/vendor_cxx.py b/tools/workspace/vendor_cxx.py index fd2776acc8d1..dba133e8ff9d 100644 --- a/tools/workspace/vendor_cxx.py +++ b/tools/workspace/vendor_cxx.py @@ -12,8 +12,8 @@ def _rewrite_one_text(*, text, edit_include): """Rewrites the C++ file contents in `text` with specific alterations: - The paths in #include statements are replaced per the (old, new) pairs in - the include_edit list. Only includes that use quotation marks will be - changed. The pairs are literal strings that must be a prefix of the path. + the include_edit list. The pairs are literal strings that must be a prefix + of the path. - Wraps an inline namespace "drake_vendor" with hidden symbol visibility around the entire file; it is withdrawn prior to any include statement. @@ -27,6 +27,7 @@ def _rewrite_one_text(*, text, edit_include): # Re-spell the project's own include statements. for old_inc, new_inc in edit_include: text = text.replace(f'#include "{old_inc}', f'#include "{new_inc}') + text = text.replace(f'#include <{old_inc}', f'#include <{new_inc}') # Add an inline namespace around the whole file, but disable it around # include statements.