Package a subset of Drake in the Bazel build

[david-german-tri]

Contributes to #3129, #1766.

To demo this, do the following from the workspace root on OS X. (Yes, this is awkward! It's designed to be friendly to downstream build systems, and isn't especially considerate of humans.)

mkdir ~/test
bash tools/package_drake.sh ~/test/drake.tar.gz
cd ~/test
tar -xzf drake.tar.gz 
wget https://tinyurl.com/zge7gcs -O main.cc
install_name_tool -id lib/libdrake.so lib/libdrake.so
clang++ main.cc --std=c++14 -I ./include/external/eigen/ -I ./include -L ./lib -l drake
./a.out

On Linux, skip the install_name_tool step, and pass -Wl,-rpath ./lib to the compiler instead.

a.out should print 18.000000.

This change is 

[david-german-tri]

@jwnimmer-tri, this needs quite a bit more polish, but would you be willing to take an early design-review look?

[jwnimmer-tri]

Seems okay. I posed a few details, and once its polished I can post some more.

---

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

---

drake/BUILD, line 5 at r1 (raw file):

 
load("//tools:drake.bzl", "header_tar")

BTW Do we want to explicitly mention BUILD label visibility somewhere here (private)?

---

drake/BUILD, line 12 at r1 (raw file):

    "//drake/multibody/parsers",
    "//drake/systems/framework",
]

FYI One good way I've seen this done before is with delegation -- so here we just say "LIBDRAKE is whatever systems and multibody want" and in systems/BUILD we say "aha, our contribution to LIBDRAKE is x,y,z".

However, since in the endgame there's probably a few LIBDRAKE libraries and not just the one, I think this simpler answer is fine for now.

---

tools/drake.bzl, line 189 at r1 (raw file):

# Collects the transitive closure of header files from ctx.attr.deps.
def _transitive_hdrs_impl(ctx):
  outputs = set()

I'm not sure that outputs is a meaningful variable name here?

---

tools/drake.bzl, line 211 at r1 (raw file):

# Bazel bug, or a bug in these macros.
load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
def header_tar(name, deps=[], **kwargs):

drake_header_tar?

---

tools/package_drake.sh, line 3 at r1 (raw file):

#!/bin/bash
### Builds libdrake.so, and packages it with the required headers.

Use set -e.

---

Comments from Reviewable

[david-german-tri]

@jamiesnape for feature review

[david-german-tri]

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

---

drake/BUILD, line 5 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do we want to explicitly mention BUILD label visibility somewhere here (private)?

Done.

---

drake/BUILD, line 20 at r2 (raw file):

# The Drake binary package. libdrake.so contains all the symbols from all the
# _LIBDRAKE_COMPONENTS and all the Drake externals. We use linkstatic=1 so
# that the binary package will not contain any references to shared libraries

Earlier this week, I'm almost certain that I was able to build with linkstatic = 0, and get Drake symbols, but not external symbols, in libdrake.so. I haven't been able to reproduce that today, but linkstatic = 1 works just fine. The more I mull it over, the more I prefer it anyway.

---

tools/drake.bzl, line 189 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm not sure that outputs is a meaningful variable name here?

Done.

---

tools/drake.bzl, line 211 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

drake_header_tar?

Done.

---

tools/package_drake.sh, line 3 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Use set -e.

Done. (I was trying to figure out what this would do, and killed a terminal in the process. Then I realized.)

---

Comments from Reviewable

[david-german-tri]

Review status: 1 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

drake/BUILD, line 20 at r2 (raw file):

Previously, david-german-tri (David German) wrote…

Earlier this week, I'm almost certain that I was able to build with linkstatic = 0, and get Drake symbols, but not external symbols, in libdrake.so. I haven't been able to reproduce that today, but linkstatic = 1 works just fine. The more I mull it over, the more I prefer it anyway.

Specifically, when I use linkstatic = 0, I'm getting no symbols at all in libdrake.so.

---

Comments from Reviewable

[jamiesnape]

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

---

drake/BUILD, line 20 at r2 (raw file):

Previously, david-german-tri (David German) wrote…

Specifically, when I use linkstatic = 0, I'm getting no symbols at all in libdrake.so.

I definitely recall you building with linkstatic = 0, but I am fine with linkstatic = 1 given the Bazel documentation seems to like it.

---

tools/package_drake.sh, line 27 at r2 (raw file):

cp bazel-bin/drake/libdrake_headers.tar.gz $tmpdir/include/external/scratch
chmod -R 755 $tmpdir

Add a TODO for me to copy drake-config.cmake and drake-targets.cmake into $tmpdir/lib/cmake/drake.

---

Comments from Reviewable

[david-german-tri]

+@rpoyner-tri for additional feature review

---

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

---

Comments from Reviewable

[david-german-tri]

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

---

tools/package_drake.sh, line 27 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Add a TODO for me to copy drake-config.cmake and drake-targets.cmake into $tmpdir/lib/cmake/drake.

Done.

---

Comments from Reviewable

[rpoyner-tri]

Reviewed 1 of 4 files at r1, 3 of 4 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

tools/drake.bzl, line 207 at r3 (raw file):

def drake_header_tar(name, deps=[], **kwargs):

---

tools/package_drake.sh, line 1 at r3 (raw file):

#!/bin/bash

META: this file should be committed with execute permissions.

---

tools/package_drake.sh, line 10 at r3 (raw file):

set -e

mydir=`pwd`

For readability, prefer $() to back-tick for command expansion. for consistency, don't mix them in a single script.

---

tools/package_drake.sh, line 11 at r3 (raw file):

mydir=`pwd`
outfile="$(cd "$(dirname "$1")" && pwd)/$(basename "$1")"

$() expressions do not also need surrounding double quotes.

---

tools/package_drake.sh, line 12 at r3 (raw file):

mydir=`pwd`
outfile="$(cd "$(dirname "$1")" && pwd)/$(basename "$1")"
touch $outfile

expansions of $outfile need double quotes.

---

tools/package_drake.sh, line 13 at r3 (raw file):

outfile="$(cd "$(dirname "$1")" && pwd)/$(basename "$1")"
touch $outfile
cd $mydir

expansions of $mydir need double quotes.

---

tools/package_drake.sh, line 16 at r3 (raw file):

# Build Drake.
workspace=`bazel info workspace`

more back-tick...

---

tools/package_drake.sh, line 17 at r3 (raw file):

# Build Drake.
workspace=`bazel info workspace`
cd $workspace

This variable expansion probably does need double quoting.

---

tools/package_drake.sh, line 22 at r3 (raw file):

# Copy off all the package artifacts into a temporary directory.
# TODO(jamiesnape): Add drake-config.cmake and drake-targets.cmake.
tmpdir=`mktemp -d`

again with the back-ticks, oy...

---

tools/package_drake.sh, line 23 at r3 (raw file):

# TODO(jamiesnape): Add drake-config.cmake and drake-targets.cmake.
tmpdir=`mktemp -d`
mkdir -p $tmpdir/lib

All the expansions of $tmpdir need double quoting. (Having second thoughts about shell vs. python? =P )

---

tools/package_drake.sh, line 30 at r3 (raw file):

# Un-tar the headers. The -P flag and scratch directory are necessary because
# Bazel bakes a .. into the paths of external headers.

BTW, 'tis a pity that we support OSX, because linux tar has --transform which would solve the path problem nicely.

---

tools/package_drake.sh, line 49 at r3 (raw file):

# cd to where we started.
cd $mydir

I think this command is effectively a no-op. When you exit, you abandon a sub-shell and recover the state the prior shell, including its working directory.

---

Comments from Reviewable

[rpoyner-tri]

Does this feature also need some sphinx-ish user documentation?

---

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

---

Comments from Reviewable

[rpoyner-tri]

Also, do we have proper redistribution hygiene for the headers of our externals that are packaged?

---

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

---

Comments from Reviewable

[rpoyner-tri]

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

---

tools/package_drake.sh, line 10 at r3 (raw file):

set -e

mydir=`pwd`

On second read-through, the initialization and use of $mydir in this script is nonsense. Capturing $(pwd) at script invocation is just an arbitrary directory, but line 16, which attempts to find the workspace, will only work if we are already in a subdirectory of the workspace. Fortunately there is a formula that will always locate us within the workspace: $(dirname $0) .

---

Comments from Reviewable

[rpoyner-tri]

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

---

tools/package_drake.sh, line 10 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

On second read-through, the initialization and use of $mydir in this script is nonsense. Capturing $(pwd) at script invocation is just an arbitrary directory, but line 16, which attempts to find the workspace, will only work if we are already in a subdirectory of the workspace. Fortunately there is a formula that will always locate us within the workspace: $(dirname $0) .

better yet $(dirname "$0") -- the inner quotes do matter.

---

Comments from Reviewable

[david-german-tri]

Does this feature also need some sphinx-ish user documentation?

I think not until the contents of the package have been nailed down.

Also, do we have proper redistribution hygiene for the headers of our externals that are packaged?

Are you asking about licensing?

---

Review status: 4 of 5 files reviewed at latest revision, 15 unresolved discussions.

---

tools/drake.bzl, line 207 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

def drake_header_tar(name, deps=[], **kwargs):

I don't understand this comment.

---

tools/package_drake.sh, line 1 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

META: this file should be committed with execute permissions.

Done.

---

tools/package_drake.sh, line 10 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

For readability, prefer $() to back-tick for command expansion. for consistency, don't mix them in a single script.

Done.

---

tools/package_drake.sh, line 10 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

better yet $(dirname "$0") -- the inner quotes do matter.

Done.

---

tools/package_drake.sh, line 11 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

$() expressions do not also need surrounding double quotes.

