Install bot2-lcmgl

[jamiesnape]

Apparently Director needs this.

---

This change is 

[jamiesnape]

+@mwoehlke-kitware for feature review. High priority. FYI @fbudin69500.

[mwoehlke-kitware]

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

tools/libbot.BUILD, line 150 at r1 (raw file):

py_binary(
    name = "bot-log2mat",

BTW, why does this need to be split into a py_library and a py_binary?

---

tools/libbot.BUILD, line 349 at r1 (raw file):

)

# bot2-lcmgl

BTW, given how many sections there are (and how huge this file is), maybe BEGIN/END markers and rules would help readers find their place?

---

tools/libbot.BUILD, line 420 at r1 (raw file):

    name = "bot2_lcmgl_render",
    srcs = [
        "bot2-lcmgl/src/bot_lcmgl_client/lcmgl.h",

BTW, should this be in hdrs?

---

tools/libbot.BUILD, line 527 at r1 (raw file):

install(
    name = "install_bot2_param",
    hdrs = BOT2_PARAM_PUBLIC_HDRS,

BTW, did buildifier do this? Seems like the previous order was better... if this is buildifier's fault, we should add a custom tables (see bazelbuild/buildtools#45); not necessarily now, but at some point...

---

Comments from Reviewable

[jamiesnape]

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

---

tools/libbot.BUILD, line 150 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, why does this need to be split into a py_library and a py_binary?

BTW This is a Bazel approximation of how upstream uses shell scripts.

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, should this be in hdrs?

BTW No see bot2_lcmgl_client above. Libbot does some ugly things, so it cannot be picked up by specifying a dep.

---

tools/libbot.BUILD, line 527 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, did buildifier do this? Seems like the previous order was better... if this is buildifier's fault, we should add a custom tables (see bazelbuild/buildtools#45); not necessarily now, but at some point...

BTW It did.

---

Comments from Reviewable

[jamiesnape]

+@jwnimmer-tri for platform review.

[mwoehlke-kitware]

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

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW No see bot2_lcmgl_client above. Libbot does some ugly things, so it cannot be picked up by specifying a dep.

BTW I'm confused; since bot2_lcmgl_client is already a dep, won't any users of bot2_lcmgl_render pick up this header anyway? (Or is this another "Bazel is horrible" thing?)

---

Comments from Reviewable

[jamiesnape]

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

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW I'm confused; since bot2_lcmgl_client is already a dep, won't any users of bot2_lcmgl_render pick up this header anyway? (Or is this another "Bazel is horrible" thing?)

Libbot thing: #include "../bot_lcmgl_client/lcmgl.h"

---

Comments from Reviewable

[mwoehlke-kitware]

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

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Libbot thing: #include "../bot_lcmgl_client/lcmgl.h"

Maybe I am unclear; I wasn't suggesting to rely on picking it up via a dep, I was asking what is wrong with it being three lines lower, in hdrs of bot2_lcmgl_render instead of srcs of the same. "It's private" doesn't seem to be a reason?

---

Comments from Reviewable

[jamiesnape]

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

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Maybe I am unclear; I wasn't suggesting to rely on picking it up via a dep, I was asking what is wrong with it being three lines lower, in hdrs of bot2_lcmgl_render instead of srcs of the same. "It's private" doesn't seem to be a reason?

To this package it is effectively private. Depends on your philosophy and whether you think the same header should be public for two different packages.

---

Comments from Reviewable

[mwoehlke-kitware]

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

---

tools/libbot.BUILD, line 420 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

To this package it is effectively private. Depends on your philosophy and whether you think the same header should be public for two different packages.

OK

---

Comments from Reviewable

[jwnimmer-tri]

I guess once we've decided that Director needs to use Drake's build of libbot, then this is a necessary outcome. I'm not 100% why we're doing that, but I'll assume there's a good reason and just do a correctness review, rather dig into the appropriateness.

---

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

---

tools/libbot.BUILD, line 401 at r1 (raw file):

    "bot2-lcmgl/src/bot_lcmgl_client/lcmgl.h",
    "bot2-lcmgl/src/bot_lcmgl_render/lcmgl_decode.h",
]

BTW Do we need this variable? It seems to duplicate the hdrs of the two cc_library targets in question, so it seems like guess_hdrs could do the job? Of if Or if guess... is undesirable, perhaps the cc_library rules should refer to (a twain'd version of) this variable?

---

Comments from Reviewable

[jwnimmer-tri]

tools/libbot.BUILD, line 401 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do we need this variable? It seems to duplicate the hdrs of the two cc_library targets in question, so it seems like guess_hdrs could do the job? Of if guess... is undesirable, perhaps the cc_library rules should refer to (a twain'd version of) this variable?

(Oops. The Of if ... should read Or if ...)

---

Comments from Reviewable

[jamiesnape]

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

---

tools/libbot.BUILD, line 401 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(Oops. The Of if ... should read Or if ...)

The LCM types cause the issue. You end up with them being installed twice into two different locations if you use PACKAGE.

---

Comments from Reviewable