[workspace] Use sdformat hidden visibility #17230
Conversation
jwnimmer-tri
added
priority: low
release notes: none
jwnimmer-tri
force-pushed
the
vendoring
branch
2 times, most recently
from
08fbb82
to
803f7c4
Compare
jwnimmer-tri
force-pushed
the
vendoring
branch
2 times, most recently
from
45b8e69
to
017d25b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
|
+@rpoyner-tri for feature review, please. |
There was a problem hiding this comment.
Reviewed 2 of 10 files at r1, 5 of 13 files at r2, 12 of 15 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)
|
+@sherm1 for platform review per schedule (tomorrow), please. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
There was a problem hiding this comment.
Platform reviewed as best I can with limited understanding of the build system. I think I get the idea though and it looks good.
Reviewed 2 of 10 files at r1, 5 of 13 files at r2, 4 of 15 files at r3, 5 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions (waiting on @jwnimmer-tri)
tools/workspace/gz_math_internal/package.BUILD.bazel line 42 at r5 (raw file):
defines = [ "IGN_DESIGNATION=math", "PROJECT_VERSION_MAJOR=0",
BTW I gather the version numbers don't matter anymore but it isn't clear why. Consider adding a comment to explain why (a) they don't matter, and (b) why we have to define them at all.
tools/workspace/gz_math_internal/package.BUILD.bazel line 43 at r5 (raw file):
"IGN_DESIGNATION=math", "PROJECT_VERSION_MAJOR=0", "PROJECT_VERSION_MINOR=%0",
nit: %0 -> 0
tools/workspace/gz_utils_internal/package.BUILD.bazel line 36 at r5 (raw file):
"PROJECT_VERSION_PATCH=0", "PROJECT_VERSION=0.0", "PROJECT_VERSION_FULL=0.0.0",
BTW consider same comment here -- why all these zeros?
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. Remove the manually-curated version numbers from ignition builds. We don't use them for anything, so keeping them in sync is wasted effort.
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform) (waiting on @rpoyner-tri and @sherm1)
tools/workspace/gz_math_internal/package.BUILD.bazel line 42 at r5 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW I gather the version numbers don't matter anymore but it isn't clear why. Consider adding a comment to explain why (a) they don't matter, and (b) why we have to define them at all.
Using zeros here is actually the typical convention (see below). Basically, no C++ code does anything with these definitions, so their value doesn't matter and it's easier to perform upgrades when we don't need to keep this information in sync. The fact that these used to have real version numbers was vestigial from when we shipped this code as a shared library.
tools/workspace/fcl/package.BUILD.bazel:61: "OCTOMAP_MAJOR_VERSION=0",
tools/workspace/fcl/package.BUILD.bazel:62: "OCTOMAP_MINOR_VERSION=0",
tools/workspace/fcl/package.BUILD.bazel:63: "OCTOMAP_PATCH_VERSION=0",
tools/workspace/libcmaes/package.BUILD.bazel:34: "LIBCMAES_VERSION_STRING=0.0.0",
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status:
When building C++ code, we can't use
-fvisibility=hiddenbecause 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.Towards #17231.
Requires:
This change is