Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install_data: Make explicit, remove unused cruft #11010

Closed
EricCousineau-TRI opened this issue Mar 25, 2019 · 3 comments · Fixed by #18923
Closed

install_data: Make explicit, remove unused cruft #11010

EricCousineau-TRI opened this issue Mar 25, 2019 · 3 comments · Fixed by #18923
Assignees
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: high

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Mar 25, 2019

(Warning: Rant-y)

At present, install_data implicitly names its target models. I've always been confused by this, and I want that to change.

Additionally, there are zero usages of any customization of prod_models_prefix or test_models_prefix. Those should be explicitly defined too, rather than arbitrary prefixes. The extra_prod_models story also makes me sad.

This should be broken into two parts:

  • Model globbing should be consolidated, to permit globbing install + test data. (Test data isn't even installed; why is it even part of install_data???)
  • Installation for models should be consolidated (and named specifically for drake, as it does that with its dest prefix).

EDIT: More complaint: Why do we even concatenate prod_models + test_models into models??? We should only have models and test_models; if anything, test_models should depend on models (if we even really want that combination.)

Ran into this when considering #11009

@jwnimmer-tri
Copy link
Collaborator

At present, install_data implicitly names its target models.

FYI In an intermediate revision of #11089, the author mistakenly added a new redundant filegroup to examples/pendulum, presumably because the existing install_data filegroup was so obscured as to be unknown.

So +1 on resolving this pain point.

@jwnimmer-tri jwnimmer-tri added the component: build system Bazel, CMake, dependencies, memory checkers, linters label Apr 28, 2020
@jwnimmer-tri
Copy link
Collaborator

For clarity, here's the kind of patch we're looking for:

--- a/manipulation/models/iiwa_description/BUILD.bazel
+++ b/manipulation/models/iiwa_description/BUILD.bazel
@@ -4,7 +4,7 @@ load(
     "@drake//tools/skylark:drake_cc.bzl",
     "drake_cc_googletest",
 )
-load("//tools/install:install_data.bzl", "install_data")
+load("//tools/install:install_data.bzl", "models_filegroup", "install_data")
 load("//tools/lint:lint.bzl", "add_lint_tests")
 
 # This package is public so that other packages can refer to
@@ -13,6 +13,15 @@ package(
     default_visibility = ["//visibility:public"],
 )
 
+models_filegroup(
+    name = "models",
+    # testonly_filegroup_name = "test_models",  # N.B. Not needed here, no test_models.
+    extra = [
+        "LICENSE.TXT",
+        "iiwa_stack.LICENSE.txt",
+    ],
+)
+
 # === test/ ===
 
 drake_cc_googletest(
@@ -27,10 +36,7 @@ drake_cc_googletest(
 )
 
 install_data(
-    extra_prod_models = [
-        "LICENSE.TXT",
-        "iiwa_stack.LICENSE.txt",
-    ],
+    data = [":models"],
 )
 
 add_lint_tests()

The goal is that all target names are literals in the BUILD file, i.e., name = "models", and that silly intermediate labels like prod_models go away.

@jwnimmer-tri
Copy link
Collaborator

This is getting in my way for #15774. I'll do it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants