Use the system protobuf. #7323

clalancette · 2017-10-24T12:42:19Z

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.

This is the third part of #7278 , and makes things work properly on Linux. This PR should be considered an RFC, because I know for a fact that it does not work as-is on MacOS. I was able to make MacOS work by manually doing export PKG_CONFIG_PATH=/usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig and by changing $(which protoc) to /usr/local/Cellar/protobuf@2.6/2.6.0/bin/protoc on tools/skylark/drake_proto.bzl, but those are obviously hacks. I'm looking for some advice on how to make this work for MacOS, and possibly get a login for a MacOS machine to continue debugging (I lost access to the one I was using).

This change is

jamiesnape · 2017-10-24T14:31:34Z

For the Mac part, we do know with certainty the location of the pkg-config file, /usr/local/opt/protobuf@2.6/lib/pkgconfig and the libraries themselves. Personally, for now, I would add a field to pkg_config_package that adds that path to the PKG_CONFIG_PATH before calling the pkg-config executable. Alternatively, we wait for a rewrite of how we handle system dependencies.

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

WORKSPACE, line 60 at r1 (raw file):

pkg_config_package(
    name = "systemprotobuf",

BTW Is there a technical reason for this not being simply protobuf?

Comments from Reviewable

jwnimmer-tri · 2017-10-24T14:37:47Z

The bzl support code that we've forked from upstream should not be misrepresented as our own. It must live under //third_party inside Drake, with appropriate copyright and/or license statements. We should have a first commit in the PR that adds a pristine copy of it (with upstream citation details, e.g., a commit-specfic github URL of the original material), and then additional commits in the same PR can make whatever local modifications we need, keeping them as minimal as practical.

jamiesnape · 2017-10-24T14:40:57Z

Does the system protobuf on Xenial have a protobuf-config file or similar? If not, then drake.cps and the install target will need fixing up.

clalancette · 2017-10-24T18:00:22Z

The bzl support code that we've forked from upstream should not be misrepresented as our own. It must live under //third_party inside Drake, with appropriate copyright and/or license statements. We should have a first commit in the PR that adds a pristine copy of it (with upstream citation details, e.g., a commit-specfic github URL of the original material), and then additional commits in the same PR can make whatever local modifications we need, keeping them as minimal as practical.

OK, that sounds fine to me. I'll rebase it to do that. About the copyright/license; the original doesn't have any copyright/license statement in it. Should we keep it as it is, should we take the license from the upstream protobuf repository (looks MIT-ish), or do something else?

jwnimmer-tri · 2017-10-24T18:04:31Z

I'm slightly lost. You should use the license from wherever you copied it from. (Or if it didn't have one, we can't use it.) If https://github.com/google/protobuf/blob/master/protobuf.bzl was what you started from, then https://github.com/google/protobuf/blob/master/LICENSE would be the license.

clalancette · 2017-10-24T18:07:24Z

I'm slightly lost. You should use the license from wherever you copied it from. (Or if it didn't have one, we can't use it.) If https://github.com/google/protobuf/blob/master/protobuf.bzl was what you started from, then https://github.com/google/protobuf/blob/master/LICENSE would be the license.

Sorry I wasn't clear. That's exactly where I copied from to start with. The file itself didn't have a license header, but the repository as a whole does. I didn't realize that we just ended up putting the whole LICENSE file into the //third_party area, but I see that we do. So that makes sense; I'll import that file and the LICENSE file as well. Thanks for the clarification.

clalancette · 2017-10-24T18:09:19Z

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

WORKSPACE, line 60 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW Is the a technical reason for this not being simply protobuf?

No, not really. I could easily enough rename it to just "protobuf" if that is preferred.

Comments from Reviewable

clalancette · 2017-10-24T18:14:53Z

Does the system protobuf on Xenial have a protobuf-config file or similar? If not, then drake.cps and the install target will need fixing up.

The Xenial libprotobuf-dev package contains the headers and a pkg-config .pc file. I think you are referring to a cmake configuration file above; is that correct? If so, libprotobuf-dev does not contain it, so if we want that, we'd have to change drake.cps as you mention.

jamiesnape · 2017-10-24T18:15:57Z

I think you are referring to a cmake configuration file above; is that correct?

Yes, exactly.

clalancette · 2017-10-24T19:49:10Z

The code that I just pushed should resolve the initial comments, with the exception of generating a cmake file as requested by @jamiesnape . I'll work on that next.

clalancette · 2017-10-24T19:50:45Z

Review status: 0 of 14 files reviewed at latest revision, 1 unresolved discussion.

WORKSPACE, line 60 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

No, not really. I could easily enough rename it to just "protobuf" if that is preferred.

Actually, with the new "third_party" thing that we are doing, it can't just be called protobuf anymore, because that name is now taken up by the third_party/com_github_google_protobuf workspace. So I'm going to leave this as systemprotobuf, unless we come up with a better name.

Comments from Reviewable

jamiesnape · 2017-10-24T20:10:29Z

Review status: 0 of 14 files reviewed at latest revision, 1 unresolved discussion.

WORKSPACE, line 60 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Actually, with the new "third_party" thing that we are doing, it can't just be called protobuf anymore, because that name is now taken up by the third_party/com_github_google_protobuf workspace. So I'm going to leave this as systemprotobuf, unless we come up with a better name.

I would probably say something like libprotobuf since all the pkg_config_package calls are to system something, but it is not that important.

Comments from Reviewable

clalancette · 2017-10-24T21:59:59Z

The code that I just pushed should resolve the initial comments, with the exception of generating a cmake file as requested by @jamiesnape . I'll work on that next.

After looking into this, I realized that I was actually wrong in my previous answer. While libprotobuf-dev does indeed not ship with a cmake file, the package cmake-data (part of standard cmake) has a built-in module for protobuf in /usr/share/cmake-3.5/Modules/FindProtobuf.cmake. It is a fairly straightforward cmake module that uses find_library to find the protobuf, protobuf-lite, and protoc libraries, along with the protoc executable. @jamiesnape Given that, do you still think we need to generate cmake files with this PR?

jamiesnape · 2017-10-25T12:25:30Z

CMake always bundles FindProtobuf.cmake. The issue is that the version in CMake 3.5.1 has issues. I would be happy bundling the version here (from CMake 3.10), setting the module path to include it and then using that. (Put it in third_party/com_kitware_gitlab_cmake_cmake/3.10/Modules and install it to lib/cmake/drake/modules/3.10.)

jamiesnape · 2017-10-25T16:55:06Z

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

WORKSPACE, line 68 at r4 (raw file):

    modname = "protobuf",
    # This is the hard-coded path to the Homebrew package on MacOS.
    pkg_config_path = "/usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig",

Should be /usr/local/opt/protobuf@2.6/lib/pkgconfig. If the bottles are rebuilt or the formula edited, this would break.

Comments from Reviewable

jamiesnape · 2017-10-25T16:55:40Z

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

WORKSPACE, line 68 at r4 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Should be /usr/local/opt/protobuf@2.6/lib/pkgconfig. If the bottles are rebuilt or the formula edited, this would break.

(also macOS, not MacOS)

Comments from Reviewable

clalancette · 2017-10-25T16:59:53Z

I've just pushed a commit that does make things work on MacOS. As pointed out in the commit message, it is kind of icky because we have the same hard-code /usr/local/Cellar/protobuf@2.6/2.6.0 encoded in several places, and we also expose the /usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig at the top-level WORKSPACE file.

I've tried a few different things to make this nicer, but I haven't been successful with any of them:

I attempted to use action_env=PATH=<hard-coded paths> in tools/bazel.rc (along with PKG_CONFIG_PATH), but there isn't really a way to only do it if the platform is OSX.
I tried to make a new repository rule that would call out to pkg_config_package conditionally based on MacOS, but that doesn't work because pkg_config_package is a repository rule and thus must be called from the WORKSPACE file.
I tried to embed the hard-coded /usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig path down in pkg_config.bzl. This works, but violates the layering by adding package-specific data down in a generic tool.

At this point, I'm kind of out of ideas, so additional thoughts on how to make this nicer would be appreciated.

jamiesnape · 2017-10-25T17:06:43Z

I don't have an issue with adding the /usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig in the WORKSPACE other than you should not be using the path in Cellar in any of the places that it is used in this PR because it may change to /usr/local/Cellar/protobuf@2.6/2.6.0_1 if they have to rebuild the formula for whatever reason. That is why they have the /usr/local/opt/protobuf@2.6 symlink.

clalancette · 2017-10-25T17:09:32Z

I don't have an issue with adding the /usr/local/Cellar/protobuf@2.6/2.6.0/lib/pkgconfig in the WORKSPACE other than you should not be using the path in Cellar in any of the places that it is used in this PR because it may change to /usr/local/Cellar/protobuf@2.6/2.6.0_1 if they have to rebuild the formula for whatever reason. That is why they have the /usr/local/opt/protobuf@2.6 symlink.

I'm happy to change those paths to the more stable ones. I'll do that now. Assuming we are OK with this approach, I think the last currently open request has to do with importing the newer cmake protobuf module.

jamiesnape · 2017-10-25T17:13:26Z

Not so happy with the PATH setting in protobuf.bzl. I wonder if there is a way we can make it more obvious.

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

clalancette · 2017-10-25T17:20:30Z

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

WORKSPACE, line 68 at r4 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

(also macOS, not MacOS)

Done.

Comments from Reviewable

clalancette · 2017-10-25T17:20:55Z

I'm not really that happy with it either. I'll tinker around some more and see if I can do something better.

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

jamiesnape · 2017-10-25T20:08:25Z

Would it be less painful to support the latest protobuf on Mac? Do we need compatibility between Mac protobufs and Linux protobufs?

clalancette · 2017-10-26T14:08:13Z

Would it be less painful to support the latest protobuf on Mac?

Yeah, I think it would be. If we just supported the latest protobuf, I don't think we'd have to munge the PATH at all in protobuf.bzl, and it would just find the latest installed protoc. I'm less sure about the PKG_CONFIG_PATH, but it is definitely worth a try.

Do we need compatibility between Mac protobufs and Linux protobufs?

The one area of concern here is that protobuf is being used to store "configuration" data out to disk. The question is whether that data is wire-compatible between version 2 and version 3, given that we are explicitly specifying version 2 in the .proto files. I'll do a quick test here and see if they are compatible or not.

jamiesnape · 2017-10-26T14:10:27Z

I'm less sure about the PKG_CONFIG_PATH, but it is definitely worth a try.

You would not need that either. The .pc is in the default path for the latest protobuf.

clalancette · 2017-10-26T16:38:55Z

You would not need that either. The .pc is in the default path for the latest protobuf.

Got it.

The one area of concern here is that protobuf is being used to store "configuration" data out to disk. The question is whether that data is wire-compatible between version 2 and version 3, given that we are explicitly specifying version 2 in the .proto files. I'll do a quick test here and see if they are compatible or not.

After going over how protobuf is used more carefully, in all cases except for the Matlab RPC, we are using the "text" representation of protobufs. We know that this works in both protobuf 2 and protobuf 3 because both the master branch and this branch pass tests. This leaves the matlab RPC question. After doing some testing there, it looks like protoc version 2, compiling syntax 2, and protoc version 3, compiling syntax 2, both output the same on-the-wire format. For comparison, protoc version 3, compiling syntax 3, outputs a slightly different on-the-wire format. Thus, I think things will remain compatible, even if macOS and Xenial are on different versions. Assuming there is no other objection to them having different versions, I will change this patch up to use the "default" protobuf package on macOS, which gets rid of a lot of the nastiness. This will unfortunately require another change to install_prereqs.sh, but oh well.

clalancette · 2017-10-26T19:38:14Z

OK, so the latest goes back to using whatever "default" macOS has installed. I was not able to test it on macOS yet, but I should be getting a Mac in the mail tomorrow. @soonho-tri If you have time to run the compile and tests on this branch today, that would be helpful, but if not, I'll do the testing tomorrow. Now that I think we are mostly in agreement on that, I'll look into importing the cmake file that @jamiesnape suggested.

jamiesnape · 2017-10-26T19:57:33Z

If you update install_prereqs.sh in this PR, you can call one of the unprovisioned Mac jobs on Jenkins to test also.

soonho-tri · 2017-10-26T20:18:11Z

@soonho-tri If you have time to run the compile and tests on this branch today, that would be helpful, but if not, I'll do the testing tomorrow.

@clalancette, I'll do it this evening.

jamiesnape · 2017-12-15T20:20:49Z

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

tools/workspace/protobuf/find_protobuf_cmake.BUILD.bazel, line 15 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Could Copyright.txt possibly collide with anything else in the future?
Could it be renamed to be specific to FindProtobuf?

BTW All files that we would add here would come from CMake 3.10 or earlier, so the Copyright.txt would always be the same file from CMake.

CMakeLists.txt, line 78 at r33 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

In fact, we do seem to be doing that already for libdrake.so.

Also, thinking about it --linkopt=-Wl,-rpath=${PROTOBUF_LIBRARY_DIR} gets removed during the install so just setting this to something

Comments from Reviewable

jwnimmer-tri · 2017-12-15T20:26:52Z

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

tools/skylark/drake_cc.bzl, line 398 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Another naive question: Has this conditional been tested?
Is select valid in a macro file, or does it need to be native.select?

OK Yes, this code is definitely being run (libdrake.so, as well as pybind.bzl, use it with linkshared = 1).

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Can this be slightly modified to into a BUILD label //:$name?

What's the thought process? If name and command are identical, then there would be no change. If they differ, to my thinking having the label match the command name like @repos//:command is clearer than having it match the repository name like @repos. Was this just a request for brevity? Maybe defer that until we need it in situ somehow?

Comments from Reviewable

clalancette · 2017-12-15T20:30:59Z

Review status: 17 of 18 files reviewed at latest revision, 8 unresolved discussions.

setup/ubuntu/16.04/install_prereqs.sh, line 100 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

Naive question, but how do we ensure that Mac already has patchelf present? I don't see it in mac/install_prereqs.sh.

The changes to use patchelf only happen on Linux, so we don't need it on macOS.

third_party/com_github_google_protobuf/BUILD.bazel, line 1 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

BTW This looks like our custom code; could we add that provenance file which indicates that all except a few files are part of protobuf? (and possibly mention referring to the git log?)

Sorry, I'm not sure I understand which 'provenance file' you are talking about. Can you elaborate?

tools/install/install.py.in, line 191 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

FYI This could be condensed with OrderedDict (see here):
from collections import OrderedDict
rpath = OrderedDict.fromkeys(rpath).keys()

Ah, much nicer. Changed to that now; thank you!

tools/skylark/drake_cc.bzl, line 414 at r40 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This would fail if linkopts was already in kwargs. Better to add it to the named arguments above, perhaps.

Fixed now.

tools/skylark/drake_cc.bzl, line 398 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Another naive question: Has this conditional been tested?
Is select valid in a macro file, or does it need to be native.select?

I've tested this locally (on both Linux and macOS), and it seems to work on CI (with the exception of known bad tests failing). Also, if this wasn't doing the right thing, then we know for sure that the MATLAB tests would crash. So I'm fairly confident this works.

CMakeLists.txt, line 78 at r33 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Also, thinking about it --linkopt=-Wl,-rpath=${PROTOBUF_LIBRARY_DIR} gets removed during the install so just setting this to something

@jamiesnape I think your last comment got cut-off; was there a change that you would like to see here?

Comments from Reviewable

jamiesnape · 2017-12-15T20:33:25Z

Review status: 17 of 18 files reviewed at latest revision, 7 unresolved discussions.

CMakeLists.txt, line 78 at r33 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

@jamiesnape I think your last comment got cut-off; was there a change that you would like to see here?

I honestly forget now, but I think we good in the current iteration.

Comments from Reviewable

jwnimmer-tri · 2017-12-15T20:35:12Z

Review status: 17 of 18 files reviewed at latest revision, 6 unresolved discussions.

third_party/com_github_google_protobuf/BUILD.bazel, line 1 at r41 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Sorry, I'm not sure I understand which 'provenance file' you are talking about. Can you elaborate?

In this directory, some files are copyright Google, and some files are copyright Drake. Eric is asking for that distinction to be made clearer.

The second part was about making it clear that the origin commit of protobuf.bzl is in its git history. I guess some people expect a file in this directory to have breadcumbs, rather than just git log. (I always just use git log myself.)

Probably a short README.md would meet both needs.

Comments from Reviewable

jwnimmer-tri · 2017-12-15T20:35:35Z

Reviewed 1 of 1 files at r42.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

clalancette · 2017-12-15T20:51:22Z

Review status: 18 of 19 files reviewed at latest revision, 5 unresolved discussions.

third_party/com_github_google_protobuf/BUILD.bazel, line 1 at r41 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In this directory, some files are copyright Google, and some files are copyright Drake. Eric is asking for that distinction to be made clearer.

The second part was about making it clear that the origin commit of protobuf.bzl is in its git history. I guess some people expect a file in this directory to have breadcumbs, rather than just git log. (I always just use git log myself.)

Probably a short README.md would meet both needs.

I added a README now.

Comments from Reviewable

EricCousineau-TRI · 2017-12-15T20:53:06Z

BTW Chris, you may have introduced blocking comments through your responses. If you prefix it with Done., or manually set it to satisfied, then it should be unblocked.

Reviewed 1 of 1 files at r42, 1 of 1 files at r43.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What's the thought process? If name and command are identical, then there would be no change. If they differ, to my thinking having the label match the command name like @repos//:command is clearer than having it match the repository name like @repos. Was this just a request for brevity? Maybe defer that until we need it in situ somehow?

The request was for accuracy; if the user for some reason specifies a name different than command, then it will not be under @repos//:$command.
If it should always be the same, then perhaps there should be wrapper macro?

def which(command):
    _which(
        name = command,
        command = command,
    )

Comments from Reviewable

jwnimmer-tri · 2017-12-15T20:54:41Z

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

The request was for accuracy; if the user for some reason specifies a name different than command, then it will not be under @repos//:$command.
If it should always be the same, then perhaps there should be wrapper macro?
def which(command):
    _which(
        name = command,
        command = command,
    )

If the user does name = foo, command = bar, then tey get @foo//:bar; that seems okay? The label matches the command, the repository matches the name? Or am I misunderstanding something?

Comments from Reviewable

EricCousineau-TRI · 2017-12-15T21:11:31Z

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

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If the user does name = foo, command = bar, then tey get @foo//:bar; that seems okay? The label matches the command, the repository matches the name? Or am I misunderstanding something?

Ah, no, that was me misunderstanding.
Could this be updated to then be ... into a BUILD label @$name//:$command(or@$commandof they match), since this command itself does not do the aliasing? And could the mention of aliasing be put into a@note`?

(Also, at some point, it'd be nice to have this get multiple commands, so I could use it something like call_python_full_test and not need to use local to avoid Mac permission issues; however, I'll save that for a new PR.)

Comments from Reviewable

jwnimmer-tri · 2017-12-15T21:14:12Z

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

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, EricCousineau-TRI wrote…

Ah, no, that was me misunderstanding.
Could this be updated to then be ... into a BUILD label @$name//:$command(or@$commandof they match), since this command itself does not do the aliasing? And could the mention of aliasing be put into a@note`?

(Also, at some point, it'd be nice to have this get multiple commands, so I could use it something like call_python_full_test and not need to use local to avoid Mac permission issues; however, I'll save that for a new PR.)

Great, I'd support @name//:command or @$name//:$command here, instead of //:$command. Chris , over to you to push at that fix at some point?

Comments from Reviewable

EricCousineau-TRI · 2017-12-15T21:21:37Z

pending final discussion points. Everything worked per this PR.

Note: Seems like the installed pydrake libraries get confused whenGetDrakePath is called more than once. Running the tests only has a few pass:

# Do installation...
cd bindings/pydrake
find . -wholename '*/test/*_test.py' | xargs -n 1 python

However, this behavior happens on master. Will file separate issue.

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

Comments from Reviewable

Taken from https://github.com/google/protobuf/blob/4fc9304/protobuf.bzl Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This means using both the system version of the protoc compiler and the system libprotobuf.so to link against. To do this, we use the pristine version of 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 comment in the code explains why. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-12-18T14:55:48Z

Review status: 16 of 19 files reviewed at latest revision, 2 unresolved discussions.

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Great, I'd support @name//:command or @$name//:$command here, instead of //:$command. Chris , over to you to push at that fix at some point?

I've changed the language to use @$name//:$command as requested.

As far as the aliasing @note goes, I'm not quite sure I understand. From my understanding, what is happening here is that the top-level BUILD.bazel is aliasing a //:protoc rule to @com_google_github//:protoc. In turn, @com_google_github//:protoc depends on the @protoc rule, which comes from the top-level WORKSPACE file. In theory, we could drop the top-level BUILD.bazel aliasing completely, so this doesn't seem to be something intrinsic to which, just an implementation detail here. Or am I misunderstanding something?

Comments from Reviewable

clalancette · 2017-12-18T14:56:59Z

Review status: 16 of 19 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I'm skimming the commits correctly, we still have some back-and-forth redesign in the commit history. I think that still needs cleanup. (Fine to defer until after future edits.)

I've now rebased this onto master, and squashed everything down to 3 different commits (the original pristine protobuf.bzl, the local changes, and then the changes to make sure RPATH works). That seemed to make sense to me, but let me know if you want me to squash it down into 2 again.

Comments from Reviewable

jwnimmer-tri · 2017-12-18T15:07:30Z

pending CI. (Before merge, we should opt-in to macOS builds.)

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

a discussion (no related file):

Previously, clalancette (Chris Lalancette) wrote…

I've now rebased this onto master, and squashed everything down to 3 different commits (the original pristine protobuf.bzl, the local changes, and then the changes to make sure RPATH works). That seemed to make sense to me, but let me know if you want me to squash it down into 2 again.

SGTM.

Comments from Reviewable

clalancette · 2017-12-18T15:08:54Z

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

EricCousineau-TRI · 2017-12-18T15:48:53Z

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

tools/workspace/which.bzl, line 26 at r41 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

I've changed the language to use @$name//:$command as requested.

As far as the aliasing @note goes, I'm not quite sure I understand. From my understanding, what is happening here is that the top-level BUILD.bazel is aliasing a //:protoc rule to @com_google_github//:protoc. In turn, @com_google_github//:protoc depends on the @protoc rule, which comes from the top-level WORKSPACE file. In theory, we could drop the top-level BUILD.bazel aliasing completely, so this doesn't seem to be something intrinsic to which, just an implementation detail here. Or am I misunderstanding something?

Sorry, that was just me mixing my confusion with using //:$command with the wording mentioning alias (which at that point had implied that doing something in BUILD was necessary).

As is, it is now clear that modifying BUILD is not necessary to achieve what the doc states.

Comments from Reviewable

clalancette · 2017-12-18T17:03:34Z

The macOS failures were caused by something else, and were fixed by 96d1681 . So they can safely be ignored in this PR.

jwnimmer-tri · 2017-12-18T18:03:46Z

Agreed. Merging now.

clalancette force-pushed the use-libprotobuf branch from a5fa30d to fa5dbbd Compare October 24, 2017 19:48

EricCousineau-TRI mentioned this pull request Dec 15, 2017

pydrake: Installed version can only use GetDrakePath once before it fails? #7624

Closed

clalancette added 3 commits December 18, 2017 14:21

Import pristine protobuf.bzl file from protobuf 3.5.x.

ff622ca

Taken from https://github.com/google/protobuf/blob/4fc9304/protobuf.bzl Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Make sure to use old dtags on Linux to get RPATH.

bf3bad0

The comment in the code explains why. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette force-pushed the use-libprotobuf branch from b266404 to bf3bad0 Compare December 18, 2017 14:55

jwnimmer-tri merged commit f9baebd into RobotLocomotion:master Dec 18, 2017

clalancette deleted the use-libprotobuf branch December 18, 2017 18:11

This was referenced Dec 18, 2017

Use the system version of protobuf #7278

Closed

Fix tests using protobuf with ubsan #7638

Closed

This was referenced Jan 12, 2018

Factor misc repositories into bzl helpers #7743

Merged

Avoid using local_repository for com_google_protobuf #7745

Merged

Use the system protobuf. #7323

Use the system protobuf. #7323

Conversation

clalancette commented Oct 24, 2017 • edited by sherm1 Loading

jamiesnape commented Oct 24, 2017 • edited Loading

jwnimmer-tri commented Oct 24, 2017 • edited Loading

jamiesnape commented Oct 24, 2017

clalancette commented Oct 24, 2017

jwnimmer-tri commented Oct 24, 2017

clalancette commented Oct 24, 2017

clalancette commented Oct 24, 2017

clalancette commented Oct 24, 2017

jamiesnape commented Oct 24, 2017

clalancette commented Oct 24, 2017

clalancette commented Oct 24, 2017

jamiesnape commented Oct 24, 2017

clalancette commented Oct 24, 2017

jamiesnape commented Oct 25, 2017 • edited Loading

jamiesnape commented Oct 25, 2017

jamiesnape commented Oct 25, 2017

clalancette commented Oct 25, 2017

jamiesnape commented Oct 25, 2017 • edited Loading

clalancette commented Oct 25, 2017

jamiesnape commented Oct 25, 2017

clalancette commented Oct 25, 2017

clalancette commented Oct 25, 2017

jamiesnape commented Oct 25, 2017

clalancette commented Oct 26, 2017

jamiesnape commented Oct 26, 2017

clalancette commented Oct 26, 2017

clalancette commented Oct 26, 2017

jamiesnape commented Oct 26, 2017

soonho-tri commented Oct 26, 2017

jamiesnape commented Dec 15, 2017

jwnimmer-tri commented Dec 15, 2017

clalancette commented Dec 15, 2017

jamiesnape commented Dec 15, 2017

jwnimmer-tri commented Dec 15, 2017

jwnimmer-tri commented Dec 15, 2017

clalancette commented Dec 15, 2017

EricCousineau-TRI commented Dec 15, 2017

jwnimmer-tri commented Dec 15, 2017

EricCousineau-TRI commented Dec 15, 2017

jwnimmer-tri commented Dec 15, 2017

EricCousineau-TRI commented Dec 15, 2017

clalancette commented Dec 18, 2017

clalancette commented Dec 18, 2017

jwnimmer-tri commented Dec 18, 2017

clalancette commented Dec 18, 2017

EricCousineau-TRI commented Dec 18, 2017

clalancette commented Dec 18, 2017

jwnimmer-tri commented Dec 18, 2017

clalancette commented Oct 24, 2017 •

edited by sherm1

Loading

jamiesnape commented Oct 24, 2017 •

edited

Loading

jwnimmer-tri commented Oct 24, 2017 •

edited

Loading

jamiesnape commented Oct 25, 2017 •

edited

Loading

jamiesnape commented Oct 25, 2017 •

edited

Loading