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 and use find-module for GFlags #12205

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Install and use find-module for GFlags #12205

merged 2 commits into from
Oct 17, 2019

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Oct 17, 2019

Relates in part to RobotLocomotion/drake-external-examples#145. Basically the find-module from drake-external-examples.


This change is Reviewable

@jamiesnape jamiesnape added component: distribution Nightly binaries, monthly releases, docker, installation unused team: kitware status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits labels Oct 17, 2019
@jamiesnape
Copy link
Contributor Author

+@BetsyMcPhail for feature review.

@BetsyMcPhail
Copy link
Contributor

:lgtm:

@jamiesnape
Copy link
Contributor Author

+@ggould-tri for platform review.

@jwnimmer-tri
Copy link
Collaborator

I don't necessarily think it should prevent us from merging this PR, but FYI I've been thinking we should stop installing text_logging_gflags.h, in order to avoid the complexity (both what's already there, and this new stuff, and the next new stuff we'll need again later) vs the low benefit. Instead, I would propose adding two string constants for the default value and help to text_logging.h, and let the users do the DEFINE and FLAGS call themselves.

Copy link
Contributor

@ggould-tri ggould-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: I agree with @jwnimmer-tri both that we could and probably should simplify log setup going forward, and that this PR is fine for now regardless.

I wonder how many users would be affected.

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail)

@jamiesnape jamiesnape merged commit d8b52a6 into RobotLocomotion:master Oct 17, 2019
@jamiesnape jamiesnape deleted the find-gflags branch October 17, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: distribution Nightly binaries, monthly releases, docker, installation status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits unused team: kitware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants