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

Update lcm-related bazel library names #8727

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented May 1, 2018

Remove //util:lcm_util forwarding header, long since deprecated.

Rename //lcm:lcm to //lcm:real to clarify that it is only a concrete implementation of the interface, instead of everything in the package. Do not rename in-tree uses to match; they can use the package-level library without adding overly many spurious dependencies.

Rename //systems/lcm:lcm to //systems/lcm:lcm_pubsub to clarify that it is only the publisher and subscriber systems, instead of everything in the package. Rename all in-tree uses to match.

Add drake_cc_package_library for //lcm and //systems/lcm.

Builds on #8664. Relates #6464.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri force-pushed the build-libdrake-lcm branch from 825a798 to ac1152d Compare May 1, 2018 12:05
@jwnimmer-tri
Copy link
Collaborator Author

+@sammy-tri for all review, please.

@sammy-tri
Copy link
Contributor

One minor question + one really minor BTW.


Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


lcm/BUILD.bazel, line 9 at r1 (raw file):

    "drake_cc_package_library",
)
load("//tools/skylark:drake_py.bzl", "drake_py_test")

BTW the spelling of the path to tools/skylark is now inconsistent in this file.


tools/install/libdrake/build_components.bzl, line 110 at r1 (raw file):

    "//geometry:shape_specification",
    "//lcm:interface",
    "//lcm:lcm",

BTW Now that //lcm:lcm is the package library, are the entries below redundant? Or is the convention here to always list out all of the individual components even when the package library is included? (since it happens again below in //systems/lcm it seemed like it might be conventional)


Comments from Reviewable

@sammy-tri
Copy link
Contributor

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


tools/install/libdrake/build_components.bzl, line 110 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW Now that //lcm:lcm is the package library, are the entries below redundant? Or is the convention here to always list out all of the individual components even when the package library is included? (since it happens again below in //systems/lcm it seemed like it might be conventional)

/me facepalm. It's autogenerated. Never mind.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


lcm/BUILD.bazel, line 9 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW the spelling of the path to tools/skylark is now inconsistent in this file.

OK Hmm, I see that's a project-wide problem now. I'll open an issue => #8732.


tools/install/libdrake/build_components.bzl, line 110 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

/me facepalm. It's autogenerated. Never mind.

OK Right! Also the work on #6464 actually makes it so all of the colon-library details here get eviscerated, very soon now.


Comments from Reviewable

Remove //util:lcm_util forwarding header, long since deprecated.

Rename //lcm:lcm to //lcm:real to clarify that it is only a concrete
implementation of the interface, instead of everything in the package.
Do not rename in-tree uses to match; they can use the package-level
library without adding overly many spurious dependencies.

Rename //systems/lcm:lcm to //systems/lcm:lcm_pubsub to clarify that it
is only the publisher and subscriber systems, instead of everything in
the package.  Rename all in-tree uses to match.

Add drake_cc_package_library for //lcm and //systems/lcm.
@jwnimmer-tri jwnimmer-tri force-pushed the build-libdrake-lcm branch from ac1152d to 622d593 Compare May 1, 2018 17:46
@sammy-tri
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@jwnimmer-tri jwnimmer-tri merged commit 8e114df into RobotLocomotion:master May 1, 2018
@jwnimmer-tri jwnimmer-tri deleted the build-libdrake-lcm branch May 1, 2018 18:38
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