[workspace] Improve qhull hidden visibility #17229
Conversation
jwnimmer-tri
added
priority: low
release notes: none
jwnimmer-tri
force-pushed
the
vendor-qhull
branch
2 times, most recently
from
3fddbe1
to
9415357
Compare
jwnimmer-tri
force-pushed
the
vendor-qhull
branch
2 times, most recently
from
f947fd1
to
fed060d
Compare
|
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
jwnimmer-tri
added
release notes: newly deprecated
|
+@rpoyner-tri for feature review, please. |
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)
|
+@sammy-tri for platform review per schedule, please. |
jwnimmer-tri
force-pushed
the
vendor-qhull
branch
from
fed060d
to
0da69b0
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
There was a problem hiding this comment.
Reviewed 9 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @jwnimmer-tri)
tools/workspace/deprecation.bzl line 13 at r2 (raw file):
" and will be removed from Drake on or after {}.".format(date), ]) for label, actual in cc_aliases.items():
It doesn't look like "actual" is used here. Does this still create an alias to the library, or just print a deprecation warning?
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.
jwnimmer-tri
force-pushed
the
vendor-qhull
branch
from
0da69b0
to
8314283
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @rpoyner-tri and @sammy-tri)
tools/workspace/deprecation.bzl line 13 at r2 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
It doesn't look like "actual" is used here. Does this still create an alias to the library, or just print a deprecation warning?
Done.
Good find. I'd tested with using @qhull_internal and @qhull both at once (and saw the deprecation) but failed to test just @qhull on its own. Locally I've confirmed that r2 fails that manual test case, and now the change in r3 makes it pass again.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)
When building C++ code, we should not use
-fvisibility=hiddenbecause 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.
Towards #17231.
Requires:
This change is