kuka_plan_runner can find resources when installed

[fbudin69500]

kuka_plan_runner adds its own path to AddResourceSearchPath. Before this patch,
FindResource() was only using the environment variable or the current working
directory to find resources. No additional resource search path was set.

This is the easy solution and would have to be implemented in each executable. Another solution would be to add the current executable path in the resource search path as part of the process to find resources (in find_resource.cc). There is no cross-platform way of doing so without using argv[0]but one could use this solution which implements a solution for Linux and MacOS. If Windows support was added to drake, one could use GetModuleFileName.

---

This change is 

[jwnimmer-tri]

I'm unclear on the motivation for this PR.

I see that kuka_plan_runner is the only installed example, which is a fine reason for only fixing this in one place. However, this patch is a band-aid at best; copying this patch to other example binaries would be defective.

Is the problem of kuka_plan_runner needing an env var to work right now acutely painful, such that we want a band-aid PR merged quickly? If so, this PR would be fine with an appropriate TODO disclaimer near the changed lines, and an issue opened to track undoing it.

If its not an immediate pain point, then it seems like an infrastructural fixup would be less overall hassle, since we have to do that anyway.

---

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

---

Comments from Reviewable

[fbudin69500]

The motivation of this PR is to get other drake developers opinion on what is the best way to talk this issue. As far as I know, there is no urgency that would require to merge this PR before implementing a more global solution if that global solution was the solution chosen by the drake developers

Right now, if one runs kuka_plan_runner from the install directory, they will see the following error message:
terminate called after throwing an instance of 'std::runtime_error' what(): could not find resource [1] 25210 abort (core dumped) /tmp/drake_install/bin/kuka_plan_runner
This does not mention the possibility to set an environment variable, so it is not very helpful from a user that is not that familiar with drake and its resources.

The environment variable is ok, but it is always easier to have software that just works. The same idea was applied to the libraries: If dependent libraries are not found at runtime, one can change LD_LIBRARY_PATH. But it is much easier to have the libraries found automatically.

The general solution suggested in the PR message is a better solution, but I didn't want to implement it before getting other people's opinion. Is this what you called 'infrastructural fixup` or did you have something else in mind?

---

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

---

Comments from Reviewable

[jwnimmer-tri]

That helps a lot, thank you.

By infrastructure, I just mean fixing it in one place. Probably that is some change to common/find_resource.cc, though I could also imagine install-time solutions that make the installed image more forgiving. I will give some thought to options and report back with ideas for discussion.

---

Comments from Reviewable

[jwnimmer-tri]

Right now, if one runs kuka_plan_runner from the install directory, they will see the following error message ... what(): could not find resource ... This does not mention the possibility to set an environment variable, so it is not very helpful from a user that is not that familiar with drake and its resources.

Good point. The runtime_error string provided in get_absolute_path_or_throw should be improved, no matter what else happens with resource pathing. That can be a separate PR, and might as well happen immediately?

[jwnimmer-tri]

First, I'll note that installing examples into bin is wrong. They should go into libexec with their full package path intact. The kuka_plan_runner is in the wrong place, for example. (I noticed this while researching here.) Edit: I've moved the bin discussion over to #7442.

For making binaries Just Work, I see two possible options.

(1) Use the binary's pathname (via our implementation of resolveBinaryLocation ala 22675457) to add a resource search path. It would only take effect if we found an exe path with libexec/drake in it such as libexec/drake/drake/examples/kuka_iiwa_arm/kuka_plan_runner (with #6996 coming, soon to lose the double drake/drake probably). When it found libexec/drake, it could reason out the share path that should be searched.

(2) Use the binary's dlopen(NULL, ...) link map to find where libdrake.so is coming from (ala 5103443), and then convert the lib => share path from there, when we're in the install tree. This has the advantage of working not only for Drake's own C++ executables, but also for other folks' C++ executables, and probably lets us remove pydrake's special-casing for search paths. If this works easily on Ubuntu and macOS, it is probably the best choice.

[fbudin69500]

I tested 2) and on linux it is fairly easy to list the libraries that are loaded. However, that made me realize that the examples do not link against libdrake.so, but directly against "//drake/common:find_resource", so this method will not work in this case.
Also, trying to work around this problem by linking the examples to libdrake.so doesn't seem to be an option: libdrake.so is built with the command cc_binary() instead of cc_library() and therefore is not accepted as a dependency when building the examples.
So I think we may end up implementing both solutions, one for the examples, and one for the targets linking against libdrake.so.

