Skip to content
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

libdrake: Use alwayslink instead of legacy_whole_archive #12262

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Oct 29, 2019

As a side-effect, duplicate definitions (e.g., of main) are now an error.

Closes #12137.

Relates bazelbuild/bazel#7362.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-debug please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review October 29, 2019 16:40
@jwnimmer-tri
Copy link
Collaborator Author

+@jamiesnape for feature review, please.
+@sammy-tri for platform review per schedule, please.
\CC @EricCousineau-TRI FYI

Copy link
Contributor

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri)

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

The macOS debug error is "failed due to unexpected I/O exception: writing file failed". I wonder if that's a flake, or if this change substantially affects the build footprint.

@jamiesnape
Copy link
Contributor

Not one of our usual flakes. Flakes during building rather than testing are rare. I would try a Mojave debug build.

@jamiesnape
Copy link
Contributor

Disk usage on the Linux debug builds looks much higher (potentially up to 50 GB).

@jamiesnape
Copy link
Contributor

Looks like the disk was full on the Mac, so not a flake.

@jwnimmer-tri
Copy link
Collaborator Author

Yup. Sounds like I can profile and debug on Ubunutu for now, though.

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Nov 1, 2019

Testing with bazel build -c dbg //examples/pendulum/... from scratch. The problem seems to be simply that statically-linked programs now have all of the extra unnecessary symbols polluting them, which used to be culled when not referenced:

jwnimmer@call-cps:~/tools/bloaty$ ./bloaty \
  -d compileunits \
  --domain=file \
  ~/jwnimmer-tri/drake.12262/bazel-bin/examples/pendulum/print_symbolic_dynamics \
  -- \
  ~/jwnimmer-tri/drake.master/bazel-bin/examples/pendulum/print_symbolic_dynamics

    FILE SIZE   
 -------------- 
  [NEW] +32.8Mi    math/discrete_algebraic_riccati_equation.cc
  [NEW] +20.3Mi    math/continuous_lyapunov_equation.cc
  +3.5% +14.1Mi    [176 Others]
  [NEW] +11.5Mi    math/continuous_algebraic_riccati_equation.cc
  [NEW] +7.73Mi    math/rigid_transform.cc
  [NEW] +7.25Mi    math/discrete_lyapunov_equation.cc
  [NEW] +7.17Mi    multibody/tree/space_xyz_mobilizer.cc
  [NEW] +6.89Mi    geometry/proximity/distance_to_point_with_gradient.cc
  [NEW] +6.29Mi    math/rotation_matrix.cc
  [NEW] +5.50Mi    geometry/query_results/contact_surface.cc
  [NEW] +5.01Mi    math/roll_pitch_yaw.cc
  [NEW] +4.07Mi    multibody/tree/linear_spring_damper.cc
  [NEW] +2.35Mi    math/quadratic_form.cc
   +22% +1.71Mi    [section .debug_aranges]
  [NEW] +1.62Mi    multibody/tree/revolute_spring.cc
  [NEW] +1.59Mi    systems/framework/single_output_vector_source.cc
  [NEW] +1.57Mi    systems/framework/vector_system.cc
  [NEW] +1.51Mi    multibody/tree/body_node_impl.cc
  -2.4%  -914Ki    multibody/plant/tamsi_solver.cc
 -34.7% -1.09Mi    multibody/tree/weld_joint.cc
 -70.8% -7.98Mi    common/polynomial.cc
   +23%  +129Mi    TOTAL

In my initial draft of the PR, I tried to apply alwayslink = True to the intermediate cc_library targets on the way to libdrake.so, but it seemed to only take effect when places on the targets with srcs = [] directly, so I moved it up to drake_cc_library. This (extra linking) is not a good outcome, though.

I'll have to investigate more.

@jwnimmer-tri
Copy link
Collaborator Author

This is pretty close, but is still failing some installed-image acceptance tests due to losing some DT_NEEDED on the shared library.

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-incompatible-whole-archive branch 2 times, most recently from 6928cba to aef8982 Compare November 4, 2019 17:56
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-debug please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Nov 4, 2019

Ugh. The macOS debug build filled up its disk again:
https://drake-jenkins.csail.mit.edu/job/mac-catalina-clang-bazel-experimental-debug/2/

@jwnimmer-tri
Copy link
Collaborator Author

It looks like bazelbuild/rules_cc@c2b692b might be on the way to fixing this upstream.

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-incompatible-whole-archive branch from f8a3a02 to c4e4673 Compare November 20, 2019 16:22
@jwnimmer-tri jwnimmer-tri changed the title tools: Use alwayslink instead of legacy_whole_archive libdrake: Use alwayslink instead of legacy_whole_archive Nov 20, 2019
@jwnimmer-tri jwnimmer-tri force-pushed the bazel-incompatible-whole-archive branch from c4e4673 to 92fda5c Compare November 20, 2019 17:00
@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Nov 20, 2019

I finally found a solution that I am not unhappy with. It should be exactly the same as before for libdrake build time and disk footprint, and retain the improvement for link size and times for standalone binary programs.

@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-debug please
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

@jwnimmer-tri
Copy link
Collaborator Author

Guess what +@sammy-tri and +@jamiesnape? This is ready for review again!

Copy link
Contributor

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uses less than 100 MB extra space on Mac CI in debug as far as I can tell. :lgtm:

Reviewed 4 of 7 files at r2.
Reviewable status: labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

-@sammy-tri +@ggould-tri for today's on-call platform review.

@jwnimmer-tri jwnimmer-tri assigned ggould-tri and unassigned sammy-tri Nov 21, 2019
Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Well this is pretty painful. Seems correct, though.

Reviewed 4 of 7 files at r2.
Reviewable status: labeled "do not merge"

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-incompatible-whole-archive branch from 92fda5c to 4bab291 Compare November 21, 2019 16:54
@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Nov 21, 2019

I did a retest on Anzu; everything worked fine.

(I also fixed some nits I noticed while reading over this again with fresh eyes.)

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),jamiesnape

@jwnimmer-tri jwnimmer-tri merged commit d0ce5c8 into RobotLocomotion:master Nov 21, 2019
@jwnimmer-tri jwnimmer-tri deleted the bazel-incompatible-whole-archive branch November 21, 2019 18:02
@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop relying on incompatible_legacy_whole_archive
4 participants