external_data: Add stub of Bazel structure for isolated and meta-testing.

[EricCousineau-TRI]

This change is 

[EricCousineau-TRI]

The end goal (pre-Girder) is here: https://github.com/EricCousineau-TRI/drake/tree/30c757eb9c003fc57ffddaca59632e633622cdf4/tools/external_data/workspace/test/workspaces
The end goal (with Girder) is here: https://github.com/EricCousineau-TRI/external_data_bazel/tree/1f3e4918a133ace96ce884e02d4e64fbed2c8117/src/external_data_bazel/backends

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

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Note that tools/external_data/README.md gives a high-level overview for this PR.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Related #7259.

I did some hack-and-slash for lint dependencies. I only needed Bazel + Python stuff, but could still drag in cpp_lint since no dependencies are triggered in the Bazel code paths in external_data/workspace.

[EricCousineau-TRI]

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

---

tools/external_data/BUILD.bazel, line 27 at r1 (raw file):

        "workspace_symlink_forest",
        # Get necessary upstream files from Drake.
        "//tools:external_data_meta_testing_files",

BTW This is more like isolated testing rather than meta testing.
However, since meta testing kinda applied in bazel_external_data_pkg, I just kept it.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

tools/external_data/BUILD.bazel, line 16 at r1 (raw file):

# Run all tests in `workspace` as a separate package.
sh_test(
    name = "workspace_test",

Ultimately, it'd be a lot nicer if I could somehow bind bazel test @bazel_external_data_pkg//... to a test that is discoverable from bazel test @drake//..., but I have no idea how to make that work.

Unfortunately, alias (a) does not allow binding test_suites, and (b) does not actually seem to produce a physical target (such that it would not be discoverable), but I guess that's by definition:
https://docs.bazel.build/versions/master/be/general.html#alias

I will add that as a comment here, once I give CI a chance to complete these tests.

---

Comments from Reviewable

[EricCousineau-TRI]

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

[jwnimmer-tri]

This all seems very confusing. I am not even understanding what the end goal or game is, and the links above didn't help me.

I guess in the first place, why do we need to local_repository(name = "bazel_external_data_pkg", ...) pointing to TRI-authored code within drake's source tree? (Unlike Drake's third_party vendor'd code cited locally by WORKSPACE, where keeping it in a separate @repos is useful for as a dependency seam, I don't see the rationale here.) Why is the Drake workspace insufficient? I see some comments about "make it easier to develop independently" but I don't understand how that plays out, or why it would hold true for the other dozens of Drake developers who have to learn and understand this, versus something convenient for spike-test local development by one person.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Sorry about that!

The end-goal of this PR is to support automated testing of Bazel-based workflows here:
https://github.com/RobotLocomotion/bazel-external-data/blob/1478c5525279659b9a77a3898ff8881d305539bb/test/workspaces/bazel_pkg_advanced_test/workflows_test.sh#L19

Why is the Drake workspace insufficient?

The reason to place this code in a nested workspace is to drastically simplify the process of automating these tests in a clean, Bazel-friendly process. Drake would make it harder to make reproducible, isolated tests to ensure that the user / developer experience goes as desired.
Testing this behavior using Bazel is something I strongly desire to have under an automated test, both on local developer machines (for debugging) and on CI.

From my standpoint, w.r.t. the functionality contained in this module, there is very little to gain by having it next to Drake or pull in dependencies from it; the linting is done by Drake itself (per the latest push), and things like installation seems like a moot point, since this is purely a development tool.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Per f2f with @jwnimmer-tri, I will try merging bazel_external_data with drake, such that only the test directories are (useful) nested repositories, but aiming towards only need @drake and no other dependencies.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

@jwnimmer-tri I've tried out the more hermitic structure, folding the workspace into Drake, and got it to work... But I strongly dislike this setup because I feel like we're trading maintainability for consistency.

Because Bazel has no apparent ease for introspection (e.g. "give me all Skylark files that are needed to make this *.bzl file run"), we have to manually dictate transitive dependencies for things that we ultimately don't care about.
Additionally, because we list the transitive Skylark dependencies as data, that means any change (like adding a comment) will re-trigger this test.
Another minor nit is that in this setup, running bazel test --config lint ... will miss these lint tests. Of course, I could mock additional tests with the appropriate tags to propagate, but then that adds extra overhead that seems to give little return.

I'd prefer to keep it as it previously was (with the mid-level workspace - but with directory names a little more clearly named), with a mention of why it is broken up. If there are issues with importing the external data macros with it as a local repo in Drake, I'll gladly take the responsibility of solving that problem myself if it comes up.

---

Review status: 0 of 24 files reviewed at latest revision, 3 unresolved discussions.

---

tools/external_data/README.md, line 15 at r2 (raw file):

The structure:

*   `workspace/` - The local workspace for `bazel_external_data_pkg`.

This is out of date; will update once a direction is confirmed for Bazel structure.

---

Comments from Reviewable

[EricCousineau-TRI]

(And I'd still like to maintain linting on the individual test files, as there will be some non-trivial code in there - so I do want some consistency, just not at the bazel BUILD file structure.)

---

Review status: 0 of 24 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

It that "more hermetic structure" pushed to this PR (or is there a link)? I will definitely look tonight.

If I'm understanding correctly: I think we are on the same page that all files that are required by the test project (i.e., all data used by the test) has to be explicitly listed (or globbed) -- that we can't rely on the "its containing directory got symlinked in as part of a filegroup" trick? And thus the question is, depending on how the external_data tool and workspace and dependencies are phrased, either more or less of Drake's skylark code and tools end up being dependencies of external_data (and thus its tests), and thus affect the magnitude of how much data we need to list as prereqs of the test? Is that approximately right?

---

Comments from Reviewable

[EricCousineau-TRI]

Yup, the "more hermetic structure" is in r3 (3840cb5).

Is that approximately right?

Yup! Very succinctly stated.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

FYI I'm still exploring this locally and gaining understanding. There are a few facets that I plan to write up. I don't have a conclusion yet but working on it actively and should have an answer soonish.

One note so far... I can foresee that #7259 work would come into play because it expands the list of files required to be present in order to parse Drake's WORKSPACE file (from the test case). Right now the WORKSPACE directly lists many e.g. github_archive entries; but with my work, each of those will be broken out into a helper file. So rather than the ~3 workspace entries we have to glob up right now, we'd need all ~50 entries, since load statements are never lazily evaluated.

---

Comments from Reviewable

[jwnimmer-tri]

Here is a condensed version of my thoughts. Probably we should discuss live as a next step.

So I've thought about about acceptance criteria for what would be satisfactory. Within these, there are likely several designs that could all work.

1. bazel test //... runs all tests (an earlier discussion you posted below hinted that might not be the case with the sub-workspace approach?);
2. bazel test --config lint //... does cover the external_data implementation itself, but its okay if the acceptance test data itself (i.e., the test workspaces) are not linted as part of this shortcut;
3. All test case dependencies must be real files; never any symlinked-directory cheats.
4. If the external_data implementation is in its own sub-workspace, downstream users of drake are unaware of it.

• That is, they get to pull in Drake however they want i.e. local_repository or git_repository or http_archive or whatever.
• They would nominally call the Offer a supported interface to import Drake into other Bazel workspaces #7259 solution such as load("@drake//:workspace.bzl", "add_all_drake_repositories()") then add_all_drake_repositories() and we'd need the @external_data repository to come along for the ride. Possibly a hint argument to that macro would be satisfactory, but with the multiple-workspace design we'd have to explore how this works before committing.

5. If the external_data implementation is in its own sub-workspace, then (other than the workflow acceptance tests where we want to launch a new bazel server using a temp test workspace) external_data's build step and unit tests must ensure that personal bazel configurations such as caching, output directory layout, etc. are preserved -- perhaps this implies that the same bazel server has to run all tests, or perhaps there is a way to chain the settings through. (I don't recall in this setup whether there was a test_suite that pulled the tests in, or if we were launching a new bazel server from a sh_test.)

Additionally, because we list the transitive Skylark dependencies as data, that means any change (like adding a comment) will re-trigger this test.

At the scale of r3 (the list of data = [] is ~10 packages of skylark and linter tools), this doesn't seem like a big deal. Acceptance tests will often be like that, where many things can tickle them. The test will run in parallel with each other and any other things that got disturbed, and will only hit people who use test on //... which is a big time commitment anyway -- users who are testing just //systems/analysis/... or something won't tickle it.

... I feel like we're trading maintainability for consistency ... Because Bazel has no apparent ease for introspection (e.g. "give me all Skylark files that are needed to make this *.bzl file run"), we have to manually dictate transitive dependencies for things that we ultimately don't care about.

So what's going on here is that external_data's runtime deps from its workspace are very small or nothing, since it uses mostly system tools. Its compile-time deps might be no more, or possibly if we use drake_py_foo then it needs one bzl file from Drake. Its test-time deps might end up wider, such as if we reuse Drake's linting code directly in place; then we need both that linting bzl helpers and the workspace dependencies that go along with it; these are shown in the drake_workspace_files and is ~10 packages. The friction is that the test-time deps are in play even at runtime, because the load statements that pull them in are not lazy.

In terms of maintainability, the drake_workspace_files at its current scale doesn't offend me. The all_files could get rolled into the pre-existing add_lint_tests() rote epilog, so we don't need a second macro. When we forget to add something, I expect that the error message is reasonably clear, and the fix is a few lines. The trade-off seems to me either we implement the drake_workspace_files list, or implement a lint trampoline that knows how to scape and lint the external_data implementation as part of the master linting.

I'll also note that I think the choice of sub-workspace and choices of whether to use add_lint_tests within external_data's implementation, are distinct choices? I think we could omit the sub-workspace, but still do the linting out-of-package (by exporting external_data's files as all_files) so that there are no load-time dependencies from external_data on the lint infrastructure? (We could even put the linting within external_data, just not in the same package as its required runtime, so that its only loaded on demand; e.g. external_data/test has the linting rules, but is only tickled by //... not by load("@drake//tools/external_data:external_data.bzl) .)

One note so far... I can foresee that #7259 work would come into play because it expands the list of files required to be present in order to parse Drake's WORKSPACE file (from the test case) ..

This is true, but can be worked around. The test cases shouldn't use drake/WORKSPACE directly, but rather have a stub that replaces it that only loads what's necessary, using the fine-grained helper macros that will be appearing.

---

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

---

tools/external_data/BUILD.bazel, line 9 at r3 (raw file):

    "//tools/external_data/bazel_external_data:all_files",
    "//tools:all_files",
    "//tools/install:all_files",

FYI This one isn't necessary.

---

tools/external_data/workspace_test.bzl, line 31 at r3 (raw file):

        srcs = ["@drake//tools/external_data:workspace_writeable_test.sh"],
        args = args,
        data = [workspace] + data,

I'm not certain, but I think [workspace] here is cheating, using the directory trick? Don't we somehow need to glob all of the files within the test workspace, so they are declared dependencies? In workspace_writeable_test the presence of -r in the cp -r within the loop is a bad smell to me -- I think all of the dependencies should be files, not directories.

---

tools/external_data/workspace_writeable_test.sh, line 24 at r3 (raw file):

tmp_base=/tmp/bazel_workspace_test
mkdir -p ${tmp_base}
# TODO: Have this use `TEST_TMP_DIR`?

FYI $TEST_TMPDIR https://docs.bazel.build/versions/master/test-encyclopedia.html#test-interaction-with-the-filesystem

---

tools/external_data/test/workspaces/bazel_pkg_test/WORKSPACE, line 7 at r3 (raw file):

local_repository(
    name = "drake",
    path = dirname(__workspace_dir__, 5),

FYI I'd have to add some prints to be sure, but this seems wrong? Is this using the copy of (a fraction of) Drake that's been copied into our test case's tempdir? A possibly clearer approach would be to have the test runner codegen a give_me_drake() macro based on the absolute path of Drake that we've set up for the test, and then load and run that here. Then we don't need path.bzl either.

---

Comments from Reviewable

[jwnimmer-tri]

Very brief brain dump from live discussion:

• external_data itself will not be a subworkspace;
• the files within bazel_pkg_test will be enumerated as data dependencies of the test case;
• the package (i.e., directory with a BUILD.bazel file) that provides external_data.bzl will not add_lint_tests for itself, because that requires that linting be loadable during the bazel_pkg_test which makes setup more complicated; instead, external_data will do its linting tests in some other package in its directory tree;
• consider https://docs.bazel.build/versions/master/user-manual.html#bazel-releng when launching the new bazel in test cases.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 7 of 19 files reviewed at latest revision, 21 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta: This PR uses a disallowed abbreviation pkg in a handful of places.

I'd prefer have pkg in the directory name external_data_pkg_test be an exception, as it's unambiguous, the directory name is already long, and Bazel itself uses it (e.g. visibility = ["//some/package:__pkg__"]).

---

tools/external_data/expose_all_files.bzl, line 6 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Would it be better to unconditionally add the dummy? If a dummy is declared, it seems like the recipient must always be written to be aware of it (e.g., discard it), so forcing the issue by always providing it seems safest to me.

On the other hand, nothing in this PR uses the dummy feature, so its difficult for me to judge what is best.

Done. dummy was from an older attempt, but not needed here at present.

---

tools/external_data/expose_all_files.bzl, line 30 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Consider mandatory = True,.

Done.

---

tools/external_data/expose_all_files.bzl, line 46 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This duplicates the list from

drake/tools/lint/bazel_lint.bzl

Line 59 in be4f658

files = native.glob([

, and is missing some items. If they can't share the same list for dependency reasons, then at least both lists need a comment referring to the other copy, and to keep them in sync.

Done.

---

tools/external_data/expose_all_files.bzl, line 59 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW It's not obvious that the referent of Creates with those names are rules. (Suggest: "Creates rules foo...".)

FYI Also suggest s/can be one of/will be all of/.

Done.

---

tools/external_data/README.md, line 5 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We have two packages, //tools/external_data and //tools/external_data/bazel_external_data. I don't understand what code (or tests?) live (or will live) in one versus the other.

Done. bazel_external_data is named that way because that's the name I'd like for the Python package.
(We could potentially virtualize the package directory, and rename the package to src, but I'd prefer to keep this as-is for now.)

---

tools/external_data/README.md, line 27 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Maybe this doesn't need to appear in the overview? This list is already pretty overwhelming IMO. (I am going to defer worrying too much about this README until the code settles down a bit, though.)

Or maybe this bullet item could move to a "Tips for developers of external_data itself" or something.

Done. Trimmed down file list, separated out consumer / developer info.

---

tools/external_data/test/BUILD.bazel, line 11 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Here seems like the right place to document how (and why) the linting of our code (and the test workspaces) happens.

Done.

---

tools/external_data/test/external_data_workspace_test.bzl, line 39 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The name (at the call site) made me think this did the linting, but really it just collects the sources into a filegroup. I think the function needs a rename with a better verb to make it clear at the call site what's happening.

Done.

---

tools/external_data/test/external_data_workspace_test.bzl, line 41 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Suggest s/can/will/ and s/or/and/ again, since both always happen; its not a user choice given some args.

Done.

---

tools/external_data/test/external_data_workspace_test.bzl, line 59 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I know in my //tools/workspace refactoring I didn't use a verb (i.e., gflags_repository instead of add_gflags_repositoiry), but here because test can be misconstrued as a verb (I did to at first) possibly this macro should be named starting with add_ or similar?

Done.

---

tools/external_data/test/external_data_workspace_test.sh, line 1 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Per https://google.github.io/styleguide/shell.xml#Quoting I think that most of the ${foo} that lack "" in this file need them?

Done.

---

tools/external_data/test/external_data_workspace_test.sh, line 2 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I tend to prefer adding set -x in test shell scripts that are run by Bazel; it mutes output by default so isn't any trouble, and it makes finding the error (e.g. tried to use bazel run) possibly more obvious. No defect.

Done.

---

tools/external_data/test/remove_bazel_symlinks.sh, line 3 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably needs a one-sentence comment description like "Removes all bazel-* symlinks under tools/external/data/**" or similar, as well as some kind of note that this is a helper for developers to run manually and is not intended to be run by Bazel as part of a build or etc.

Maybe this file should move up a level as well? It would seem to be more obvious

Done. I'd prefer to keep this in .../test, as that's the only place where it's used.

---

tools/external_data/test/remove_bazel_symlinks.sh, line 4 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW https://google.github.io/styleguide/shell.xml#Quoting I think?

Done.

---

tools/external_data/test/workspace_test.bzl, line 13 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This didn't really tell me what's happening. A better API comment would be something like "Copies all of the runfiles to a writeable tmpdir, then execcutes args.".

Done.

---

tools/external_data/test/workspace_test.bzl, line 18 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't understand why this is here, or precisely what it means.

Done.

Ultimately, it'd be nice to have something like srcs = [native.package_name_for_macro() + ":workspace_test.sh], so that way this macro could be called in any package, and have it still resolve to the files neighboring the *.bzl file.
However, it's a small improvement, so left it as-is.

---

tools/external_data/test/workspace_test.sh, line 35 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is weird. Above, we shift off ${cmd} but never tough the rest, except for expanding them here? If we lose the shift and do exec "$@" here, would that work just as well? (To me, that seems like the most natural semantics for a wrapper.) In any case, if the current logic stays it needs some comments to explain the what / how / why (probably including the workspace_test.bzl that seems to use shell quoting very carefully, and I didn't really understand why).

Done.

Removed shell quoting from workspace_test.bzl as well (but commenting still that sh_test will oddly concatenate the arguments...).

---

tools/external_data/test/external_data_pkg_test/BUILD.bazel, line 9 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

So we are running a no-op test both inside the testing workspace (this rule) and outside of it (in @drake//tools/external_data/bazel_external_data:stub_test rule). I understand that the test is a stub for now and will be discarded, but its difficult for me to see why once we have a more featureful test, we would want to run the same test code from two workspaces? That seems redundant. If the only purpose is to ensure that files make it from Drake into the test workspace, it seems like the load statements would be good enough for that purpose?

If this is a stepping stone that gets 100% removed then that's fine, but if there's something else going on maybe we should discuss (is there an up-to-date feature branch to look at)?

OK I just wanted to have an actual (stub) test in the package; however, this will be 100% removed once the other tests come into play.

---

tools/external_data/test/external_data_pkg_test/WORKSPACE, line 7 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I've pasted a discussion from the prior home of this file into here, for further consideration. Previously:

[Jeremy] FYI I'd have to add some prints to be sure, but this seems wrong? Is this using the copy of (a fraction of) Drake that's been copied into our test case's tempdir? A possibly clearer approach would be to have the test runner codegen a give_me_drake() macro based on the absolute path of Drake that we've set up for the test, and then load and run that here. Then we don't need path.bzl either.

[Eric] OK I'd strongly prefer to keep path.bzl; otherwise, test workspace would be crippled because a developer (me) could not run it directly out-of-the-box.

I am going to think more on this and experiment a little.

Tried out a different iteration; same functionality, but more purposeful (and less sed-y).

---

tools/workspace/test_repositories.bzl, line 4 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I'd be okay without the extra level of indirection of this file, i.e., moving the load and external_data_test_repositories(workspace_dir) directly into WORKSPACE. No defect if you prefer this though.

Done. Not at all attached to this setup; just wanted to see if I could stabilize WORKSPACE a bit, but perhaps there won't be much motion from this stuff.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 46 files at r2, 2 of 37 files at r4, 12 of 15 files at r5.
Review status: 18 of 19 files reviewed at latest revision, 7 unresolved discussions.

---

tools/external_data/expose_all_files.bzl, line 46 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

The comment here is good, but I'm not seeing the comment at the other location (so that editors there are reminded to also fix this copy).

---

tools/external_data/test/remove_bazel_symlinks.sh, line 3 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

Done. I'd prefer to keep this in .../test, as that's the only place where it's used.

OK Aha! I had misread the cd $(dirname "${0}") as changing to the external_data folder, not the external_data/test folder. Now that I read it correctly, this home makes perfect sense.

---

tools/external_data/test/workspace_test.sh, line 35 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

Removed shell quoting from workspace_test.bzl as well (but commenting still that sh_test will oddly concatenate the arguments...).

I believe that having the args = [] to a workspace_test rule or external_data_workspace_test rule be eval'd instead of left alone is non-standard, so if you keep the eval here then the rules documentation for those two should be changed to have a disclaimer? In other words, if I want to pass a filename or switch that contains a space through to a workspace_test, I will have to fight against shell quoting, whereas in a sh_test I wouldn't have to do that.

(Personally, I would just change this to exec here and CMD_DEFAULT to a list.)

((Or maybe I've misunderstood how things work; I didn't actually run any experiments.))

---

tools/external_data/test/workspace_test.sh, line 14 at r5 (raw file):

# Handle case when running with `bazel run`.
if [[ -z "${TEST_TMPDIR:-}" ]]; then
    tmp_base=/tmp/bazel_workspace_test

FYI Consider /tmp/drake_external_data_workspace_test to be super-clear to a user what has placed files in /tmp? (Though this is only for manual uses, so maybe not important.)

---

tools/external_data/test/external_data_pkg_test/drake_path.bzl, line 2 at r5 (raw file):

# -*- python -*-

If I'm understanding correctly, this file needs a disclaimer comment that it is not part of the code under test, but rather is always overwritten during test execution -- except when the developer is doing their own manual testing gymnastics. As it stands, a developer would think that this code is in play during their test exection.

---

Comments from Reviewable

[jwnimmer-tri]

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

---

a discussion (no related file):

Previously, EricCousineau-TRI wrote…

I'd prefer have pkg in the directory name external_data_pkg_test be an exception, as it's unambiguous, the directory name is already long, and Bazel itself uses it (e.g. visibility = ["//some/package:__pkg__"]).

I can live with that, but the python variable name pkg_reldir then should still be fixed?

---

tools/external_data/README.md, line 5 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

Done. bazel_external_data is named that way because that's the name I'd like for the Python package.
(We could potentially virtualize the package directory, and rename the package to src, but I'd prefer to keep this as-is for now.)

OK I see, so the Python support library and cli binary code should go into bazel_external_data so their python module name is as desired, but the skylark wrappers for same should go into the parent folder as external_data.bzl. Do I have that correct? If so, SGTM.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 13 of 19 files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can live with that, but the python variable name pkg_reldir then should still be fixed?

Done. Renamed to package_relpath (still shooting for common abbreviations, as relpath is used in Python.)

---

tools/external_data/expose_all_files.bzl, line 46 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The comment here is good, but I'm not seeing the comment at the other location (so that editors there are reminded to also fix this copy).

Done.

---

tools/external_data/README.md, line 5 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I see, so the Python support library and cli binary code should go into bazel_external_data so their python module name is as desired, but the skylark wrappers for same should go into the parent folder as external_data.bzl. Do I have that correct? If so, SGTM.

Yup!

---

tools/external_data/test/workspace_test.sh, line 35 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I believe that having the args = [] to a workspace_test rule or external_data_workspace_test rule be eval'd instead of left alone is non-standard, so if you keep the eval here then the rules documentation for those two should be changed to have a disclaimer? In other words, if I want to pass a filename or switch that contains a space through to a workspace_test, I will have to fight against shell quoting, whereas in a sh_test I wouldn't have to do that.

(Personally, I would just change this to exec here and CMD_DEFAULT to a list.)

((Or maybe I've misunderstood how things work; I didn't actually run any experiments.))

Done.

---

tools/external_data/test/workspace_test.sh, line 14 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Consider /tmp/drake_external_data_workspace_test to be super-clear to a user what has placed files in /tmp? (Though this is only for manual uses, so maybe not important.)

OK I'd prefer to keep this as-is, unless it creates issues in the future.

---

tools/external_data/test/external_data_pkg_test/drake_path.bzl, line 2 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I'm understanding correctly, this file needs a disclaimer comment that it is not part of the code under test, but rather is always overwritten during test execution -- except when the developer is doing their own manual testing gymnastics. As it stands, a developer would think that this code is in play during their test exection.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

LGTM though I will wait to post the emoji in case @sammy-tri review encounters any major revisions.

---

Reviewed 6 of 6 files at r6.
Review status: 17 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

tools/external_data/test/external_data_workspace_test.sh, line 25 at r6 (raw file):

# Run command.
eval "$@"

BTW Should this also be exec?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 16 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

tools/external_data/test/external_data_workspace_test.sh, line 25 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Should this also be exec?

Done. Oops! Thanks for catching that!

---

Comments from Reviewable

[jwnimmer-tri]

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

---

Comments from Reviewable

[jwnimmer-tri]

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

---

Comments from Reviewable

[jwnimmer-tri]

Pinging @sammy-tri for the second review.

---

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

---

Comments from Reviewable

[sammy-tri]

First pass complete

---

Reviewed 5 of 46 files at r2, 6 of 37 files at r4, 7 of 15 files at r5, 3 of 6 files at r6, 2 of 3 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

tools/external_data/test/remove_bazel_symlinks.sh, line 1 at r8 (raw file):

#!/bin/bash

I can't figure out where this is called from? external_data_workspace_test.sh has a thing where it removes bazel symlinks, but not be calling this.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

tools/external_data/test/remove_bazel_symlinks.sh, line 1 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I can't figure out where this is called from? external_data_workspace_test.sh has a thing where it removes bazel symlinks, but not be calling this.

Sorry, this is purely a utility script (only ran by the user if they ran bazel test in a downstream workspace).
Would you like me to remove it, or just document that this is for convenience?

---

Comments from Reviewable

[sammy-tri]

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

---

tools/external_data/test/remove_bazel_symlinks.sh, line 1 at r8 (raw file):

Previously, EricCousineau-TRI wrote…

Sorry, this is purely a utility script (only ran by the user if they ran bazel test in a downstream workspace).
Would you like me to remove it, or just document that this is for convenience?

I think documentation should be adequate (though I do worry a bit about bitrot if it's never invoked).

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

tools/external_data/test/remove_bazel_symlinks.sh, line 1 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I think documentation should be adequate (though I do worry a bit about bitrot if it's never invoked).

Done. Went ahead and removed it for the time being.

---

Comments from Reviewable

[sammy-tri]

---

Reviewed 1 of 46 files at r2, 1 of 37 files at r4, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 46 files at r2, 1 of 37 files at r4, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[jwnimmer-tri]

Hmm, did we test this on macOS yet? Probably worth a spin, if not.

[jwnimmer-tri]

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

I'll fire it off. If you think we don't need it, you can merge anyway.

[EricCousineau-TRI]

Testing on Mac is an excellent idea! (given how finicky Bazel sandboxes are there :( )