-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tools] Link ignition-math statically #15600
[tools] Link ignition-math statically #15600
Conversation
2ebbd15
to
6f5b5ea
Compare
It was an oddball in terms of being a shared library. TBD This is breaking change, in that it's no longer exported for downstream projects to build against.
6f5b5ea
to
ec1f2cf
Compare
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please |
+@rpoyner-tri for feature review, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)
tools/workspace/ignition_math/package.BUILD.bazel, line 237 at r2 (raw file):
}), linkshared = 1, linkstatic = 1,
BTW For my own edification, what is the implication of simultaneously declaring linkshared
and linkstatic
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)
tools/workspace/ignition_math/package.BUILD.bazel, line 237 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW For my own edification, what is the implication of simultaneously declaring
linkshared
andlinkstatic
?
https://docs.bazel.build/versions/4.2.0/be/c-cpp.html#cc_binary.linkshared
"linkshared" means to create a shared library.
"linkstatic" means that the deps should come in as *.a
files (object code copied into the new *.so
), not as references to more *.so
files.
It was an oddball in terms of being a shared library. Most all of our other private dependencies are currently being linked statically. For simplicity, we prefer that the only dependencies linked as shared are either public dependencies (such as spdlog) and/or LGPL'd (such as lcm).
Partially reverts #7622.
Tested locally that:
ls -l bazel-bin/external/ignition_math/libdrake_ignition_math.so
shows a non-empty sizelib/libdrake_ignition_math.so
after abazel run //:install
shows a non-empty size.This change is