[fbudin69500]

I updated the bazel build such that the examples that we are installing depend in libdrake.so. I then implemented a platform specific detection of libdrake.so path both for MacOS and Linux. This works. There are still a few things to polish before this PR is ready (some tests seem to be failing), but it should be fairly close to being good (from my point of view).

[jwnimmer-tri]

I marked a few suggested spots for cleanup.

I would like to get @jamiesnape's feedback on the general approach.

I wonder if using a smaller shared library would make more sense. So like in //drake/common have libdrake_marker.so internally linked, have find_resource.cc merely look for that, and don't pivot the examples to use monolithic libdrake.

---

Reviewed 1 of 6 files at r2.
Review status: 1 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

drake/bindings/python/pydrake/init.py, line 13 at r2 (raw file):

from .pydrake_path import getDrakePath

# Adding searchable path as inferred by pydrake. This assumes that the python

This hunk (here on down) seems like it should also be removed, since the AddRes... call that uses it is gone now.

---

drake/common/find_resource.cc, line 294 at r2 (raw file):

  return binary_dirname;
}
#endif

Maybe the "where is libdrake.so" code could move into a second file, to help declutter this one, and to isolate platform-specific code into a single exclusive location from platform-independent code? (The "../share/drake" addition could still live within this file.)

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 90 at r2 (raw file):

        "@com_github_gflags_gflags//:gflags",
    ],
)

The "create an example that links to libdrake.so" mechanics will have to be abstracted into a helper macro. Repeating it several times is too brittle.

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 111 at r2 (raw file):

        ":oracular_state_estimator",
        "//drake/examples/kuka_iiwa_arm/iiwa_world:iiwa_wsg_diagram_factory",
        "//drake/lcmtypes:schunk",  # lcm header files

Probably this (within the helper macro) should just depend on all LCM headers, so either //drake/lcmtypes:drake_lcmtypes_headers or a different similar label if that one doesn't work. Trying to slice it finely seems at odds with using monolithic libdrake.so.

---

drake/examples/kuka_iiwa_arm/kuka_plan_runner.cc, line 187 at r2 (raw file):

int main(int argc, char* argv[]) {

BTW This change (adding two unused arguments) can probably be reverted now?

---

Comments from Reviewable

[jamiesnape]

I think this is fine and I probably would keep libdrake.so monolithic for now. If we were modularizing for other reasons (which I would be very much in favor of), I would split this functionality off into a separate library.

---

Review status: 1 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jwnimmer-tri]

OK

---

Review status: 1 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[fbudin69500]

Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

drake/common/find_resource.cc, line 294 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe the "where is libdrake.so" code could move into a second file, to help declutter this one, and to isolate platform-specific code into a single exclusive location from platform-independent code? (The "../share/drake" addition could still live within this file.)

Done.

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 90 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The "create an example that links to libdrake.so" mechanics will have to be abstracted into a helper macro. Repeating it several times is too brittle.

Done.

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 111 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably this (within the helper macro) should just depend on all LCM headers, so either //drake/lcmtypes:drake_lcmtypes_headers or a different similar label if that one doesn't work. Trying to slice it finely seems at odds with using monolithic libdrake.so.

Done.

---

drake/bindings/python/pydrake/init.py, line 13 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This hunk (here on down) seems like it should also be removed, since the AddRes... call that uses it is gone now.

Done.

---

drake/examples/kuka_iiwa_arm/kuka_plan_runner.cc, line 187 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This change (adding two unused arguments) can probably be reverted now?

Done.

---

Comments from Reviewable

[jwnimmer-tri]

FYI I'm okay anytime you want to rebase this, even if it resets reviewable's discussions.

---

Reviewed 8 of 12 files at r3.
Review status: 6 of 10 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

drake/common/find_resource.cc, line 17 at r3 (raw file):

#include <link.h>
#endif

Are these includes needed here anymore?

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 133 at r3 (raw file):

