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

[tools] Port tests to use runfiles properly #22186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Nov 15, 2024

In Bazel 8, we can't just assume runfiles are in the cwd with well-known names.

Towards #22182.


This change is Reviewable

In Bazel 8, we can't just assume runfiles are in the cwd with
well-known names.
@jwnimmer-tri jwnimmer-tri added priority: low 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 Nov 15, 2024
@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for feature review, please.

@jwnimmer-tri jwnimmer-tri removed the status: single reviewer ok https://drake.mit.edu/reviewable.html label Nov 15, 2024
Copy link
Contributor

@xuchenhan-tri xuchenhan-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:

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers


tools/lint/buildifier.py line 24 at r1 (raw file):

    """Returns a list starting with the buildifier executable, followed by any
    required default arguments."""
    manifest = runfiles.Create()

BTW, is runfiles.Create() a new functionality from Bazel? This looks a lot cleaning than the find_data script. Wonder why we don't use it before.

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.

+@sammy-tri for platform review per schedule, please.

Reviewable status: LGTM missing from assignee sammy-tri(platform)


tools/lint/buildifier.py line 24 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, is runfiles.Create() a new functionality from Bazel? This looks a lot cleaning than the find_data script. Wonder why we don't use it before.

I suppose it is "new" relative to when find_data was originally written in 2017, but it has been around for 5 years (#10659) or probably even earlier. This buildifier wrapper some of the oldest code in Drake, and just never got revisited after we learned about the Rlocation function offered by Bazel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants