Skip to content

Commit

Permalink
[parsing] Stop using mutable globals (RobotLocomotion#16791)
Browse files Browse the repository at this point in the history
  • Loading branch information
jwnimmer-tri authored and aykut-tri committed Jun 1, 2022
1 parent 860b656 commit 122371c
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 29 deletions.
1 change: 0 additions & 1 deletion multibody/parsing/detail_sdf_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,6 @@ sdf::ParserConfig MakeSdfParserConfig(
MultibodyPlant<double>* plant,
bool test_sdf_forced_nesting) {

// TODO(marcoag) ensure that we propagate the right ParserConfig instance.
sdf::ParserConfig parser_config;
parser_config.SetWarningsPolicy(sdf::EnforcementPolicy::ERR);
parser_config.SetDeprecatedElementsPolicy(sdf::EnforcementPolicy::WARN);
Expand Down
60 changes: 32 additions & 28 deletions multibody/parsing/test/detail_sdf_geometry_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,33 @@ using std::unique_ptr;
using systems::Context;
using systems::LeafSystem;

sdf::ParserConfig MakeStrictConfig() {
sdf::ParserConfig result;
result.SetWarningsPolicy(sdf::EnforcementPolicy::ERR);
result.SetDeprecatedElementsPolicy(sdf::EnforcementPolicy::ERR);
result.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::ERR);
return result;
}

sdf::SDFPtr ReadString(const std::string& input) {
sdf::SDFPtr result(new sdf::SDF());
sdf::init(result);

sdf::ParserConfig config = MakeStrictConfig();
sdf::Errors errors;
const bool success = sdf::readString(input, config, result, errors);
if (!success) {
for (const auto& error : errors) {
drake::log()->error("Parse error: {}", error);
}
// Note that we don't throw here, we just spam the console. This is not
// great, but it matches the pre-existing behavior which wants this helper
// to return a default-constructed value in the case of syntax errors.
}

return result;
}

// Helper to create an sdf::Geometry object from its SDF specification given
// as a string. Example of what the string should contain:
// <cylinder>
Expand All @@ -78,18 +105,7 @@ unique_ptr<sdf::Geometry> MakeSdfGeometryFromString(
" </link>"
" </model>"
"</sdf>";
sdf::SDFPtr sdf_parsed(new sdf::SDF());
sdf::init(sdf_parsed);
sdf::Errors errors;
const bool success = sdf::readString(sdf_str, sdf_parsed, errors);
if (!success) {
for (const auto& error : errors) {
drake::log()->error("MakeSdfGeometryFromString parse error: {}", error);
}
// Note that we don't throw here, we just spam the console. This is not
// great, but it matches the pre-existing behavior which wants this helper
// to return a default-constructed value in the case of syntax errors.
}
sdf::SDFPtr sdf_parsed = ReadString(sdf_str);
sdf::ElementPtr geometry_element =
sdf_parsed->Root()->GetElement("model")->
GetElement("link")->GetElement("visual")->GetElement("geometry");
Expand Down Expand Up @@ -120,18 +136,7 @@ unique_ptr<sdf::Visual> MakeSdfVisualFromString(
" </link>"
" </model>"
"</sdf>";
sdf::SDFPtr sdf_parsed(new sdf::SDF());
sdf::init(sdf_parsed);
sdf::Errors errors;
const bool success = sdf::readString(sdf_str, sdf_parsed, errors);
if (!success) {
for (const auto& error : errors) {
drake::log()->error("MakeSdfVisualFromString parse error: {}", error);
}
// Note that we don't throw here, we just spam the console. This is not
// great, but it matches the pre-existing behavior which wants this helper
// to return a default-constructed value in the case of syntax errors.
}
sdf::SDFPtr sdf_parsed = ReadString(sdf_str);
sdf::ElementPtr visual_element =
sdf_parsed->Root()->GetElement("model")->
GetElement("link")->GetElement("visual");
Expand Down Expand Up @@ -166,9 +171,7 @@ unique_ptr<sdf::Collision> MakeSdfCollisionFromString(
" </link>"
" </model>"
"</sdf>";
sdf::SDFPtr sdf_parsed(new sdf::SDF());
sdf::init(sdf_parsed);
sdf::readString(sdf_str, sdf_parsed);
sdf::SDFPtr sdf_parsed = ReadString(sdf_str);
sdf::ElementPtr collision_element =
sdf_parsed->Root()->GetElement("model")->
GetElement("link")->GetElement("collision");
Expand Down Expand Up @@ -482,7 +485,8 @@ GTEST_TEST(SceneGraphParserDetail, VisualGeometryNameRequirements) {
"</sdf>",
visual_str);
sdf::Root root;
auto errors = root.LoadSdfString(sdf_str);
sdf::ParserConfig config = MakeStrictConfig();
auto errors = root.LoadSdfString(sdf_str, config);
return errors.empty();
};

Expand Down
51 changes: 51 additions & 0 deletions tools/workspace/sdformat/patches/fix_broken_config.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
Fix a lack of ParserConfig propagation.

This fix should be submitted to libsdformat upstream.

--- src/parser.cc.orig 2021-09-30 15:33:35.000000000 -0700
--- src/parser.cc 2022-03-14 23:23:10.623173055 -0700
@@ -181,7 +181,7 @@
/// \param[out] _errors Captures errors encountered during parsing.
static void insertIncludedElement(sdf::SDFPtr _includeSDF,
const SourceLocation &_sourceLoc, bool _merge,
- sdf::ElementPtr _parent, sdf::Errors &_errors)
+ sdf::ElementPtr _parent, const ParserConfig &_config, sdf::Errors &_errors)
{
Error invalidFileError(ErrorCode::FILE_READ,
"Included model is invalid. Skipping model.");
@@ -231,7 +231,7 @@
// We create a throwaway sdf::Root object in order to validate the
// included entity.
sdf::Root includedRoot;
- sdf::Errors includeDOMerrors = includedRoot.Load(_includeSDF);
+ sdf::Errors includeDOMerrors = includedRoot.Load(_includeSDF, _config);
_errors.insert(_errors.end(), includeDOMerrors.begin(),
includeDOMerrors.end());

@@ -1512,7 +1512,7 @@
ElementPtr refSDF;
refSDF.reset(new Element);
std::string refFilename = refSDFStr + ".sdf";
- initFile(refFilename, refSDF);
+ initFile(refFilename, _config, refSDF);
_sdf->RemoveFromParent();
_sdf->Copy(refSDF);

@@ -1585,7 +1585,7 @@
SDFPtr includeSDF(new SDF);
includeSDF->Root(includeSDFTemplate->Root()->Clone());

- if (!readFile(filename, includeSDF))
+ if (!readFile(filename, _config, includeSDF, _errors))
{
Error err(
ErrorCode::FILE_READ,
@@ -1803,7 +1803,7 @@
SourceLocation sourceLoc{includeXmlPath, _source,
elemXml->GetLineNum()};

- insertIncludedElement(includeSDF, sourceLoc, toMerge, _sdf, _errors);
+ insertIncludedElement(includeSDF, sourceLoc, toMerge, _sdf, _config, _errors);
continue;
}
}
78 changes: 78 additions & 0 deletions tools/workspace/sdformat/patches/no_global_config.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
Change the global singleton into a bomb.

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

--- src/ParserConfig.cc.orig 2021-09-30 15:33:35.000000000 -0700
+++ src/ParserConfig.cc 2022-03-14 22:49:17.262811731 -0700
@@ -57,8 +57,7 @@
/////////////////////////////////////////////////
ParserConfig &ParserConfig::GlobalConfig()
{
- static auto *defaultConfig = new ParserConfig;
- return *defaultConfig;
+ throw std::runtime_error("Drake must never use ParserConfig::GlobalConfig()");
}

/////////////////////////////////////////////////

--- src/parser.cc.orig 2021-09-30 15:33:35.000000000 -0700
+++ src/parser.cc 2022-03-14 23:16:20.973451521 -0700
@@ -597,7 +597,7 @@

ElementPtr element(new Element);

- initFile(filename, element);
+ initFile(filename, ParserConfig{}, element);

// override description for include elements
tinyxml2::XMLElement *description = child->FirstChildElement("description");
--- src/ign.cc.orig 2021-09-30 15:33:35.000000000 -0700
+++ src/ign.cc 2022-03-15 00:13:58.609410856 -0700
@@ -36,7 +36,7 @@
int result = 0;

sdf::Root root;
- sdf::Errors errors = root.Load(_path);
+ sdf::Errors errors = root.Load(_path, sdf::ParserConfig{});
if (!errors.empty())
{
for (auto &error : errors)
@@ -85,8 +85,12 @@
return -1;
}

- if (!sdf::readFile(_path, sdf))
+ if (!sdf::readFile(_path, sdf::ParserConfig{}, sdf, errors))
{
+ for (auto &error : errors)
+ {
+ std::cerr << error << std::endl;
+ }
std::cerr << "Error: SDF parsing the xml failed.\n";
return -1;
}
@@ -147,8 +152,13 @@
return -1;
}

- if (!sdf::readFile(_path, sdf))
+ sdf::Errors errors;
+ if (!sdf::readFile(_path, sdf::ParserConfig{}, sdf, errors))
{
+ for (auto &error : errors)
+ {
+ std::cerr << error << std::endl;
+ }
std::cerr << "Error: SDF parsing the xml failed.\n";
return -1;
}
@@ -170,7 +180,7 @@
}

sdf::Root root;
- sdf::Errors errors = root.Load(_path);
+ sdf::Errors errors = root.Load(_path, sdf::ParserConfig{});
if (!errors.empty())
{
std::cerr << errors << std::endl;
2 changes: 2 additions & 0 deletions tools/workspace/sdformat/repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ def sdformat_repository(
build_file = "@drake//tools/workspace/sdformat:package.BUILD.bazel",
patches = [
"@drake//tools/workspace/sdformat:patches/console.patch",
"@drake//tools/workspace/sdformat:patches/fix_broken_config.patch",
"@drake//tools/workspace/sdformat:patches/no_global_config.patch",
"@drake//tools/workspace/sdformat:patches/no_urdf.patch",
],
mirrors = mirrors,
Expand Down

0 comments on commit 122371c

Please sign in to comment.