Use yaml_cpp from Ubuntu Xenial.

[nuclearsandwich]

This splits the yaml-cpp changes from #7347 into a separate pull request.

yaml-cpp is already part of the setup scripts for Xenial and Mac.

CC collaborators @j-rivero and @clalancette

---

This change is 

[jamiesnape]

Note for reviewers, please verify that redistributable packages still work. Shambhala is the test case at this present moment in time.

---

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

tools/install/libdrake/drake.cps, line 87 at r1 (raw file):

      "X-CMake-Find-Args": ["CONFIG"]
    },
    "yaml-cpp": {

Note that the install packages would be broken with this change. See below for a temporary workaround.

---

tools/install/libdrake/drake.cps, line 106 at r1 (raw file):

      ],
      "Compile-Features": ["c++14"],
      "Link-Flags": ["-ltinyxml2"],

Changing this to

"Link-Flags": [
  "-ltinyxml2",
  "-lyaml-cpp"
],

would probably be good enough for now, but it is not entirely correct as it implicitly relies on the headers being the search path. Ideally, the yaml-cpp header would not be used in a public header, but that is not currently the case.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

tools/install/libdrake/drake.cps, line 106 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Changing this to

"Link-Flags": [
  "-ltinyxml2",
  "-lyaml-cpp"
],

would probably be good enough for now, but it is not entirely correct as it implicitly relies on the headers being the search path. Ideally, the yaml-cpp header would not be used in a public header, but that is not currently the case.

(also ideally Bazel would not suffer from #492 so this could be omitted altogether)

---

Comments from Reviewable

[jwnimmer-tri]

@drake-jenkins-bot mac-elcapitan-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please

+@jwnimmer-tri as platform review volunteer.
+@soonho-tri as 🍎 local build tester, please.

We will still need to find an Ubuntu volunteer to feature review and test vs Shambhala.

[jwnimmer-tri]

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

---

drake/automotive/maliput/monolane/BUILD.bazel, line 80 at r1 (raw file):

    deps = [
        ":builder",
        "@yaml_cpp",

Why is this removed? This is exactly the place it should have been.

---

drake/automotive/maliput/monolane/BUILD.bazel, line 127 at r1 (raw file):

        ":loader",
        "//drake/common:text_logging_gflags",
        "@yaml_cpp",

This cc file doesn't include anything from yaml, so this line should not be here.

---

drake/automotive/maliput/utility/BUILD.bazel, line 41 at r1 (raw file):

        "//drake/automotive/maliput/monolane",
        "//drake/common:text_logging_gflags",
        "@yaml_cpp",

This cc file doesn't include anything from yaml, so this line should not be here.

I pulled the PR locally to explore. The actual problem is that monolane's loader.cc fails to declare is deps on @yaml_cpp. Once it's added there, it's no longer needed here.

Ditto for the same edit below.

---

Comments from Reviewable

[stonier]

+@stonier as 🐧 (xenial) feature review and local build tester

---

Comments from Reviewable

[stonier]

Testing currently fine for this branch and shambhala (using this branch).

Awaiting changes requested by @jamiesnape and @jwnimmer-tri.

---

Comments from Reviewable

[nuclearsandwich]

Changing this to

"Link-Flags": [
  "-ltinyxml2",
  "-lyaml-cpp"
],

would probably be good enough for now, but it is not entirely correct as it implicitly relies on the headers being the search path. Ideally, the yaml-cpp header would not be used in a public header, but that is not currently the case.

Shall I make this change on the PR branch?

[jamiesnape]

Either that or a proper fix. This PR cannot break the packages.

[jwnimmer-tri]

If both Jamie is correct about the metadata changes being required, and Daniel is correct that Shambhala tests are passing, does that mean Shambhala is not yet a good enough test of the packages? Should we be testing PRs like this against, say, Spartan also?

---

Comments from Reviewable

[nuclearsandwich]

Either that or a proper fix. This PR cannot break the packages.

I will start with this change as I'm not clear on what the proper fix would be, nor how to test an attempt at one locally. What are you exercising to exhibit the breakage locally?

Should I be concerned that on both my local workspace and in an isolated docker container the PR as submitted builds (bazel build //...) without reported errors? It sounds like the two of you both had issues immediately.

---

Why is this removed? This is exactly the place it should have been.
...
This cc file doesn't include anything from yaml, so this line should not be here.

I pulled the PR locally to explore. The actual problem is that monolane's loader.cc fails to declare is deps on @yaml_cpp. Once it's added there, it's no longer needed here.

I will make these changes and test again. At the time I was under the impression that I was addressing build failures but I've rebased against master enough since then that it's entirely possible I bungled something but not enough to break my local build.

[jamiesnape]

I will start with this change as I'm not clear on what the proper fix would be, nor how to test an attempt at one locally. What are you exercising to exhibit the breakage locally?

Drake uses yaml-cpp in its public headers, you can use grep to verify that, so if someone uses that header in their code it will be broken. I have no idea if there is actually coverage of that.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

Should I be concerned that on both my local workspace and in an isolated docker container the PR as submitted builds (bazel build //...) without reported errors? It sounds like the two of you both had issues immediately.

FYI I did not experience any errors. My comments are all about code review and bad smells.

Jamie's comments about (Edit to add: include and) link flags are about whether Drake's installed or packaged binary release will work for users consuming it. Because of #7260, we don't have CI coverage of that yet. So the author (and reviewer, here Daniel) has to test manually against some exemplar downstream users -- Shambhala at least. (My recent question asks whether Shambhala on its own is good enough.)

[jwnimmer-tri]

+(status: curate commits before merging)
http://drake.mit.edu/reviewable.html#curated-commits

---

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

---

Comments from Reviewable

[soonho-tri]

🍎 tested. bazel test //drake/automotive/maliput/monolane/... -c dbg passed.

---

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

---

Comments from Reviewable

[stonier]

Jamie's comments about (Edit to add: include and) link flags are about whether Drake's installed or packaged binary release will work for users consuming it. Because of #7260, we don't have CI coverage of that yet. So the author (and reviewer, here Daniel) has to test manually against some exemplar downstream users -- Shambhala at least. (My recent question asks whether Shambhala on its own is good enough.)

Just to be clear, we do have CI coverage, but it's posthumous. Next steps are to get it into buildcop review and ultimately some form of it as a PR hook. Obviously, I am looking forward to that so I don't have a manual step involved.

Having said that, this wouldn't have been caught by CI since in normal cases it gets saved by discovering it via the default system path setting. We should actually provide the system include path and libs here, just like for any other installed cmake/pkgconfig module.

@jamiesnape what's the recommended way to handle this? Push the discovered yaml-cpp include directory into the "Includes" list?

---

Comments from Reviewable

[soonho-tri]

Note for reviewers, please verify that redistributable packages still work. Shambhala is the test case at this present moment in time.

I think I found an existing problem. Filed an issue at RobotLocomotion/drake-external-examples#56 .

---

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

---

Comments from Reviewable

[soonho-tri]

Please ignore my previous comment.

---

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

---

Comments from Reviewable

[jamiesnape]

@jamiesnape what's the recommended way to handle this? Push the discovered yaml-cpp include directory into the "Includes" list?

That would work on Linux, but is brittle on Mac since the pkg-config file there unfortunately points to /usr/local/Cellar/yaml-cpp/0.5.3 instead of /usr/local or /usr/local/opt/yaml-cpp. Probably the correct answer would be to write a find-module, install it, and reference it in drake-config.cmake.

---

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

---

Comments from Reviewable

[stonier]

That should be fine though since the setup script for Mac will make sure that gets installed?

Most other pkg-config scripts rely on the fact that their dependencies are explicitly installed by the system and don't go hunting for potentially a different one than the binaries were compiled with.

Additionally, there is only likely to be support for Mac binaries on local installs for the foreseeable future.

[jamiesnape]

That should be fine though since the setup script for Mac will make sure that gets installed?

The problem is when someone does brew upgrade and there happens to be an update to yaml-cpp, you end up with a disconnect.

[stonier]

We do not have it yet, but only planning on support for Mac binaries on local installs for the foreseeable future which should mostly avoid any brittleness with the system (patch) version changing. If for any reason that does happen - the policy would be just to make sure you update your drake install when you do a brew upgrade.

I'd prefer that than going hunting for and potentially finding the wrong yaml-cpp on linux.

[jamiesnape]

I'd prefer that than going hunting for and potentially finding the wrong yaml-cpp on linux.

Not sure how that would happen. The find-module would just call pkg-config.

[stonier]

Not sure how that would happen. The find-module would just call pkg-config.

Someone installs a (possibly non system patched) version in /usr/local.

I would rather it points explicitly to the dependencies it was created from rather than have the potential to be misdirected. If for homebrew that means upgrading the installed drake following a brew upgrade, then that's fine for the foreseeable future until we get to the point of producing mac binaries.

[jamiesnape]

I would rather it points explicitly to the dependencies it was created from rather than have the potential to be misdirected. If for homebrew that means upgrading the installed drake following a brew upgrade, then that's fine for the foreseeable future until we get to the point of producing mac binaries.

Ok, your decision, but from past experience, and the number of bug reports that it generates, that is not a good idea (you asked me for the "recommended way").

[jamiesnape]

So, in the absence of find-modules (for better or worse), we have a prototype pc2cps utility and I would suggest that this PR does the minimum to keep the packages working while we iterate on that, since this a general problem for all the new pkg-config dependencies.

[nuclearsandwich]

For what it is worth, we suffer a steady amount of issue traffic in ROS 2 as a result of hard coded paths to installed binaries (ros2/ros2#311 is a representative example). These paths change at upstream's discretion. There are platform limitations preventing us from working around our issue there easily, but hard coding these paths is going to be a definite developer experience hit, especially on a rolling release platform like MacOS + Homebrew.

[jwnimmer-tri]

platform

---

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

---

Comments from Reviewable

[jamiesnape]

Reviewed 4 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[nuclearsandwich]

Should I be addressing the two failures [1] [2] with the CMake builds?

[jwnimmer-tri]

Possibly they are due to Jenkins having not exactly the right sources established to build. Since there are merge conflicts anyway, perhaps just update the PR to base off latest master. The push from that will cause a re-test, and we'll see what it looks like then.

[jwnimmer-tri]

Reviewed 4 of 4 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

The cmake builds have passed now. We'll do a final check of Mac builds:

@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-elcapitan-clang-bazel-experimental-everything please

[jwnimmer-tri]

I've confirmed on Ubuntu that the library is listed:

jwnimmer@call-cc:~/jwnimmer-tri/drake-distro/tmp$ ldd lib/libdrake.so  | grep yaml
	libyaml-cpp.so.0.5 => /usr/lib/x86_64-linux-gnu/libyaml-cpp.so.0.5 (0x00007fd4bb00a000)

I've confirmed on Ubuntu that Shambhala drake_cmake_installed still works 🐧. I think we can de-assign @stonier if everything else is ready and he doesn't object.

@soonho-tri Do you want to re-test locally on Mac? If so, can you LGTM? If not, just de-assign yourself?

[soonho-tri]

• tested bazel test //drake/automotive/maliput/monolane/... -c dbg (passed).

---

Reviewed 3 of 8 files at r1, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

The elcapitan error is:

//drake/systems/controllers/qp_inverse_dynamics:valkyrie_balancing_test  FAILED in 47.0s

But I don't think that test uses yaml:

jwnimmer@call-cc:~/jwnimmer-tri/drake-distro$ bazel query 'somepath("//drake/systems/controllers/qp_inverse_dynamics:valkyrie_balancing_test", "@yaml_cpp//:yaml_cpp")'
INFO: Empty results

so probably not the fault of this PR, but rather just a flake. We'll try a retest.

[jwnimmer-tri]

@drake-jenkins-bot mac-elcapitan-clang-bazel-experimental-everything please