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] Add cmake_configure_files (plural) macro #19952

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Aug 7, 2023

Towards #19945.

+@xuchenhan-tri for both reviews (off-schedule), please.


This change is Reviewable

@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 Aug 7, 2023
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: x2 with two questions.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions


tools/workspace/cmake_configure_file.bzl line 101 at r1 (raw file):

    )

def cmake_configure_files(

BTW, can you refer me to a place where this new function is used?

Code quote:

def cmake_configure_files(

tools/workspace/cmake_configure_file.py line 163 at r1 (raw file):

            and len(args.input) > 0):
        parser.print_usage()
        sys.exit(1)

nit, is there a useful error message if len(args.input) == 0?

Code quote:

sys.exit(1)

This is important when configuring multiple files with the same
bag of definitions, so `strict = True` mode still makes sense.

Improve the diagnostic output to show *all* missing definitions, not
just the first one. This helps a lot during Local development.
@jwnimmer-tri jwnimmer-tri force-pushed the configure-files-plural branch from b24fdda to dfe40d8 Compare August 7, 2023 17:52
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


tools/workspace/cmake_configure_file.bzl line 101 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, can you refer me to a place where this new function is used?

It's used in #19945.


tools/workspace/cmake_configure_file.py line 163 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit, is there a useful error message if len(args.input) == 0?

Now there is.

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.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee xuchenhan-tri(platform)

@xuchenhan-tri xuchenhan-tri merged commit 92e830d into RobotLocomotion:master Aug 7, 2023
@jwnimmer-tri jwnimmer-tri deleted the configure-files-plural branch August 7, 2023 22:28
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.

2 participants