drake_cc_binary(
    name = "kuka_plan_runner",
    srcs = ["kuka_plan_runner.cc"] + ["//tools/install/libdrake:libdrake.so"],

Unclear why this doesn't use drake_example_cc_binary.

---

tools/skylark/drake_cc.bzl, line 515 at r3 (raw file):

        test_rule_flaky = 0,
        **kwargs):
    """Creates a rule to declare a C++ binary.

Needs more documentation - both about what this does, and advice on why or where it should be used.

---

Comments from Reviewable

[fbudin69500]

Review status: 1 of 8 files reviewed at latest revision, 3 unresolved discussions.

---

tools/skylark/drake_cc.bzl, line 515 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Needs more documentation - both about what this does, and advice on why or where it should be used.

Done.

---

drake/common/find_resource.cc, line 17 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Are these includes needed here anymore?

Done.

---

drake/examples/kuka_iiwa_arm/BUILD.bazel, line 133 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear why this doesn't use drake_example_cc_binary.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 11 of 15 files at r4.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

common/find_loaded_library.cc, line 87 at r4 (raw file):

// This code has been adapted from:
// http://syprog.blogspot.ru/2011/12/listing-loaded-shared-objects-in-linux.html

FYI The names and mnemonics in https://stackoverflow.com/a/27522149 were more useful for me (and a bit more C++-ish) to follow along with.

---

common/find_loaded_library.cc, line 89 at r4 (raw file):

// Chained list of shared objects
struct lmap {

Private structure declarations should appear in the anonymous namespace.

Ditto below

---

common/find_loaded_library.cc, line 90 at r4 (raw file):

// Chained list of shared objects
struct lmap {
  void*    base_address;     // Base address of the shared object

BTW Extra horizontal whitespace

---

common/find_loaded_library.cc, line 93 at r4 (raw file):

  char*    path;             // Absolute file name (path) of the shared object
  void*    not_needed;       // Pointer to the dynamic section of the SO
  struct lmap *next, *prev;  // chain of loaded objects

Inconsistent capitalization Chain.

---

common/find_loaded_library.cc, line 98 at r4 (raw file):

// Content of that dlopen is saved in this structure.
struct something {
  void*  pointers[3];

BTW Extra horizontal whitespace

---

common/find_loaded_library.h, line 4 at r4 (raw file):

#include <string>
#include <vector>

BTW unused?

---

common/find_loaded_library.h, line 11 at r4 (raw file):

optional<std::string> loaded_library_path(const std::string &library_name);

Should be CamelCase http://drake.mit.edu/styleguide/cppguide.html#Function_Names

Also the & should be next to string not library_name.

---

common/find_loaded_library.h, line 11 at r4 (raw file):

optional<std::string> loaded_library_path(const std::string &library_name);

Missing API documentation on public method.

---

common/find_resource.cc, line 231 at r4 (raw file):

  // (3) Find where `librake.so` is, and add search path that corresponds to
  // resource folder in install tree based on `libdrake.so` location.
  static optional<string> from_libdrake = resource_path_from_libdrake();

http://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables

The easy fix is:

static const never_destroyed<optional<string>> from_libdrake =
    resource_path_from_libdrake();

---

common/find_resource.cc, line 232 at r4 (raw file):

  // resource folder in install tree based on `libdrake.so` location.
  static optional<string> from_libdrake = resource_path_from_libdrake();
  candidate_dirs.emplace_back(from_libdrake);

Case (3) probably needs a regression test of some kind? (Or did I overlook it?)

---

common/find_resource.cc, line 233 at r4 (raw file):

  static optional<string> from_libdrake = resource_path_from_libdrake();
  candidate_dirs.emplace_back(from_libdrake);
  // (4) Search in cwd (and its parent, grandparent, etc.) to find Drake's

Missing blank line between numbered list items (for consistency with other paragraphs above).

---

tools/skylark/drake_cc.bzl, line 530 at r4 (raw file):

        test_rule_data = [],
        test_rule_size = None,
        test_rule_flaky = 0,

BTW Is there a reason to repeat args that are passed through unchanged to drake_cc_binary? The kwargs could handle them if we wanted to.

---

tools/skylark/drake_cc.bzl, line 540 at r4 (raw file):

    which let the process discover the location of drake resources at runtime
    based on the location of `libdrake.so` which is loaded by the process.
    """

It seems to me like this macro should fail-fast if deps contains a label //drake/something that isn't under //drake/examples/something. That seems like it would help developers apply this macro correctly -- where you have to remove some deps as this PR does when we choose to use this macro.

Probably should also fail-fast if native.package_name() does not startswith examples.

---

Comments from Reviewable

