Add ParserConfig class to encapsulate file path settings #439

azeey · 2020-12-15T01:28:05Z

Toward #373

This adds a new ParserConfig object that contains configuration options for the libsdformat parser. This replaces the old globals g_uriPathMap and g_findFileCB. To do this in a backward compatible way, the ParserConfig class provides a singleton object that calls to sdf::setSdfFindCallback and sdf::addURIPath use.

New overloads of the main SDFormat file and string loading APIs allow users to pass in a ParserConfig object. If omitted, these APIs will default to the singleton ParserConfig object.

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

codecov-io · 2020-12-15T18:21:51Z

Codecov Report

Merging #439 (b9bb22b) into master (fc4dc8c) will increase coverage by 0.14%.
The diff coverage is 99.01%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
+ Coverage   87.60%   87.75%   +0.14%     
==========================================
  Files          63       64       +1     
  Lines        9616     9668      +52     
==========================================
+ Hits         8424     8484      +60     
+ Misses       1192     1184       -8

Impacted Files	Coverage Δ
src/parser.cc	`80.35% <97.87%> (+1.21%)`	⬆️
src/ParserConfig.cc	`100.00% <100.00%> (ø)`
src/Root.cc	`96.59% <100.00%> (+0.09%)`	⬆️
src/SDF.cc	`93.33% <100.00%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc4dc8c...b9bb22b. Read the comment docs.

brawner

I don't feel confident enough in the code base to approve/request changes yet, so I'll leave that to other reviewers. But I did have a few comments.

My main question is whether the DefaultConfig() should be a mutable singleton. Typically a default cannot be changed by client code, so I would expect the API to return a static const ParserConfig &. What were the tradeoffs compared to passing around a reference to a config object in the parsing code, with sdf::Root::Load() and related functions taking a default argument?

If the mutable singleton is the best approach here, what would you think about changing it's name to GlobalConfig()?

src/ParserConfig.cc

include/sdf/ParserConfig.hh

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey

If the mutable singleton is the best approach here, what would you think about changing it's name to GlobalConfig()?

I thought a mutable singleton was needed to maintain backward compatibility. The functions sdf::setFindFileCallback and sdf::addURIPath used to modify the globals g_findFileCB and g_uriPathMap respectively. Now they modify the singleton. But I do agree that GlobalConfig is a much better name. Renamed in deda523

src/ParserConfig.cc

include/sdf/ParserConfig.hh

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2020-12-19T19:17:34Z

@osrf-jenkins run tests please

azeey · 2020-12-19T19:53:29Z

Not sure why the Jenkins CI status is not updating even though they are finished:

Ubuntu
Homebrew
Windows

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

EricCousineau-TRI · 2021-01-28T20:36:28Z

May need rebase or merge (again)?

EricCousineau-TRI · 2021-01-28T20:38:25Z

include/sdf/parser.hh

@@ -40,22 +40,60 @@ namespace sdf
  //
  class Root;

-  /// \brief Init based on the installed sdf_format.xml file


BTW This is a large amount of combinatorics... But I don't have a better solution.

Yeah :(. I think there are some redundancies that we can deprecate, but we still end up with a lot of functions.

EricCousineau-TRI

First pass done, PTAL!

include/sdf/parser.hh

EricCousineau-TRI · 2021-01-28T20:45:56Z

include/sdf/ParserConfig.hh

+// Forward declare private data class.
+class ParserConfigPrivate;
+
+/// This class contains configuration options for the libsdformat parser.


In addition to below comment (how path resolution works), it's unclear to me what the precedence is for these different attributes (e.g. does callback override search paths, or the other way around).

Is there documentation that this can forward-reference, or should it be written here?

I added some documentation in SDFImpl.hh (6f53a9f) and referenced it here in 1d321e0

Yay! Thanks!

EricCousineau-TRI · 2021-01-28T20:46:01Z

include/sdf/SDFImpl.hh

@@ -64,6 +65,21 @@ namespace sdf
                       bool _searchLocalPath = true,
                       bool _useCallback = false);

+  /// \brief Find the absolute path of a file.


This new function, and the old flavor, don't fully specify how the searching actually takes place. (I understand it's implementation specific, but still.)

From _useCallback, I infer that "normal mechanism" means _searchLocalPath, but I don't know it for sure.
Can you make this text more concise, and tell what the priorities are?

I don't know about more concise, but I listed the search order in 6f53a9f. How does that look?

Ooh, that's excellent! Thank you!

EricCousineau-TRI · 2021-01-28T20:48:19Z

include/sdf/ParserConfig.hh

+
+  /// \brief Get the URI scheme to search directories map
+  /// \return Immutable reference to the URI scheme to search directories map
+  public: const SchemeToPathMap &URIPathMap() const;


nit It smells weird to be able to look at all paths, add a new path, but not remove a path. (assymmetry)

Is it hard to add that here?

Or perhaps the right workflow is to just reassign? e.g. just clear it out.

So if I wanted to remove or reorder a path, I would just do something like Parser::GLobalConfig() = my_new_config?

I was trying to make this PR replicate existing functionality without adding any new features other than being able to specify a non-global config. It would be easy to add a function to remove a URI scheme, but adding a path to an existing URI scheme or reordering paths seem best suited for a future PR, IMO.

Aye, makes sense to me.

EricCousineau-TRI · 2021-01-28T20:49:49Z

test/integration/parser_config.cc

@@ -0,0 +1,240 @@
+/*


Are there any tests which explicitly show shadowing / prioritization?
e.g. both a search path and a callback could resolve the same file (possibly to different places), but only one of them wins out?

Added test in 8977cd1

scpeters

there is a SDFPtr readFile(const std::string &) at parser.hh:105 that didn't get overloaded

also, #474 was just merged, so you can use ImplPtr in ParserConfig

include/sdf/ParserConfig.hh

include/sdf/parser.hh

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-02-02T18:44:28Z

there is a SDFPtr readFile(const std::string &) at parser.hh:105 that didn't get overloaded

I was trying to have one less overload by only overloading the signature that also contains an sdf::Error. I did the same for readString.

Here's a list of all the functions in parser.hh that work with ParserConfig. I think it helps to see them without the documentation:

bool init(SDFPtr _sdf);

bool initFile(const std::string &_filename, SDFPtr _sdf);
bool initFile(const std::string &_filename, const ParserConfig &_config, SDFPtr _sdf);

bool initFile(const std::string &_filename, ElementPtr _sdf);
bool initFile(const std::string &_filename, const ParserConfig &_config, ElementPtr _sdf);

bool initString(const std::string &_xmlString, SDFPtr _sdf);
bool initString(const std::string &_xmlString, const ParserConfig &_config, SDFPtr _sdf);

sdf::SDFPtr readFile(const std::string &_filename);
sdf::SDFPtr readFile(const std::string &_filename, Errors &_errors);
sdf::SDFPtr readFile(const std::string &_filename, const ParserConfig &_config, Errors &_errors);

bool readFile(const std::string &_filename, SDFPtr _sdf);
bool readFile(const std::string &_filename, SDFPtr _sdf, Errors &_errors);
bool readFile(const std::string &_filename, const ParserConfig &_config, SDFPtr _sdf, Errors &_errors);

bool readFileWithoutConversion(const std::string &_filename, SDFPtr _sdf, Errors &_errors);
bool readFileWithoutConversion(const std::string &_filename, const ParserConfig &_config, SDFPtr _sdf, Errors &_errors);

bool readString(const std::string &_xmlString, SDFPtr _sdf);
bool readString(const std::string &_xmlString, SDFPtr _sdf, Errors &_errors);
bool readString(const std::string &_xmlString, const ParserConfig &_config, SDFPtr _sdf, Errors &_errors);

bool readString(const std::string &_xmlString, ElementPtr _sdf);
bool readString(const std::string &_xmlString, ElementPtr _sdf, Errors &_errors);
bool readString(const std::string &_xmlString, const ParserConfig &_config, ElementPtr _sdf, Errors &_errors);

bool readStringWithoutConversion( const std::string &_xmlString, SDFPtr _sdf, Errors &_errors);
bool readStringWithoutConversion(const std::string &_xmlString, const ParserConfig &_config, SDFPtr _sdf, Errors &_errors);

bool convertFile(const std::string &_filename, const std::string &_version, SDFPtr _sdf);
bool convertFile(const std::string &_filename, const std::string &_version, const ParserConfig &_config, SDFPtr _sdf);

bool convertString(const std::string &_sdfString, const std::string &_version, SDFPtr _sdf);
bool convertString(const std::string &_sdfString, const std::string &_version, const ParserConfig &_config, SDFPtr _sdf);

It's interesting that we have sdf::SDFPtr readFile(const std::string &_filename); and bool readString(const std::string &_xmlString, SDFPtr _sdf);. I'm guessing the latter was added along with bool readString(const std::string &_xmlString, ElementPtr _sdf);. Should we deprecate sdf::SDFPtr readFile(const std::string &_filename);?

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters · 2021-02-03T01:16:20Z

there is a SDFPtr readFile(const std::string &) at parser.hh:105 that didn't get overloaded

I was trying to have one less overload by only overloading the signature that also contains an sdf::Error. I did the same for readString.

that sounds fine to me

Should we deprecate sdf::SDFPtr readFile(const std::string &_filename);?

I would be ok with that. It's not the safest API since it doesn't have any way to report errors

scpeters · 2021-02-03T01:34:01Z

I've reported the windows compiler warning to the ign-utils repository gazebosim/gz-utils#9

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-02-03T17:18:44Z

@osrf-jenkins run tests please

scpeters

this is looking good to me; I just have two small suggestions for extra test expectations

scpeters · 2021-02-03T20:00:03Z

test/integration/parser_config.cc

+
+  ASSERT_TRUE(sdf::ParserConfig::GlobalConfig().FindFileCallback());
+  EXPECT_EQ("test/dir2",
+      sdf::ParserConfig::GlobalConfig().FindFileCallback()("empty"));


how do you feel about adding some expectations about sdf::findFile?

diff --git a/test/integration/parser_config.cc b/test/integration/parser_config.cc index d6bd2fb3..3f9780ac 100644 --- a/test/integration/parser_config.cc +++ b/test/integration/parser_config.cc @@ -51,6 +51,9 @@ TEST(ParserConfig, GlobalConfig) ASSERT_TRUE(sdf::ParserConfig::GlobalConfig().FindFileCallback()); EXPECT_EQ("test/dir2", sdf::ParserConfig::GlobalConfig().FindFileCallback()("empty")); + // sdf::findFile requires explicitly enabling callbacks + EXPECT_EQ("test/dir2", sdf::findFile("empty", false, true)); + EXPECT_EQ("test/dir2", sdf::findFile("empty", true, true)); } /////////////////////////////////////////////////

scpeters · 2021-02-03T20:01:27Z

test/integration/parser_config.cc

+  EXPECT_EQ(it->second.front(), testDir);
+
+  ASSERT_TRUE(config.FindFileCallback());
+  EXPECT_EQ("test/dir2", config.FindFileCallback()("empty"));


how do you feel about calling sdf::findFile here as well?

diff --git a/test/integration/parser_config.cc b/test/integration/parser_config.cc @@ -79,6 +82,8 @@ TEST(ParserConfig, NonGlobalConfig) ASSERT_TRUE(config.FindFileCallback()); EXPECT_EQ("test/dir2", config.FindFileCallback()("empty")); + EXPECT_EQ("test/dir2", sdf::findFile("empty", false, true, config)); + EXPECT_EQ("test/dir2", sdf::findFile("empty", true, true, config)); EXPECT_TRUE(sdf::ParserConfig::GlobalConfig().URIPathMap().empty()); EXPECT_FALSE(sdf::ParserConfig::GlobalConfig().FindFileCallback());

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-02-04T18:36:14Z

@osrf-jenkins run tests please

azeey added 2 commits December 11, 2020 18:14

Remove outdated error message

c4e2ba4

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add ParserConfig class to encapsulate file path settings

89dc847

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey added the 🏢 edifice Ignition Edifice label Dec 15, 2020

azeey added this to the SDFormat 1.8 / libsdformat11 milestone Dec 15, 2020

azeey requested review from scpeters and EricCousineau-TRI December 15, 2020 01:28

azeey self-assigned this Dec 15, 2020

azeey requested a review from brawner December 15, 2020 16:53

azeey added 3 commits December 15, 2020 11:56

Use semi-colon as path separator in windows

7badb17

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add header to CMakeLists

b4f1da3

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge remote-tracking branch 'upstream/master' into parser_config

e3e05f3

brawner reviewed Dec 15, 2020

View reviewed changes

src/ParserConfig.cc Outdated Show resolved Hide resolved

include/sdf/ParserConfig.hh Outdated Show resolved Hide resolved

azeey added 2 commits December 19, 2020 12:14

Address reviewer feedback

f9e3ef2

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Rename DefaultConfig to GlobalConfig

deda523

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey commented Dec 19, 2020

View reviewed changes

src/ParserConfig.cc Outdated Show resolved Hide resolved

include/sdf/ParserConfig.hh Outdated Show resolved Hide resolved

Fix style

b9bb22b

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey added 4 commits January 14, 2021 12:27

Merge remote-tracking branch 'upstream/master' into parser_config

ddafb64

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge remote-tracking branch 'upstream/master' into parser_config

74eacf0

Fix test expectations after merge

f088be8

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge remote-tracking branch 'upstream/master' into parser_config

5084a6d

azeey mentioned this pull request Jan 22, 2021

Implement SDFormat 1.8 Interface API #475

Merged

EricCousineau-TRI reviewed Jan 28, 2021

View reviewed changes

brawner mentioned this pull request Jan 30, 2021

Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

Merged

brawner mentioned this pull request Jan 30, 2021

//joint/axis/use_parent_model_frame does not fail fast or issue a warning in SDFormat 1.7? #327

Closed

scpeters added 2 commits February 1, 2021 22:26

Merge branch 'master' into parser_config

5dfe1a0

Merge branch 'master' into parser_config

1cf635a

scpeters reviewed Feb 2, 2021

View reviewed changes

include/sdf/ParserConfig.hh Outdated Show resolved Hide resolved

include/sdf/ParserConfig.hh Outdated Show resolved Hide resolved

include/sdf/ParserConfig.hh Show resolved Hide resolved

include/sdf/parser.hh Outdated Show resolved Hide resolved

azeey added 2 commits February 2, 2021 11:25

sdf -> SDF

e42bbda

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Use ImplPtr for ParserConfig

6388959

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey added 5 commits February 2, 2021 12:47

Implement initString overload

fde3b1b

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

List the search order in sdf::findFile

6f53a9f

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add test checking that URI paths have precedence over findFile callbacks

8977cd1

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Codecheck

5972b53

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add reference to sdf::findFile in the docs of ParserConfig

1d321e0

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Expand documentation for ParserConfig::SetFindCallback

8a9e4e8

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge branch 'master' into parser_config

53af3c7

scpeters approved these changes Feb 3, 2021

View reviewed changes

Add test expecations with sdf::findFile

ed20a74

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

EricCousineau-TRI approved these changes Feb 4, 2021

View reviewed changes

azeey merged commit a390765 into gazebosim:master Feb 4, 2021

azeey deleted the parser_config branch February 4, 2021 18:55

EricCousineau-TRI mentioned this pull request Feb 25, 2021

Stop using global variables #373

Open

traversaro mentioned this pull request Mar 29, 2021

Resolve Fuel resources when included in models and worlds robotology/gym-ignition#309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ParserConfig class to encapsulate file path settings #439

Add ParserConfig class to encapsulate file path settings #439

azeey commented Dec 15, 2020 •

edited

Loading

codecov-io commented Dec 15, 2020 •

edited

Loading

brawner left a comment

azeey left a comment

azeey commented Dec 19, 2020

azeey commented Dec 19, 2020

EricCousineau-TRI commented Jan 28, 2021

EricCousineau-TRI Jan 28, 2021

azeey Feb 2, 2021

EricCousineau-TRI left a comment

EricCousineau-TRI Jan 28, 2021

azeey Feb 2, 2021

EricCousineau-TRI Feb 4, 2021

EricCousineau-TRI Jan 28, 2021

azeey Feb 2, 2021

EricCousineau-TRI Feb 4, 2021

EricCousineau-TRI Jan 28, 2021

azeey Feb 2, 2021

EricCousineau-TRI Feb 4, 2021

EricCousineau-TRI Jan 28, 2021

azeey Feb 2, 2021

scpeters left a comment

azeey commented Feb 2, 2021 •

edited

Loading

scpeters commented Feb 3, 2021

scpeters commented Feb 3, 2021

azeey commented Feb 3, 2021

scpeters left a comment

scpeters Feb 3, 2021

scpeters Feb 3, 2021

azeey commented Feb 4, 2021

Add ParserConfig class to encapsulate file path settings #439

Add ParserConfig class to encapsulate file path settings #439

Conversation

azeey commented Dec 15, 2020 • edited Loading

codecov-io commented Dec 15, 2020 • edited Loading

Codecov Report

brawner left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

azeey commented Dec 19, 2020

azeey commented Dec 19, 2020

EricCousineau-TRI commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

azeey commented Feb 2, 2021 • edited Loading

scpeters commented Feb 3, 2021

scpeters commented Feb 3, 2021

azeey commented Feb 3, 2021

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey commented Feb 4, 2021

azeey commented Dec 15, 2020 •

edited

Loading

codecov-io commented Dec 15, 2020 •

edited

Loading

azeey commented Feb 2, 2021 •

edited

Loading