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

Remove linkstatic boilerplate. #5157

Merged
merged 2 commits into from
Feb 13, 2017

Conversation

david-german-tri
Copy link
Contributor

@david-german-tri david-german-tri commented Feb 13, 2017

Follows up on #5155.

Fixes #5104.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 16 of 39 files at r1.
Review status: 16 of 39 files reviewed at latest revision, 1 unresolved discussion.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

    lcm_package = "drake",
    lcm_srcs = ["lcmt_body_acceleration.lcm"],
    linkstatic = 1,

Meta the lcm_cc_library does not use drake_cc_library so is missing the new default. I think that's the correct call graph (because we don't want Drake warnings in generated code), but this PR ends up removing linkstatic = 1 on these libraries, which is perhaps not intended?


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 23 of 39 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Review status: 39 of 40 files reviewed at latest revision, 1 unresolved discussion.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta the lcm_cc_library does not use drake_cc_library so is missing the new default. I think that's the correct call graph (because we don't want Drake warnings in generated code), but this PR ends up removing linkstatic = 1 on these libraries, which is perhaps not intended?

Done.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Review status: all files reviewed at latest revision, all discussions resolved.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

Done.

Hmm, should tools/bot_core_lcmtypes.BUILD should drop its linkstatic = 1 now, since its just repeating the default?


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Review status: all files reviewed at latest revision, all discussions resolved.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm, should tools/bot_core_lcmtypes.BUILD should drop its linkstatic = 1 now, since its just repeating the default?

Ditto for tools/robotlocomotion_lcmtypes.BUILD.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ditto for tools/robotlocomotion_lcmtypes.BUILD.

Maybe. That boilerplate is far less contagious, so I'm inclined to leave it for a followup (if at all). I think it's a good thing this PR doesn't touch tools/ at all, since many of the linkstatic in there remain necessary.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Review status: all files reviewed at latest revision, all discussions resolved.


drake/lcmtypes/BUILD, line 12 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

Maybe. That boilerplate is far less contagious, so I'm inclined to leave it for a followup (if at all). I think it's a good thing this PR doesn't touch tools/ at all, since many of the linkstatic in there remain necessary.

Find by me. If we ever change lcm.bzl to choose a different library type, we'll have to correct those two files also, but we'll probably remember and/or catch it in PR review.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants