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

Tease apart models_filegroup vs install_data #18923

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Mar 2, 2023

Delete unused PendulumWithFriction.urdf.


I've tested locally and the list of installed files is the same between this PR and the master branch.

Closes #11010.

Towards #15774.

FYI https://drake.mit.edu/stable.html#model-files


This change is Reviewable

@jwnimmer-tri jwnimmer-tri force-pushed the models-filegrouping branch from 009b690 to 6f1f821 Compare March 2, 2023 00:30
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review March 2, 2023 00:30
@jwnimmer-tri jwnimmer-tri added priority: medium status: single reviewer ok https://drake.mit.edu/reviewable.html release notes: none This pull request should not be mentioned in the release notes labels Mar 2, 2023
@jwnimmer-tri jwnimmer-tri force-pushed the models-filegrouping branch from 6f1f821 to 1ece06b Compare March 2, 2023 00:38
@jwnimmer-tri
Copy link
Collaborator Author

+@EricCousineau-TRI I think you get right of first refusal for this review. If you're busy, we can push it to @sammy-tri instead.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 34 of 34 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:, beautimous! Small nit on globbing, though maybe it's pre-existing.

Reviewed 23 of 34 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


tools/skylark/drake_data.bzl line 36 at r2 (raw file):

    ]
    exclude = glob_exclude + [
        "**/test/*",

nit (pre-existing) It's not clear to me how intuitive this exclude pattern will be for testonly = True.

Possible options:

  • TODO
  • glob_exclude gains these defaults only when testonly = False
  • The default values are these, explicitly, perhaps via global constant DEFAULT_GLOB_EXCLUDE, but users can override them. Docstring says they should?

Delete unused PendulumWithFriction.urdf.
@jwnimmer-tri jwnimmer-tri force-pushed the models-filegrouping branch from 1ece06b to e6745c7 Compare March 2, 2023 15:41
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)


tools/skylark/drake_data.bzl line 36 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (pre-existing) It's not clear to me how intuitive this exclude pattern will be for testonly = True.

Possible options:

  • TODO
  • glob_exclude gains these defaults only when testonly = False
  • The default values are these, explicitly, perhaps via global constant DEFAULT_GLOB_EXCLUDE, but users can override them. Docstring says they should?

Aha! Thanks, I'd meant to circle back here and totally forgot.

There are no tests that use this. I've update the docs.

PTAL.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform) (waiting on @jwnimmer-tri)


tools/skylark/drake_data.bzl line 36 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Aha! Thanks, I'd meant to circle back here and totally forgot.

There are no tests that use this. I've update the docs.

PTAL.

OK Sweet ,thanks!

@EricCousineau-TRI EricCousineau-TRI merged commit 2467945 into RobotLocomotion:master Mar 2, 2023
@jwnimmer-tri jwnimmer-tri deleted the models-filegrouping branch March 2, 2023 18:57
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install_data: Make explicit, remove unused cruft
2 participants