Done.

---

tools/package_drake.sh, line 12 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

expansions of $outfile need double quotes.

Done.

---

tools/package_drake.sh, line 13 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

expansions of $mydir need double quotes.

Done.

---

tools/package_drake.sh, line 16 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

more back-tick...

Done.

---

tools/package_drake.sh, line 17 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

This variable expansion probably does need double quoting.

Done.

---

tools/package_drake.sh, line 22 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

again with the back-ticks, oy...

Done.

---

tools/package_drake.sh, line 23 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

All the expansions of $tmpdir need double quoting. (Having second thoughts about shell vs. python? =P )

Done.

---

tools/package_drake.sh, line 49 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I think this command is effectively a no-op. When you exit, you abandon a sub-shell and recover the state the prior shell, including its working directory.

Done.

---

Comments from Reviewable

[rpoyner-tri]

yes, licensing. I saw something like eigen/unsupported and it made my spidey-sense tingle. I guess I'll leave that to @jwnimmer-tri to fret over.

---

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

---

tools/package_drake.sh, line 11 at r3 (raw file):

Previously, david-german-tri (David German) wrote…

Done.

BTW, you could drop the outermost double quotes above (since it contains two auto-quoted expansions joined by a non-space), but I'm willing to roll with author's discretion.

---

Comments from Reviewable

[david-german-tri]

I'm mulling what to do about licenses, and hope to post a proposal or updated code shortly.

+@jwnimmer-tri for platform review (and @jamiesnape PTAL)

---

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

---

Comments from Reviewable

[david-german-tri]

I'm mulling what to do about licenses, and hope to post a proposal or updated code shortly.

Done, PTAL.

Re. unsupported/Eigen in particular, it's great to know that we are already building with EIGEN_MPL2_ONLY!

---

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

---

Comments from Reviewable

[jamiesnape]

---

Reviewed 1 of 1 files at r3, 1 of 1 files at r5, 5 of 5 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[david-german-tri]

@drake-jenkins-bot retest this please

[rpoyner-tri]

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

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 4 files at r2, 1 of 1 files at r5, 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):
FYI It would be nice to get CI testing of the final tarball output, but I think that is follow-up work. I guess having Director consume and use it is good, but I wonder if we want to do something more focused? You could imagine a RobotLocomotion sample project that showed how to use Drake as an external, and we could build that in CI? For now even, we could be happy with CI just running this script, testing for exitcode 0, and ensuring that the tarball is non-empty?

---

drake/BUILD, line 11 at r6 (raw file):

# This list is obviously incomplete.
# TODO(david-german-tri, jamiesnape): Add everything Director needs, and
# eventually everything any consumer of Drake would need.

BTW Do we want to remind people here to update the list of packaged licenses below, too?

---

drake/systems/primitives/BUILD, line 18 at r6 (raw file):

        ":constant_value_source",
        ":constant_vector_source",
    ]

I think this list should eventually list all primitives, so for now at least should have a TODO to that effect?

---

tools/drake.bzl, line 205 at r6 (raw file):

# Creates a .tar.gz that includes all the headers exported by
# the libraries in deps.

BTW We have been using python docstrings (within the function body) for API comments like the above. I'm not sure it matter much, though?

---

tools/drake.bzl, line 217 at r6 (raw file):

          extension="tar.gz",
          files=[":" + name + "_gather"],
          strip_prefix="/")

BTW Was there any voodoo for why we need "/" here that needs to be written down?

---

tools/package_drake.sh, line 11 at r6 (raw file):

mydir=$(dirname "$0")
outfile="$(cd $(dirname "$1") && pwd)/$(basename "$1")"

This does bad things if $1 is empty.
[ -n "$1" ] || (echo "error: missing argument" && false) is an approximate fix.

---

Comments from Reviewable

[david-german-tri]

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

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI It would be nice to get CI testing of the final tarball output, but I think that is follow-up work. I guess having Director consume and use it is good, but I wonder if we want to do something more focused? You could imagine a RobotLocomotion sample project that showed how to use Drake as an external, and we could build that in CI? For now even, we could be happy with CI just running this script, testing for exitcode 0, and ensuring that the tarball is non-empty?

I like the sample project idea a lot.

---

drake/BUILD, line 11 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do we want to remind people here to update the list of packaged licenses below, too?

Done.

---

drake/systems/primitives/BUILD, line 18 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this list should eventually list all primitives, so for now at least should have a TODO to that effect?

Done.

---

tools/drake.bzl, line 205 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW We have been using python docstrings (within the function body) for API comments like the above. I'm not sure it matter much, though?

Done.

---

tools/drake.bzl, line 217 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Was there any voodoo for why we need "/" here that needs to be written down?

Done.

---

tools/package_drake.sh, line 11 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This does bad things if $1 is empty.
[ -n "$1" ] || (echo "error: missing argument" && false) is an approximate fix.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

---

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

---

Comments from Reviewable