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

Use the system version of protobuf #7278

Closed
wants to merge 3 commits into from

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Oct 17, 2017

Having an internal copy of protobuf linked to libdrake.so is problematic for external users. There are 2 major problems:

  1. If an external user wants to link against libdrake.so and link against a different version of protobuf, the symbols will collide and cause havoc.
  2. Since bazel run //:install installs the internal protobuf headers, anything trying to build against the libdrake.so headers will automatically get a newer version of protobuf headers, which they may not want.

To fix the above problems, this fairly large PR does two major things:

  1. It changes drake to use the system version of protobuf instead of a WORKSPACE downloaded version. In particular, on Ubuntu 16.04, this will use the system versions of protobuf 2.6 instead of the downloaded 3.1 that was being used before. This is made to work by copying protobuf.bzl from the upstream protobuf project (https://github.com/google/protobuf/blob/master/protobuf.bzl), and modifying it. The modifications mostly consist of renaming the rules to be cc_sysproto_library and py_sysproto_library, as well as changing how protoc gets found by using command in the ctx.action instead of executable. Once that is in place, we then have to modify the .proto files throughout the tree to make them protobuf-2 compatible (as a side-note, protobuf-3 can also generate code for these protobuf-2 style .proto files, so this should be compatible into the future).

  2. It splits out any components of libdrake.so that contained protobuf symbols. The thinking here is that libdrake.so will impose no protobuf on a user that wants to link against it. In order to do this, anything that directly depends on protobuf gets pushed out to a libdrake-loader.so, which does depend on protobuf. This was accomplished by creating factory functions for the two places that do actually use protobuf; RigidBodyAliasTreeGroupsLoadFromFile and ParamSetLoadFromFile. These two factory functions create an object (RigidBodyTreeAliasGroups and ParamSet, respectively), populate it from the protobuf file being passed in, and then return that object as a std::unique_ptr. Consumers of this can then use the unique_ptr or trivially convert it to a shared_ptr at their convenience. Note that the patch that implements all of the above is larger than might be assumed because I had to push the object passing up through several layers to split it out properly.

A few of things to note:

  1. Depending on how CI is configured, this may fail CI because it requires 2 new packages on Ubuntu 16.04 (libprotobuf-dev and protobuf-compiler) and one new package on MacOS (protobuf@2.6).
  2. I haven't actually tested this on MacOS, since I don't have a machine handy to test with. There may be some gotcha there.
  3. If desired, this can actually be split into two PRs, though the second patch can't go in until the first one does. Just let me know if that is preferable.

This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator

FYI The bzl macros that declare proto libraries should probably rebase onto #7279.

For the rest, we should probably first do a short design review to agree on the overall approach, and then work the changes into separate PRs. For example, some the C++ refactoring could probably go in first.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Oct 17, 2017

Depending on how CI is configured, this may fail CI because it requires 2 new packages on Ubuntu 16.04 (libprotobuf-dev and protobuf-compiler) and one new package on MacOS (protobuf@2.6).

The default CI builders use a pre-built AMI image and ignore the install_prereqs.sh script. The mechanism we'll want for this PR is to ask the jenkins bot to build linux-xenial-unprovisioned-clang-bazel-experimental (that name comes from https://drake-jenkins.csail.mit.edu/view/Unprovisioned/). That build will run the install_prereqs.sh script; if the build passes, it will confirm that the PR as-written would pass if the CI image had those new packages installed. Once that's passing, we'll open up a new PR that just has the install_prereqs.sh changes. Once that PR is reviewed and merged onto master, then we'll open an issue to ask for the AMIs to be updated. After that issue is resolved, then this PR can then be rebased and should pass normal CI.

@clalancette
Copy link
Contributor Author

For the rest, we should probably first do a short design review to agree on the overall approach, and then work the changes into separate PRs. For example, some the C++ refactoring could probably go in first.

Sure, sounds good. What's the usual procedure for doing a design review? @stonier should be involved as well.

@jwnimmer-tri
Copy link
Collaborator

We've already discussed the approach in email, so I think we're already mostly aligned. I was unsure whether there were any further PR-spanning caveats that we should address up-front, given the code now on the table, but after a quick skim I think it will be OK to address it chunk-wise, so I say we can skip any more pre-review phases.

I see at least these changes mixed into this PR:

  1. Add new items to install_prereqs.sh for Ubuntu.
  2. Add new items to install_prereqs.sh for Mac (missing so far?).
  3. Change the *.proto contents to use syntax = "proto2".
  4. Switch protoc and libprotobuf from proto3 to proto2.
  5. Refactor the alias_groups C++ code.
  6. Change the packaging of libdrake.so to have multiple shared libraries.

I think each of those items could be its own PR, filed one at a time (waiting for the prior step to merge before opening the next one)? We could still keep this PR open (labeled as "status: do not review") with the full set of commits for design reference and CI feedback on the end goal. I have seen that workflow do well for large changes. It should help focus review on each piece, without letting, e.g., the debate on Step (5) get in the way of Step (2), and allowing for the most appropriate reviewers for each step to be assigned the review. Would a plan like that work for you?

Another reason I suggest breaking it up is that, per email, I am still not on board with Step (6). I think in the medium term, either we drop the code in question from the install entirely, or we install it as part of libdrake.so, no middle ground. Yes, we should eventually modularize Drake's binary library images, but not in a brittle and haphazard way like this PR's code to achieve Step (6) embodies. But I'll also assert that we can hold that debate until Step (5) or (6) and still gain value by getting (1..4) onto master.

In any case, I think the most immediate next step is to rebase this PR onto the latest master, so that it can be run in CI, then ask the bot to run an unprovisioned Ubuntu build to prove it passes in final form, and then we can get Step (1) merged so that CI is ready for whatever comes next.

@clalancette
Copy link
Contributor Author

That sounds reasonable to me. I'm going to leave this open as you mention for reference purposes, and I'll start opening smaller PRs with the various pieces.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Oct 19, 2017

I had another realization: in terms of evidence for what packages shall be needed for CI, so that we can merge the update_prereqs.sh changes and make new AMIs, I'd be satisfied with either a CI build of -unprovisioned on this PR (as mentioned before), or if you assert that a local Docker build with a bazel test //... passes.

This means using both the system version of the protoc compiler
and the system libprotobuf.so to link against.  To do this,
we copy a version of the protobuf.bzl file from upstream protobuf,
and modify it to use "command" instead of "executable" to run
protoc.  We then use pkg-config to find the libprotobuf.so
flags as appropriate.  The combination of all of this seems
to make things work properly.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
The point of this is to make sure that no symbols from libprotobuf
are needed or exported by libdrake.so.  That way, the core
of libdrake.so just has objects, and they can be optionally
populated by protobuf by linking against an additional library
called libdrake-loader.so.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@jwnimmer-tri
Copy link
Collaborator

@clalancette Should this PR, a prior draft of the protobuf changes, be closed now?

@clalancette
Copy link
Contributor Author

The only reason to keep this one open would be because of point 2 in the initial comment (basically, splitting libdrake.so up so it has a loader section). We don't have a PR for that (or agreement on that way forward, for that matter), so if you don't think we need to track that for now, I'm fine with closing it.

@jwnimmer-tri
Copy link
Collaborator

Thanks, I'd forgotten about that. If you want to leave open for that reason, that's fine by me.

@clalancette
Copy link
Contributor Author

PR #7323 went in now, letting us use the system protobuf library and doing the first of the two things this original PR did. The second part of this PR was splitting libdrake.so up into two parts, one of which used the protobuf symbols, and one of which didn't. @stonier Are you still interested in doing that second part? If so, we probably need to have a design review, since we don't have consensus on how to move forward with that.

@stonier
Copy link
Contributor

stonier commented Dec 18, 2017

Short story - I'm fine with closing this one out here.

We can shelve the loader module to do 'as needed'. If there's a plan to modularise the drake libraries which has been recently discussed, then may be a good time to re-raise the issue in advance.

@clalancette
Copy link
Contributor Author

Short story - I'm fine with closing this one out here.

We can shelve the loader module to do 'as needed'. If there's a plan to modularise the drake libraries which has been recently discussed, then may be a good time to re-raise the issue in advance.

Sounds good. Closing this out (but keeping the branch around on the OSRF fork).

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

Successfully merging this pull request may close these issues.

3 participants