Teach drake bazel rules to play nicely when drake is an external bazel project #6190

EricCousineau-TRI · 2017-05-26T15:28:07Z

This change would permit drake to be used as an external in a Bazel project, and would permit drake's tools/... to be usable outside of drake.

This follows the workaround listed here.

~~Move all non-rule externals to @drake//tools:externals.bzl~~
Replace @// with @drake//
Replace tools/*.BUILD with @drake//tools:*.BUILD

Worked with @stonier to whip up an example project:

drake_external/
- This consumes @drake//drake/automotive:curve2 @drake//drake/solvers:mathematical_program externally

This change is

EricCousineau-TRI · 2017-05-26T15:29:38Z

+@jwnimmer-tri for feature review, please.
+@sherm1 for platform review, please.

Comments from Reviewable

jamiesnape · 2017-05-26T16:11:16Z

Maybe in future for fundamental changes like this that are going to create a lot of conflicts, send out some advanced warning.

jwnimmer-tri · 2017-05-26T17:07:00Z

a discussion (no related file):
Sorry, I should have opened a discussion thread:

Can we trickle this in, maybe? Just do the @// to @drake// rename in the BUILD file citations in a first PR?

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:07:37Z

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

a discussion (no related file):
I'm also curious to know what external project we are targeting. I am hesitant to try to support anything like this until and unless we add Drake CI support for a downstream project using us in this fashion.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T17:19:12Z

@jamiesnape Sorry about that, this seemed like a relatively small change that would enable devs / external users to easily consume drake as an external in Bazel. If this inhibits / conflicts with someone else's PR in flight, we could always block this one until the others go in, and let that serve as the warning.
That being said, I would like to get this in, as it would make things a little simpler for prototyping work.

Are there any PRs in particular that you are concerned about? VTK and install script stuff?

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Sorry, I should have opened a discussion thread:

Can we trickle this in, maybe? Just do the @// to @drake// rename in the BUILD file citations in a first PR?

Yup, shall do.

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm also curious to know what external project we are targeting. I am hesitant to try to support anything like this until and unless we add Drake CI support for a downstream project using us in this fashion.

Right now, I am thinking of my own projects for one-off prototypes to inform design decisions.

Could we not use a dummy external in CI to get this workflow started as soon as possible?
(It could also be used to test CMake / pkg-config / cps integration as well.)

And this is not critical to be in master, but if it were not to make it into master at this point, I would probably maintain my own devel branch and merge things into it so I could use this setup, as it is quite useful to me.

Comments from Reviewable

sherm1 · 2017-05-26T17:19:49Z

Sorry, @EricCousineau-TRI -- this is too far outside my area of expertise to be an effective platform reviewer for it.
-@sherm1

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

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:21:08Z

I will wear the platform hat; just fine a second for feature review too, please.

Comments from Reviewable

* Replace `@//` with `@drake//` * Replace `tools/*.BUILD` with `@drake//tools:*.BUILD`

EricCousineau-TRI · 2017-05-26T17:26:54Z

@sherm1 @jwnimmer-tri sounds good!

+@jamiesnape for feature review, if you are available?
If this messes up any PRs in flight for VTK / install, could you just block on this PR?

Comments from Reviewable

jamiesnape · 2017-05-26T17:28:45Z

I am. No need to block if this makes your life easier.

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

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:29:06Z

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Right now, I am thinking of my own projects for one-off prototypes to inform design decisions.

Could we not use a dummy external in CI to get this workflow started as soon as possible?
(It could also be used to test CMake / pkg-config / cps integration as well.)

And this is not critical to be in master, but if it were not to make it into master at this point, I would probably maintain my own devel branch and merge things into it so I could use this setup, as it is quite useful to me.

I definitely agree this moves us toward a better long-term goal. I am fine for a small dummy in CI to prove this works. (I didn't mean to imply that Spartan or something big or difficult should go into our CI).

I would just ask to see the dummy stood up soonish, secured onto this PR train. If it were prototyped such that I could test it locally outside of CI while reviewing the WORKSPACE part of the next PR, all the better. It sound like you have a small downstream project working locally; getting (a cut-down version of that) into RobotLocomotion would be excellent.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T17:32:03Z

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I definitely agree this moves us toward a better long-term goal. I am fine for a small dummy in CI to prove this works. (I didn't mean to imply that Spartan or something big or difficult should go into our CI).

I would just ask to see the dummy stood up soonish, secured onto this PR train. If it were prototyped such that I could test it locally outside of CI while reviewing the WORKSPACE part of the next PR, all the better. It sound like you have a small downstream project working locally; getting (a cut-down version of that) into RobotLocomotion would be excellent.

Awesome. I actually have a simple external based here:
https://github.com/EricCousineau-TRI/drake_external#testing

It has a minimal set of bash commands to clone the repository, and build what is needed.
It could be extended to use whatever the Bazel-only dependencies of install is, such that everything exposed externally is built.

Would that work?

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T17:36:27Z

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

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Awesome. I actually have a simple external based here:
https://github.com/EricCousineau-TRI/drake_external#testing

It has a minimal set of bash commands to clone the repository, and build what is needed.
It could be extended to use whatever the Bazel-only dependencies of install is, such that everything exposed externally is built.

Would that work?

Using Matt's //drake:install dependency, getting some failures in the external. Will see if I can fix that.

Comments from Reviewable

jamiesnape · 2017-05-26T17:39:15Z

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

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Using Matt's //drake:install dependency, getting some failures in the external. Will see if I can fix that.

A RobotLocomotion/drake-examples repo that Jenkins just clones and then runs bazel test ... and mkdir build && cmake .. && make && ctest . would work for me. It would be easy to run on continuous.

Comments from Reviewable

stonier · 2017-05-26T17:41:39Z

That being said, I would like to get this in, as it would make things a little simpler for prototyping work.

Similar - I started doing this (i.e. a bazel workspace chained drake) just to write some simple drake experiments to understand the code and learn a bit of bazel free of all the drake noise. That's turned into unofficially starting work on a canonical set of simple user projects (shambhala) that can be a template (and possibly a CI test later) for others.

This use case should be both the simplest, and a very standard use case. I think it's a good baseline requirement to help drive conventions for our bazel layout in way that helps users (and that requirement won't change). i.e. it's not a parallel support case and better now than a major refactor later.

Aside from the dependencies, be nice to also have access to some of the other bazel infra drake is building - e.g. github.bzl, bitbucket.bzl, install.bzl.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:41:50Z

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

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

A RobotLocomotion/drake-examples repo that Jenkins just clones and then runs bazel test ... and mkdir build && cmake .. && make && ctest . would work for me. It would be easy to run on continuous.

Yes, it has to be an RL repository not personal repo.

We've talked about writing up canonical examples of how to integrate Drake into downstream projects. I think we should probably have one CMake example and one Bazel-only example, completely separate, and that this PR is towards the latter (Bazel-only). So RobotLocomotion/drake-example-bazel maybe?

I don't think it needs to do bazel run -- :install; just having some sample C++ code that deps on Drake libraries should be good enough for now.

a discussion (no related file):
Please take a look at the bzl files in ./third_party/kythe/tools/build_rules/config; I think a few of them also need a @drake// change.

tools/gtest.BUILD, line 34 at r1 (raw file):

    linkopts = select({
        "@drake//tools:linux": ["-pthread"],
        "@//conditions:default": [],

Meta Could you explain why the @//conditions:default doesn't turn into @drake//conditions:default? (Same for spdlog.)

Comments from Reviewable

jamiesnape · 2017-05-26T17:45:35Z

Looks cool. Just let me know if/when you want a Jenkins job setting up. Should be pretty easy.

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

Comments from Reviewable

stonier · 2017-05-26T17:47:03Z

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, it has to be an RL repository not personal repo.

We've talked about writing up canonical examples of how to integrate Drake into downstream projects. I think we should probably have one CMake example and one Bazel-only example, completely separate, and that this PR is towards the latter (Bazel-only). So RobotLocomotion/drake-example-bazel maybe?

I don't think it needs to do bazel run -- :install; just having some sample C++ code that deps on Drake libraries should be good enough for now.

I wasn't intending to drop shambhala into RobotLocomotion and CI just yet, but that was the goal.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T17:49:28Z

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Please take a look at the bzl files in ./third_party/kythe/tools/build_rules/config; I think a few of them also need a @drake// change.

Looking at that now - I feel a little dirty dropping @drake// in a third party, even if it is relatively small.
I will see if I can proxy pkg_config_package to explicitly specify build_file_template as @drake//....

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:51:04Z

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Looking at that now - I feel a little dirty dropping @drake// in a third party, even if it is relatively small.
I will see if I can proxy pkg_config_package to explicitly specify build_file_template as @drake//....

If you git blame those lines, IIRC you'll see I've already had to fork them to change their paths for Drake's use.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T17:53:31Z

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If you git blame those lines, IIRC you'll see I've already had to fork them to change their paths for Drake's use.

Oh lol, well, then I'll add that in.

However, it seems that we can provide an upstream update to kythe in much the same way - make its paths robust against being used as an external.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T17:56:10Z

I've pulled and tested this locally with TRI-internal users of Drake, and all seems fine.

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

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Oh lol, well, then I'll add that in.

However, it seems that we can provide an upstream update to kythe in much the same way - make its paths robust against being used as an external.

Fine by me. I was like "meh" at the time.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T18:52:56Z

Review status: 11 of 19 files reviewed at latest revision, 3 unresolved discussions.

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Fine by me. I was like "meh" at the time.

Hmm... Just successfully ported kythe to its own package, but now it makes me kinda meh as well.
Let me know if you'd like me to roll that change back.

I've updated the example to consume @drake//drake:libdrake.so (as data), and it seems to work out, at least the for the targets that it consumes:
EricCousineau-TRI/drake_external@2e6e75a

tools/gtest.BUILD, line 34 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta Could you explain why the @//conditions:default doesn't turn into @drake//conditions:default? (Same for spdlog.)

Done.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T19:12:27Z

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

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

Hmm... Just successfully ported kythe to its own package, but now it makes me kinda meh as well.
Let me know if you'd like me to roll that change back.

I've updated the example to consume @drake//drake:libdrake.so (as data), and it seems to work out, at least the for the targets that it consumes:
EricCousineau-TRI/drake_external@2e6e75a

OK The kythe thing is fine.

FYI I think the Bazel example we end up putting in RobotLocomotion should not use libdrake.so but rather the regular Bazel cc_library stuff via labels.

WORKSPACE, line 19 at r2 (raw file):

workspace(name = "drake")

BTW Is this two blank lines now? Seems excessive.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T19:17:23Z

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK The kythe thing is fine.

FYI I think the Bazel example we end up putting in RobotLocomotion should not use libdrake.so but rather the regular Bazel cc_library stuff via labels.

Sounds good!

WORKSPACE, line 19 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Is this two blank lines now? Seems excessive.

Done.

Comments from Reviewable

jamiesnape · 2017-05-26T19:17:25Z

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK The kythe thing is fine.

FYI I think the Bazel example we end up putting in RobotLocomotion should not use libdrake.so but rather the regular Bazel cc_library stuff via labels.

A cc_library(name = "drake") or all the component libraries?

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T19:21:46Z

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

A cc_library(name = "drake") or all the component libraries?

Seems like a good idea to me! Threw it in here for grins, but can defer that to some other PR.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T19:22:24Z

Reviewed 1 of 1 files at r3.
Review status: 19 of 20 files reviewed at latest revision, 3 unresolved discussions.

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

A cc_library(name = "drake") or all the component libraries?

FYI I think doing an example like this would be best:

cc_binary(
    ....
    deps = [
        "@drake//drake/systems/analysis",
        "@drake//drake/systems/framework",
        "@drake//drake/systems/primitives:constant_value_source",
    ],
)

So using per-directory rollup libraries where appropriate, and small libraries otherwise.

drake/BUILD, line 27 at r4 (raw file):

    name = "drake",
    deps = _LIBDRAKE_COMPONENTS,
)

Nope. The uberlibrary should only be for CMake export / binary installation. It should not be a public Bazel export.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T19:27:36Z

Review status: 19 of 20 files reviewed at latest revision, 3 unresolved discussions.

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I think doing an example like this would be best:
cc_binary(
    ....
    deps = [
        "@drake//drake/systems/analysis",
        "@drake//drake/systems/framework",
        "@drake//drake/systems/primitives:constant_value_source",
    ],
)
So using per-directory rollup libraries where appropriate, and small libraries otherwise.

True, for external projects, but for the test, would it be more useful to include the megalib?

Added an example of Jamie's suggestion on the downstream thing (adding //drake/solvers):
EricCousineau-TRI/drake_external@07b0e28

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T19:29:25Z

drake/BUILD, line 27 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Nope. The uberlibrary should only be for CMake export / binary installation. It should not be a public Bazel export.

For external testing with Bazel, it'd still be nice to get to all of the projects, and not have to sync it.

Mayhaps, in something like //tools/library.bzl, we can define something like libdrake_components(), such that they could be imported, via:

load("@drake//tools:libdrake.bzl", "libdrake_components")

Comments from Reviewable

jamiesnape · 2017-05-26T19:34:42Z

Review status: 19 of 20 files reviewed at latest revision, 3 unresolved discussions.

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

True, for external projects, but for the test, would it be more useful to include the megalib?

Added an example of Jamie's suggestion on the downstream thing (adding //drake/solvers):
EricCousineau-TRI/drake_external@07b0e28

We should be exposing the same interface regardless of the build system someone is using to consume it.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T20:07:01Z

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

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

We should be exposing the same interface regardless of the build system someone is using to consume it.

Let's have that debate somewhere else? libdrake.so is not the final answer, so I don't want to export it publicly from Bazel.

Comments from Reviewable

jamiesnape · 2017-05-26T20:11:02Z

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Let's have that debate somewhere else? libdrake.so is not the final answer, so I don't want to export it publicly from Bazel.

Ok.

Comments from Reviewable

EricCousineau-TRI · 2017-05-26T20:11:43Z

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

drake/BUILD, line 27 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

For external testing with Bazel, it'd still be nice to get to all of the projects, and not have to sync it.

Mayhaps, in something like //tools/library.bzl, we can define something like libdrake_components(), such that they could be imported, via:
load("@drake//tools:libdrake.bzl", "libdrake_components")

Reverted. Will create a separate issue on this.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T20:18:16Z

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

Comments from Reviewable

jamiesnape · 2017-05-26T20:30:06Z

Reviewed 7 of 8 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

jwnimmer-tri · 2017-05-26T23:13:55Z

"unstable due to git failures" flake
@drake-jenkins-bot linux-xenial-gcc-ninja-experimental please

jwnimmer-tri · 2017-05-30T21:30:41Z

FYI I just dismissed @stonier's blocking discussion comment, which I presume was accidental.

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

Comments from Reviewable

EricCousineau-TRI · 2017-05-30T21:48:29Z

Awesome, thanks!

EricCousineau-TRI assigned jwnimmer-tri and sherm1 May 26, 2017

EricCousineau-TRI force-pushed the feature/sep_deps_rebase branch from f3f3c26 to 3997715 Compare May 26, 2017 17:12

sherm1 removed their assignment May 26, 2017

Start enabling drake to be used as an external Bazel project:

9d16d99

* Replace `@//` with `@drake//` * Replace `tools/*.BUILD` with `@drake//tools:*.BUILD`

EricCousineau-TRI force-pushed the feature/sep_deps_rebase branch from 3997715 to 9d16d99 Compare May 26, 2017 17:23

EricCousineau-TRI assigned jamiesnape May 26, 2017

Add comments

24ed823

EricCousineau-TRI force-pushed the feature/sep_deps_rebase branch from 9b75766 to 22d163d Compare May 26, 2017 19:16

EricCousineau-TRI force-pushed the feature/sep_deps_rebase branch from 14029d5 to 22d163d Compare May 26, 2017 20:05

Make kythe a separate repository.

13776ad

EricCousineau-TRI force-pushed the feature/sep_deps_rebase branch from 22d163d to 13776ad Compare May 26, 2017 20:08

EricCousineau-TRI changed the title ~~Enable drake to be used as an external Bazel project~~ Teah drake bazel rules to play nicely when drake is an external bazel project May 26, 2017

EricCousineau-TRI changed the title ~~Teah drake bazel rules to play nicely when drake is an external bazel project~~ Teach drake bazel rules to play nicely when drake is an external bazel project May 26, 2017

EricCousineau-TRI mentioned this pull request May 26, 2017

Enable drake to be consumed as a bazel external (as a local and remote repository) #6193

Closed

5 tasks

EricCousineau-TRI mentioned this pull request May 27, 2017

[Waiting for #6190] Encapsulate all dependencies into tools/externals*.bzl. #6195

Closed

jwnimmer-tri mentioned this pull request May 30, 2017

Install more external targets from the Bazel build #6201

Merged

EricCousineau-TRI merged commit cd25384 into RobotLocomotion:master May 30, 2017

jamiesnape removed their assignment Jun 22, 2021

Teach drake bazel rules to play nicely when drake is an external bazel project #6190

Teach drake bazel rules to play nicely when drake is an external bazel project #6190

Conversation

EricCousineau-TRI commented May 26, 2017 • edited Loading

EricCousineau-TRI commented May 26, 2017

jamiesnape commented May 26, 2017

jwnimmer-tri commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

sherm1 commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jamiesnape commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jamiesnape commented May 26, 2017

stonier commented May 26, 2017

jwnimmer-tri commented May 26, 2017

jamiesnape commented May 26, 2017

stonier commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jamiesnape commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jwnimmer-tri commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jamiesnape commented May 26, 2017

jwnimmer-tri commented May 26, 2017

jamiesnape commented May 26, 2017

EricCousineau-TRI commented May 26, 2017

jwnimmer-tri commented May 26, 2017

jamiesnape commented May 26, 2017

jwnimmer-tri commented May 26, 2017

jwnimmer-tri commented May 30, 2017

EricCousineau-TRI commented May 30, 2017

EricCousineau-TRI commented May 26, 2017 •

edited

Loading