[jwnimmer-tri]

+@soonho-tri for feature review of the APPLE portion (first half) of find_loaded_library.cc.

+@sammy-tri for feature review of the proposal to use drake_cc_example_binary for the installed Kuka examples.

---

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

---

Comments from Reviewable

[soonho-tri]

for feature review of the APPLE portion (first half) of find_loaded_library.cc.

Done. PTAL.

---

Reviewed 4 of 15 files at r4.
Review status: all files reviewed at latest revision, 22 unresolved discussions.

---

common/find_loaded_library.cc, line 15 at r4 (raw file):

#endif

using std::string;

It seems that we need #include <string> unconditionally. BTW what is <string.h> that we include only for non-apple environment?

---

common/find_loaded_library.cc, line 28 at r4 (raw file):

                                  mach_msg_type_number_t* size) {
  mach_msg_type_number_t  dataCnt =
    reinterpret_cast<mach_msg_type_number_t>(*size);

It's not clear why we need reinterpret_cast here. size is already of mach_msg_type_number_t.

---

common/find_loaded_library.cc, line 33 at r4 (raw file):

  kern_return_t kr = vm_read(mach_task_self(), addr, *size,
                             &readMem, &dataCnt);
  if (kr) {

BTW, better to be verbose here by having if (kr != KERN_SUCCESS)?

---

common/find_loaded_library.cc, line 36 at r4 (raw file):

    return NULL;
  }
  return ( reinterpret_cast<unsigned char *>(readMem));

BTW, unnecessary space between return ( and reinterpret_cast.

---

common/find_loaded_library.cc, line 43 at r4 (raw file):

// with relative directory to find resource files in drake install tree.
// This function is specific to MacOS
optional<string> loaded_library_path(const std::string &library_name) {

BTW, const std::string & -> const string&. Please check other occurrences.

---

common/find_loaded_library.cc, line 45 at r4 (raw file):

optional<string> loaded_library_path(const std::string &library_name) {
  optional<string> binary_dirname;
  struct task_dyld_info dyld_info;

I think the use of struct in this function body can be safely omitted.

---

common/find_loaded_library.cc, line 63 at r4 (raw file):

    mach_msg_type_number_t size2 =
      sizeof(struct dyld_image_info) * infos->infoArrayCount;
    uint8_t* info_addr = readProcessMemory(

BTW, is it better to have unsigned char* info_addr to match the return type of readProcessMemory?

---

common/find_loaded_library.cc, line 74 at r4 (raw file):

    for (uint32_t i=0; i < infos->infoArrayCount; i++) {
      const char * pos_slash = strrchr(info[i].imageFilePath, '/');
      if (!strcmp(pos_slash + 1, library_name.c_str())) {

Can we use strncmp instead of strcmp?

---

common/find_loaded_library.cc, line 76 at r4 (raw file):

      if (!strcmp(pos_slash + 1, library_name.c_str())) {
        binary_dirname = string(info[i].imageFilePath,
          pos_slash - info[i].imageFilePath);

How about we have explicit return statement,return string(...), here and change the other occurrences of return binary_dirname; with return nullopt;?

---

Comments from Reviewable

[soonho-tri]

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

---

common/find_loaded_library.cc, line 15 at r4 (raw file):

It seems that we need #include unconditionally

Never mind. I found it in .h file.

---

Comments from Reviewable

[sammy-tri]

Reviewed 1 of 14 files at r3, 9 of 15 files at r4.
Review status: all files reviewed at latest revision, 24 unresolved discussions.

---

common/find_loaded_library.cc, line 88 at r4 (raw file):

// http://syprog.blogspot.ru/2011/12/listing-loaded-shared-objects-in-linux.html

// Chained list of shared objects

Is there a compelling reason to define struct lmap again here instead of using struct link_map from link.h (which we're already including)? We should perhaps consider explaining why we're redefining it.

---

common/find_loaded_library.cc, line 109 at r4 (raw file):

  optional<string> binary_dirname;
  struct lmap* pl;
  void* ph = dlopen(NULL, RTLD_NOW);

Could we avoid defining struct something by phrasing this code as dlinfo(ph, RTLD_DI_LINKMAP, &pl);? (I didn't try it, but the man page for dlinfo implies this is the supported way to get the link map).

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

        "//drake/lcm",
        "//drake/systems/lcm",
        "//drake/systems/lcm:lcm_driven_loop",

These are all real dependencies of this code. I'm not sure it's a good policy to remove deps because they happen to be captured by a transitive dependency (which could be brittle if the something downstream changes its dependencies)? (and META throughout this file).

---

Comments from Reviewable

[jwnimmer-tri]

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

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

These are all real dependencies of this code. I'm not sure it's a good policy to remove deps because they happen to be captured by a transitive dependency (which could be brittle if the something downstream changes its dependencies)? (and META throughout this file).

This is actually the point of this PR, and what I wanted your eyes on.

The upshot is when using :drake_shared_library, we get libdrake dynamically in the drake_example_cc_binary targets, which means that the library path can be used to find data resources without the user having to configure any environment variables.

One consequence of that is that building these examples now unconditionally depends on all of libdrake, rather than just the fraction that is used at runtime.

Another consequence is that we can't list things twice here; doing that would violate ODR. The premise of this PR is that examples link to libdrake.so always -- and libdrake is guaranteed to have every part of Drake that is non-testonly, non-examples.

In writing this out now, I realize that dev code isn't generally part of libdrake.so, but might conceivably be used by an example program, so would still have to be listed here. That might be a maintenance burden.

Another alternative might be to have an used-by-install-rules-only target, that links the example dynamically; it could programmatically strip out the deps, so that the list here would remain unchanged (and used to narrow the dependencies at development-time).

---

Comments from Reviewable

[sammy-tri]

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

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is actually the point of this PR, and what I wanted your eyes on.

The upshot is when using :drake_shared_library, we get libdrake dynamically in the drake_example_cc_binary targets, which means that the library path can be used to find data resources without the user having to configure any environment variables.

One consequence of that is that building these examples now unconditionally depends on all of libdrake, rather than just the fraction that is used at runtime.

Another consequence is that we can't list things twice here; doing that would violate ODR. The premise of this PR is that examples link to libdrake.so always -- and libdrake is guaranteed to have every part of Drake that is non-testonly, non-examples.

In writing this out now, I realize that dev code isn't generally part of libdrake.so, but might conceivably be used by an example program, so would still have to be listed here. That might be a maintenance burden.

Another alternative might be to have an used-by-install-rules-only target, that links the example dynamically; it could programmatically strip out the deps, so that the list here would remain unchanged (and used to narrow the dependencies at development-time).

Ok, thanks, hopefully I understand a bit better now.

Building these examples (and the .so for that matter!) every time anything anywhere in drake changes might get annoying, but for someone actively developing another part of drake the answer is probably to use a more specific target for bazel build or bazel test which doesn't include examples/kuka_iiwa_arm. An used-by-install-rules-only target would fix this issue, the question is if the bulld behavior is going to be sufficiently obnoxious to make that mandatory for this PR.

For the dev/ question -- I don't really think we should be installing stuff from dev directories (or depending on stuff in dev directories from the non-dev part of the tree), so I'm not too worried about that not working.

---

Comments from Reviewable

[jwnimmer-tri]

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Ok, thanks, hopefully I understand a bit better now.

Building these examples (and the .so for that matter!) every time anything anywhere in drake changes might get annoying, but for someone actively developing another part of drake the answer is probably to use a more specific target for bazel build or bazel test which doesn't include examples/kuka_iiwa_arm. An used-by-install-rules-only target would fix this issue, the question is if the bulld behavior is going to be sufficiently obnoxious to make that mandatory for this PR.

For the dev/ question -- I don't really think we should be installing stuff from dev directories (or depending on stuff in dev directories from the non-dev part of the tree), so I'm not too worried about that not working.

For context: the thought process is also that (1) approximately all examples should be installed, and (2) all installed examples will switch to use drake_example_cc_binary and thus have the "libdrake.so dependency" consequences.

---

Comments from Reviewable

[jwnimmer-tri]

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

---

tools/skylark/drake_cc.bzl, line 540 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems to me like this macro should fail-fast if deps contains a label //drake/something that isn't under //drake/examples/something. That seems like it would help developers apply this macro correctly -- where you have to remove some deps as this PR does when we choose to use this macro.

Probably should also fail-fast if native.package_name() does not startswith examples.

Caveat: code in a /dev/ package is not installed, so could realistically still be included in deps. Probably its okay to still fail in that case, though really examples with dev code in them should not be installed anyway. So I think that caveat turns into comments / documentation here, not code.

---

Comments from Reviewable

[fbudin69500]

Review status: 2 of 10 files reviewed at latest revision, 26 unresolved discussions.

---

common/find_loaded_library.cc, line 15 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

It seems that we need #include unconditionally

Never mind. I found it in .h file.

<string> is included in find_loaded_library.h
<string.h> is to include functions to work with c-strings, such as strcmp.

---

common/find_loaded_library.cc, line 28 at r4 (raw file):

mach_msg_type_number_t dataCnt =

---

common/find_loaded_library.cc, line 28 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

It's not clear why we need reinterpret_cast here. size is already of mach_msg_type_number_t.

Done.

---

common/find_loaded_library.cc, line 36 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, unnecessary space between return ( and reinterpret_cast.

Done.

---

common/find_loaded_library.cc, line 43 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, const std::string & -> const string&. Please check other occurrences.

Done.

---

common/find_loaded_library.cc, line 45 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I think the use of struct in this function body can be safely omitted.

Done.

---

common/find_loaded_library.cc, line 63 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, is it better to have unsigned char* info_addr to match the return type of readProcessMemory?

Done.

---

common/find_loaded_library.cc, line 74 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Can we use strncmp instead of strcmp?

I don't see the benefits of using strncmp over strcmp since we compare the end of the string, and therefore its size does not matter in this case. Did you think about a usecase in which it would be useful?

---

common/find_loaded_library.cc, line 76 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

How about we have explicit return statement,return string(...), here and change the other occurrences of return binary_dirname; with return nullopt;?

Done.

---

common/find_loaded_library.cc, line 87 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI The names and mnemonics in https://stackoverflow.com/a/27522149 were more useful for me (and a bit more C++-ish) to follow along with.

I removed most of it.

---

common/find_loaded_library.cc, line 88 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is there a compelling reason to define struct lmap again here instead of using struct link_map from link.h (which we're already including)? We should perhaps consider explaining why we're redefining it.

I removed it.

---

common/find_loaded_library.cc, line 89 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Private structure declarations should appear in the anonymous namespace.

Ditto below

I removed it.

---

common/find_loaded_library.cc, line 90 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Extra horizontal whitespace

Done.

---

common/find_loaded_library.cc, line 93 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Inconsistent capitalization Chain.

Removed.

---

common/find_loaded_library.cc, line 98 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Extra horizontal whitespace

Done.

---

common/find_loaded_library.cc, line 109 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Could we avoid defining struct something by phrasing this code as dlinfo(ph, RTLD_DI_LINKMAP, &pl);? (I didn't try it, but the man page for dlinfo implies this is the supported way to get the link map).

Done.

---

common/find_loaded_library.h, line 4 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW unused?

Done.

---

common/find_loaded_library.h, line 11 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing API documentation on public method.

Done.

---

common/find_loaded_library.h, line 11 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Should be CamelCase http://drake.mit.edu/styleguide/cppguide.html#Function_Names

Also the & should be next to string not library_name.

Done.

---

common/find_resource.cc, line 231 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

http://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables

The easy fix is:

static const never_destroyed<optional<string>> from_libdrake =
    resource_path_from_libdrake();

It seems easier to simply remove the static keyword. Since this function is not called very often, the overhead of calling this extra bit of code is very limited (if one even calls this function multiple times).

---

common/find_resource.cc, line 232 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Case (3) probably needs a regression test of some kind? (Or did I overlook it?)

The Python test common_install_test calls this section of the code now that it does not manually call AddResourceSearchPath(). A test has been added for find_loaded_library.

---

common/find_resource.cc, line 233 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing blank line between numbered list items (for consistency with other paragraphs above).

Done.

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For context: the thought process is also that (1) approximately all examples should be installed, and (2) all installed examples will switch to use drake_example_cc_binary and thus have the "libdrake.so dependency" consequences.

What is the conclusion of this discussion? For now I haven't updated this PR to take into account what is said here.

---

tools/skylark/drake_cc.bzl, line 530 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Is there a reason to repeat args that are passed through unchanged to drake_cc_binary? The kwargs could handle them if we wanted to.

Done.

---

tools/skylark/drake_cc.bzl, line 540 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Caveat: code in a /dev/ package is not installed, so could realistically still be included in deps. Probably its okay to still fail in that case, though really examples with dev code in them should not be installed anyway. So I think that caveat turns into comments / documentation here, not code.

I added a fail-fast if this macro is not used in examples, but I am not sure I understand the first comment about the dependencies. If I look at iiwa_controller which uses this new macro, it depends on @com_github_gflags_gflags//:gflags which is not in examples.

---

Comments from Reviewable

[fbudin69500]

Review status: 2 of 10 files reviewed at latest revision, 18 unresolved discussions.

---

common/find_loaded_library.cc, line 33 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, better to be verbose here by having if (kr != KERN_SUCCESS)?

Done.

---

Comments from Reviewable

[sammy-tri]

pending open issues

---

Reviewed 2 of 15 files at r4, 8 of 8 files at r5.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

What is the conclusion of this discussion? For now I haven't updated this PR to take into account what is said here.

Given Jeremy's comment that "almost all examples will be installed" I think that we'll eventually need the used-by-install-rules-only target to avoid constantly rebuilding examples which don't care about the actual change. For now, I'm OK landing this as is (and seeing how much, if any, significant pain results in terms of compile times).

---

Comments from Reviewable

[fbudin69500]

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

---

examples/kuka_iiwa_arm/BUILD.bazel, line 95 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Given Jeremy's comment that "almost all examples will be installed" I think that we'll eventually need the used-by-install-rules-only target to avoid constantly rebuilding examples which don't care about the actual change. For now, I'm OK landing this as is (and seeing how much, if any, significant pain results in terms of compile times).

Since the examples are linked against a shared library libdrake.so, I wouldn't expect things to be much slower, but I guess we'll see.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 15 files at r4, 7 of 8 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

tools/skylark/drake_cc.bzl, line 540 at r4 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

I added a fail-fast if this macro is not used in examples, but I am not sure I understand the first comment about the dependencies. If I look at iiwa_controller which uses this new macro, it depends on @com_github_gflags_gflags//:gflags which is not in examples.

To clarify: the following would be a bug in code that uses this macro:

drake_example_cc_binary(
  ...
  deps = [
    "//solvers:solver_id",
  ]
  ...
)

This would re-link the solver_id.o into the binary, leading to an ODR violation versus the copy of solver_id.o in libdrake.so. Everything in deps for this macro should either start with@ or : or //examples, I think? Might be worth asserting that here? (Even some things in @ should not be re-linked here, but I guess there's only so much we can do. Manually wrangling ODR is just plain hard.)

---

Comments from Reviewable

[fbudin69500]

Review status: 9 of 10 files reviewed at latest revision, 2 unresolved discussions.

---

tools/skylark/drake_cc.bzl, line 540 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To clarify: the following would be a bug in code that uses this macro:

drake_example_cc_binary(
  ...
  deps = [
    "//solvers:solver_id",
  ]
  ...
)

This would re-link the solver_id.o into the binary, leading to an ODR violation versus the copy of solver_id.o in libdrake.so. Everything in deps for this macro should either start with@ or : or //examples, I think? Might be worth asserting that here? (Even some things in @ should not be re-linked here, but I guess there's only so much we can do. Manually wrangling ODR is just plain hard.)

I added some fail-fast checks and some comments.

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 1 of 15 files at r4, 1 of 8 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

tools/skylark/drake_cc.bzl, line 550 at r6 (raw file):

            fail("Dependency used in `drake_example_cc_binary()` macro should\
                not already be part of libdrake.so: %s" % dep)
    # This makes sure that //tools/install/libdrake:libdrake.so and

BTW The libdrake.so guard is unnecessary -- Bazel will already de-duplicate it for us; though I guess its harmless to keep it here.

---

Comments from Reviewable

[soonho-tri]

---

Reviewed 2 of 15 files at r4, 2 of 8 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[fbudin69500]

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

---

tools/skylark/drake_cc.bzl, line 550 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The libdrake.so guard is unnecessary -- Bazel will already de-duplicate it for us; though I guess its harmless to keep it here.

Better have extra guards than not enough :)

---

Comments from Reviewable

[fbudin69500]

Can this be merged now? I think it is ready.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

Blocked on a sentiment from @jamiesnape, but then yeah LGTM to merge.

---

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

---

Comments from Reviewable

[jamiesnape]

---

Reviewed 1 of 14 files at r3, 3 of 15 files at r4, 7 of 8 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable