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

[parsing] Stop using mutable globals #16791

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Mar 15, 2022

This ensures that our parsing configuration is always what we asked for.

Towards #16784.

A related issue has been filed upstream as gazebosim/sdformat#881.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: medium release notes: fix This pull request contains fixes (no new features) labels Mar 15, 2022
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please.

Copy link
Contributor

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

+@sherm1 for platform review.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

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, LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)


tools/workspace/sdformat/patches/no_global_config.patch, line 3 at r1 (raw file):

Change the global singleton into a bomb.

Use the default config when parsing the built-in root.sdf.

Working

I should tune up this comment slightly, before we merge this PR.

Suggestion:

Use the default config when parsing the built-in root.sdf, and in
the ign_sdf command-line tool.

@jwnimmer-tri jwnimmer-tri force-pushed the parsing-sdf-global-debung branch from f0fe21a to 8a3f931 Compare March 15, 2022 17:54
Copy link
Contributor

@rpoyner-tri rpoyner-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 r2, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

@jwnimmer-tri jwnimmer-tri force-pushed the parsing-sdf-global-debung branch from 8a3f931 to 9da5f8c Compare March 15, 2022 20:34
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm:

Reviewed 1 of 5 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform) (waiting on @jwnimmer-tri)

Copy link
Contributor

@rpoyner-tri rpoyner-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 all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri merged commit 517ca2f into RobotLocomotion:master Mar 15, 2022
@jwnimmer-tri jwnimmer-tri deleted the parsing-sdf-global-debung branch March 15, 2022 21:14
scpeters added a commit to scpeters/sdformat that referenced this pull request Mar 16, 2022
Copied from patch in RobotLocomotion/drake#16791
by jwnimmer-tri.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to gazebosim/sdformat that referenced this pull request Mar 16, 2022
* Fix a lack of ParserConfig propagation.

Copied from patch in RobotLocomotion/drake#16791
by jwnimmer-tri.

* Pass ParserConfig to parser helper functions

Require ParserConfig argument in parser.cc's private
helper functions: initDoc, initXml, and _initFile.

Signed-off-by: Steve Peters <[email protected]>
xuchenhan-tri pushed a commit to xuchenhan-tri/drake that referenced this pull request Mar 24, 2022
aykut-tri pushed a commit to aykut-tri/drake that referenced this pull request May 27, 2022
aykut-tri pushed a commit to aykut-tri/drake that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants