Adds Bazel macros, Python CLI, and basic workflow tests.

[EricCousineau-TRI]

An unfortunately large initial PR.

Per f2f with @sammy-tri, will work on adding in Travis CI and linting.

---

This change is 

[EricCousineau-TRI]

Starting review on GitHub; will shift to Reviewable once it's connected.

[EricCousineau-TRI]

+@sammy-tri for feature review, please.

---

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

---

Comments from Reviewable

[sammy-tri]

Checkpoint, slowly working my way through. (apologies for the delay)

---

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

---

README.md, line 3 at r1 (raw file):

# About

This is a Bazel-focused implementation for incorporating external data into the workspace (not to be versioned in Git, etc.) for testing and running binaries.

BTW perhaps this could be a bit more clear? I feel like the goal is that there is versioning for the file provided by the hash, but the data is not stored in git.

---

docs/setup.md, line 5 at r1 (raw file):

## Prerequisites

For a basic client, you must base `python` and the `girder_client` installed.

I'm not sure what "you must base" means..

---

docs/setup.md, line 33 at r1 (raw file):

* Inspect the configuration examples in `docs/config`:
    * `external_data.user.yml` - Copy this to `~/.config/bazel_external_data/config.yml`

Is it possible (or already the case) to make this optional if the user isn't going to upload? (my motivation here is so that something like git clone https://RobotLocomotion/drake.git && cd drake && sudo ./setup/ubuntu/16.04/install_prereqs.sh && bazel build //... will "just work" without any additional configuration (as I think is the case now).)

---

docs/workflows.md, line 14 at r1 (raw file):

        http_archive(
            name = "bazel_external_data_pkg",
            url = "https://github.com/EricCousineau-TRI/bazel_external_data/archive/ce71398ac319cf4dedcf3eed3ae5c2c25e4eb1b4.zip",

Perhaps update the repo from EricCousineau-TRI to RobotLocomotion?

---

docs/workflows.md, line 34 at r1 (raw file):

3. Under `${workspace}/tools`:

    1. Add `BUILD` (to enable it as a package).

I'm not sure I'd be able to follow this instruction. Does this just mean "make sure there's a BUILD (or BUILD.bazel) file present in the tools directory at all?

---

docs/workflows.md, line 70 at r1 (raw file):

        load("//tools:external_data.bzl",
            "external_data",
            "external_data_group",

Do you actually need to load all three of these if you're only calling external_data?

---

docs/workflows.md, line 102 at r1 (raw file):

        cd data
        touch dragon.obj

Why is the 'touch' needed?

---

docs/workflows.md, line 117 at r1 (raw file):

3. To test if Bazel can download this file, execute this in `:/data`:

        bazel build :dragon.obj

Where does this file wind up after this operation? bazel-bin somewhere? (I know I tested this workflow so I probably knew the answer once, but I can't remember)

---

tools/macros.bzl, line 62 at r1 (raw file):

        'no_cache' - Download the file, do not use the cache.
    @param settings
        Settings for the given repository (or even individual target).

Wouldn't any settings passed to external_data be for an individual target by definition? Or have I misunderstood something?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 10 of 72 files reviewed at latest revision, 9 unresolved discussions.

---

README.md, line 3 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW perhaps this could be a bit more clear? I feel like the goal is that there is versioning for the file provided by the hash, but the data is not stored in git.

Done.

---

docs/setup.md, line 5 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I'm not sure what "you must base" means..

Done. (Also, removed Girder stuff - will add that in subsequent PR, once base structure is good to go.)

---

docs/setup.md, line 33 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is it possible (or already the case) to make this optional if the user isn't going to upload? (my motivation here is so that something like git clone https://RobotLocomotion/drake.git && cd drake && sudo ./setup/ubuntu/16.04/install_prereqs.sh && bazel build //... will "just work" without any additional configuration (as I think is the case now).)

Done. This is mentioned below, and I will ensure that this is part of a Girder test.

---

docs/workflows.md, line 14 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Perhaps update the repo from EricCousineau-TRI to RobotLocomotion?

Done.

---

docs/workflows.md, line 34 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I'm not sure I'd be able to follow this instruction. Does this just mean "make sure there's a BUILD (or BUILD.bazel) file present in the tools directory at all?

Done.

---

docs/workflows.md, line 70 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Do you actually need to load all three of these if you're only calling external_data?

Done.

---

docs/workflows.md, line 102 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Why is the 'touch' needed?

Done.

---

docs/workflows.md, line 117 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Where does this file wind up after this operation? bazel-bin somewhere? (I know I tested this workflow so I probably knew the answer once, but I can't remember)

Done.

---

tools/macros.bzl, line 62 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Wouldn't any settings passed to external_data be for an individual target by definition? Or have I misunderstood something?

Done.

---

Comments from Reviewable

[sammy-tri]

commit some more comments I forgot to submit before break

---

Reviewed 2 of 73 files at r1.
Review status: 12 of 72 files reviewed at latest revision, 12 unresolved discussions.

---

src/bazel_external_data/BUILD.bazel, line 1 at r1 (raw file):

# @note 'core' has no backends present.

Putting # -*- python -*- on the first line of the BUILD.bazel files (as drake does) would be convenient for those whose editors recognize that tag

---

src/bazel_external_data/BUILD.bazel, line 10 at r1 (raw file):

        "hashes.py",
    ],
    imports = [".."],

This looks strange enough that it probably deserves a comment.

---

src/bazel_external_data/config_helpers.py, line 15 at r1 (raw file):

def guess_start_dir(filepath):

It's not obvious why a function with this behavior has the name guess_start_dir

---

src/bazel_external_data/util.py, line 75 at r1 (raw file):

    cur_dir = start_dir
    assert len(cur_dir) > 0
    for i in xrange(max_depth):

BTW I don't think our style guide mandates this, but I personally like using _ (as opposed to i here) to indicate that I'm deliberately declaring an unused variable in python. Alternately a comment that i is deliberately unused might be nice

---

Comments from Reviewable

[sammy-tri]

Reviewed 4 of 5 files at r2.
Review status: 16 of 72 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 12 of 72 files reviewed at latest revision, 4 unresolved discussions.

---

src/bazel_external_data/BUILD.bazel, line 1 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Putting # -*- python -*- on the first line of the BUILD.bazel files (as drake does) would be convenient for those whose editors recognize that tag

Done.

---

src/bazel_external_data/BUILD.bazel, line 10 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

This looks strange enough that it probably deserves a comment.

Done.

---

src/bazel_external_data/config_helpers.py, line 15 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

It's not obvious why a function with this behavior has the name guess_start_dir

Done.

---

src/bazel_external_data/util.py, line 75 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW I don't think our style guide mandates this, but I personally like using _ (as opposed to i here) to indicate that I'm deliberately declaring an unused variable in python. Alternately a comment that i is deliberately unused might be nice

Done.

---

Comments from Reviewable

[sammy-tri]

Still working through, sorry for how long this is taking me (I've needed a few breaks). Is this the right place to have the conversation about this code being standalone vs. living inside drake?

---

Reviewed 10 of 73 files at r1, 7 of 14 files at r3.
Review status: 29 of 72 files reviewed at latest revision, 10 unresolved discussions.

---

src/bazel_external_data/check.py, line 3 at r3 (raw file):

"""
@file
Allows uploading data file to a remote.

Is this file level comment accurate?

---

src/bazel_external_data/cli.py, line 38 at r3 (raw file):

args = parser.parse_args()

# Do not allow running under Bazel unless we have a guess for the project root from an input file.

long line (one of many in this file)

---

src/bazel_external_data/core.py, line 8 at r3 (raw file):

PROJECT_CONFIG_FILE = ".external_data.yml"
USER_CONFIG_FILE_DEFAULT = os.path.expanduser("~/.config/bazel_external_data/config.yml")

https://www.python.org/dev/peps/pep-0008/#maximum-line-length

---

src/bazel_external_data/download.py, line 1 at r3 (raw file):

from __future__ import absolute_import, print_function

Are either of these used?

---

src/bazel_external_data/download.py, line 2 at r3 (raw file):

from __future__ import absolute_import, print_function
import argparse

unused

---

src/bazel_external_data/hashes.py, line 5 at r3 (raw file):

class HashType(object):

BTW consider naming _HashType if it's not used outside of this file?

---

src/bazel_external_data/hashes.py, line 116 at r3 (raw file):

sha512 = Sha512()

if __name__ == "__main__":

Is this code automatically invoked anywhere as a test or something? My inclination is to comment why it's here or just delete it if it's never executed to avoid bitrot.

---

src/bazel_external_data/upload.py, line 1 at r3 (raw file):

from __future__ import absolute_import, print_function

unused?

---

src/bazel_external_data/upload.py, line 5 at r3 (raw file):

import os
import sys
import argparse

unused

---

src/bazel_external_data/upload.py, line 9 at r3 (raw file):

from bazel_external_data import core, util

BTW I think both this file anddownload.py would benefit from a file level comment explaining what their purpose is

---

Comments from Reviewable

[EricCousineau-TRI]

Yes, I think it would be.
Per our f2f, I was going to move this into tools/third_party most likely, as I want it to be a separate workspace still.

The only remaining item would be to, at some point, ensure that unittests using Girder via docker can be included; Jeremy's mentioned that in RobotLocomotion/drake#7654.

---

Review status: 29 of 72 files reviewed at latest revision, 10 unresolved discussions.

---

Comments from Reviewable

[sammy-tri]

Reviewed 1 of 73 files at r1, 1 of 14 files at r3.
Review status: 31 of 72 files reviewed at latest revision, 10 unresolved discussions.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 25 of 72 files reviewed at latest revision, 10 unresolved discussions.

---

src/bazel_external_data/check.py, line 3 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is this file level comment accurate?

Done.

---

src/bazel_external_data/cli.py, line 38 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

long line (one of many in this file)

Done.

---

src/bazel_external_data/core.py, line 8 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

https://www.python.org/dev/peps/pep-0008/#maximum-line-length

Done.

---

src/bazel_external_data/download.py, line 1 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Are either of these used?

Done.

---

src/bazel_external_data/download.py, line 2 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused

Done.

---

src/bazel_external_data/hashes.py, line 5 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW consider naming _HashType if it's not used outside of this file?

Done.

---

src/bazel_external_data/hashes.py, line 116 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is this code automatically invoked anywhere as a test or something? My inclination is to comment why it's here or just delete it if it's never executed to avoid bitrot.

Done. Removed, it was just a little test as I was prototyping.

---

src/bazel_external_data/upload.py, line 1 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

src/bazel_external_data/upload.py, line 5 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused

Done.

---

src/bazel_external_data/upload.py, line 9 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW I think both this file anddownload.py would benefit from a file level comment explaining what their purpose is

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

Closing for now.