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

Avoid using local_repository for com_google_protobuf #7745

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 12, 2018

Avoid using local_repository for @com_google_protobuf.

Also properly install the ubsan fixup.

Also move the ubsan fixup cc_library out of the @com_google_protobuf repository. If a downstream Bazel user of Drake were to choose to use modern @com_google_protobuf (not Drake's vendored copy) then this label would go missing and drake_proto.bzl would have errors. Instead, we build and install this header as part of Drake.

Builds on #7323 and #7644.

Relates #7259.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

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

@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-memcheck-ubsan-everything please

@jwnimmer-tri
Copy link
Collaborator Author

+@EricCousineau-TRI and +@soonho-tri both for all review, please. This is a tricky one so I want a thorough look.

FYI @jamiesnape or @clalancette feel free to jump in for review if you desire.

@soonho-tri
Copy link
Member

:lgtm:


Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@soonho-tri
Copy link
Member

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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

Checkpoint, design question regarding generality.


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


common/proto/BUILD.bazel, line 94 at r2 (raw file):

    include_prefix = "drake/common/proto",
    strip_include_prefix = "/third_party/com_github_google_protobuf",
)

FYI Is there any way to expose visibility to a certain macro?
(I've found it annoying that I have expose a utility target as public, even though it'll only ever be used by a macro.)


tools/workspace/com_google_protobuf/package.bzl, line 11 at r2 (raw file):

def _impl(repository_ctx):
    # Bring in our hand-written BUILD file for @com_google_protobuf.

Is there a drawback to using native.local_repository in the _impl?
If we explicitly use a file-by-file of ctx.symlink(...), would that be any different from setting native.local_repository(..., path = Label("@drake//third_party:com_github_google_protobuf"))?

My hope is that Bazel would make a symlink forest out of the existing files, rather than drop in the directory symlink directly, and thus preserve hermitic builds that we'd normally get with local_repository used directly for the active workspace, and only needing to export the directory.

In this case, we can solve this generally for any local_repository.
(Not that I want this as part of #7713 - per our prior discussions - but it'd still be nice for general and robust inclusion of in-tree third party code.)


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

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


common/proto/BUILD.bazel, line 94 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

FYI Is there any way to expose visibility to a certain macro?
(I've found it annoying that I have expose a utility target as public, even though it'll only ever be used by a macro.)

Not that I am aware, no. I think the only related thing we could do is have a public alias() here (that the macro mentions), and keep the ubsan_fixup's cc_library visibility restricted to //common:__pkg__. In this case I don't think that's any better, but in other situations perhaps.


tools/workspace/com_google_protobuf/package.bzl, line 11 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Is there a drawback to using native.local_repository in the _impl?
If we explicitly use a file-by-file of ctx.symlink(...), would that be any different from setting native.local_repository(..., path = Label("@drake//third_party:com_github_google_protobuf"))?

My hope is that Bazel would make a symlink forest out of the existing files, rather than drop in the directory symlink directly, and thus preserve hermitic builds that we'd normally get with local_repository used directly for the active workspace, and only needing to export the directory.

In this case, we can solve this generally for any local_repository.
(Not that I want this as part of #7713 - per our prior discussions - but it'd still be nice for general and robust inclusion of in-tree third party code.)

I am not sure. My worry indeed was about whether or not changes would be recognized, and I didn't try to test it at all. Probably a good question for bazel-discuss -- if you wanted to whip up an example and ask them "is this sane?".


Comments from Reviewable

Also properly install the ubsan fixup.

Also move the ubsan fixup cc_library out of the com_google_protobuf
repository.  If a downstream Bazel user of Drake were to choose to
use modern com_google_protobuf (not Drake's vendored copy) then
this label would go missing and drake_proto.bzl would have errors.
Instead, we build and install this header as part of Drake.
@jwnimmer-tri
Copy link
Collaborator Author

Review status: 9 of 11 files reviewed at latest revision, 1 unresolved discussion.


tools/workspace/com_google_protobuf/package.bzl, line 11 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure. My worry indeed was about whether or not changes would be recognized, and I didn't try to test it at all. Probably a good question for bazel-discuss -- if you wanted to whip up an example and ask them "is this sane?".

(I wasn't worrying too much about it, because the predominant situation is that our dependencies come from github, not from a vendored copy. This and spruce are the only two exceptions.)


Comments from Reviewable

@soonho-tri
Copy link
Member

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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

Reviewed 3 of 12 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


common/proto/BUILD.bazel, line 94 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Not that I am aware, no. I think the only related thing we could do is have a public alias() here (that the macro mentions), and keep the ubsan_fixup's cc_library visibility restricted to //common:__pkg__. In this case I don't think that's any better, but in other situations perhaps.

OK Thanks!


tools/workspace/com_google_protobuf/package.bzl, line 11 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(I wasn't worrying too much about it, because the predominant situation is that our dependencies come from github, not from a vendored copy. This and spruce are the only two exceptions.)

OK I will test this separate to this PR. Thanks!


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@jwnimmer-tri jwnimmer-tri merged commit a4a8dd5 into RobotLocomotion:master Jan 13, 2018
@jwnimmer-tri jwnimmer-tri deleted the bazel-reuse-workspace-pr9 branch January 13, 2018 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants