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

Add ParserConfig class to encapsulate file path settings #439

Merged
merged 24 commits into from
Feb 4, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c4e2ba4
Remove outdated error message
azeey Dec 12, 2020
89dc847
Add ParserConfig class to encapsulate file path settings
azeey Dec 12, 2020
7badb17
Use semi-colon as path separator in windows
azeey Dec 15, 2020
b4f1da3
Add header to CMakeLists
azeey Dec 15, 2020
e3e05f3
Merge remote-tracking branch 'upstream/master' into parser_config
azeey Dec 15, 2020
f9e3ef2
Address reviewer feedback
azeey Dec 19, 2020
deda523
Rename DefaultConfig to GlobalConfig
azeey Dec 19, 2020
b9bb22b
Fix style
azeey Dec 19, 2020
ddafb64
Merge remote-tracking branch 'upstream/master' into parser_config
azeey Jan 14, 2021
74eacf0
Merge remote-tracking branch 'upstream/master' into parser_config
azeey Jan 21, 2021
f088be8
Fix test expectations after merge
azeey Jan 21, 2021
5084a6d
Merge remote-tracking branch 'upstream/master' into parser_config
azeey Jan 21, 2021
5dfe1a0
Merge branch 'master' into parser_config
scpeters Feb 2, 2021
1cf635a
Merge branch 'master' into parser_config
scpeters Feb 2, 2021
e42bbda
sdf -> SDF
azeey Feb 2, 2021
6388959
Use ImplPtr for ParserConfig
azeey Feb 2, 2021
fde3b1b
Implement initString overload
azeey Feb 2, 2021
6f53a9f
List the search order in sdf::findFile
azeey Feb 2, 2021
8977cd1
Add test checking that URI paths have precedence over findFile callbacks
azeey Feb 2, 2021
5972b53
Codecheck
azeey Feb 2, 2021
1d321e0
Add reference to `sdf::findFile` in the docs of ParserConfig
azeey Feb 2, 2021
8a9e4e8
Expand documentation for ParserConfig::SetFindCallback
azeey Feb 3, 2021
53af3c7
Merge branch 'master' into parser_config
azeey Feb 3, 2021
ed20a74
Add test expecations with sdf::findFile
azeey Feb 3, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/sdf/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ set (headers
Noise.hh
Param.hh
parser.hh
ParserConfig.hh
Pbr.hh
Physics.hh
Plane.hh
Expand Down
130 changes: 130 additions & 0 deletions include/sdf/ParserConfig.hh
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright 2020 Open Source Robotics Foundation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

#ifndef SDF_PARSER_CONFIG_HH_
#define SDF_PARSER_CONFIG_HH_

#include <functional>
#include <map>
#include <string>
#include <vector>

#include <sdf/Error.hh>
#include "sdf/sdf_config.h"
#include "sdf/system_util.hh"

namespace sdf
{
inline namespace SDF_VERSION_NAMESPACE
{
// Forward declare private data class.
class ParserConfigPrivate;

/// This class contains configuration options for the libsdformat parser.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay! Thanks!

///
/// The configuration options include:
/// * A callback function used to locate a file from a given URI
/// * A map from URI schemes to search directories
///
/// For backward compatibility, the functions sdf::setFindCallback() and
/// sdf::addURIPath() update a singleton ParserConfig object, which can be
/// retrieved by ParserConfig::GlobalConfig().
///
/// The functions sdf::readFile(), sdf::readString(), and \ref Root::Load
/// "sdf::Root::Load()" have overloads that take a ParserConfig object. If
/// the ParserConfig object is omitted, these functions will use the singleton
/// ParserConfig object.
///
/// Example:
/// To set an additional URI scheme search directory without affecting the
/// global config,
///
/// \code{.cpp}
/// // Copy the global config
/// sdf::ParserConfig config = ParserConfig::GlobalConfig();
///
/// // Add the new scheme to the config
/// config.AddURIPath("newScheme://", "path/to/directory");
///
/// // Use the new config when loading a new SDFormat file
/// sdf::Root root;
/// root.Load("path/to/file.sdf", config);
/// \endcode
class SDFORMAT_VISIBLE ParserConfig
{
/// type alias for the map from URI scheme to search directories
public: using SchemeToPathMap =
std::map<std::string, std::vector<std::string> >;

/// \brief Default constructor
public: ParserConfig();

/// \brief Copy constructor
/// \param[in] _config ParserConfig to copy.
public: ParserConfig(const ParserConfig &_config);

/// \brief Move constructor
/// \param[in] _config ParserConfig to move.
public: ParserConfig(ParserConfig &&_config) noexcept;

/// \brief Move assignment operator.
/// \param[in] _config ParserConfig to move.
/// \return Reference to this.
public: ParserConfig &operator=(ParserConfig &&_config) noexcept;

/// \brief Copy assignment operator.
/// \param[in] _config ParserConfig to copy.
/// \return Reference to this.
public: ParserConfig &operator=(const ParserConfig &_config);

/// \brief Destructor
public: ~ParserConfig();

/// Mutable access to a singleton ParserConfig that serves as the global
/// ParserConfig object for all parsing operations
azeey marked this conversation as resolved.
Show resolved Hide resolved
/// \return A mutable reference to the singleton ParserConfig object
public: static ParserConfig &GlobalConfig();

/// \brief Get the find file callback function
/// \return Immutable reference to the find file callback function
public: const std::function<std::string(const std::string &)> &
FindFileCallback() const;

/// \brief Set the callback to use when libsdformat can't find a file.
/// The callback should return a complete path to the requested file, or
scpeters marked this conversation as resolved.
Show resolved Hide resolved
/// and empty string if the file was not found in the callback.
azeey marked this conversation as resolved.
Show resolved Hide resolved
/// \param[in] _cb The callback function.
public: void SetFindCallback(
std::function<std::string(const std::string &)> _cb);

/// \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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aye, makes sense to me.


/// \brief Associate paths to a URI.
/// Example paramters: "model://", "/usr/share/models:~/.gazebo/models"
/// \param[in] _uri URI that will be mapped to _path
/// \param[in] _path Colon separated set of paths.
public: void AddURIPath(const std::string &_uri, const std::string &_path);

/// \brief Private data pointer.
private: ParserConfigPrivate *dataPtr;
};
}
}

#endif
18 changes: 18 additions & 0 deletions include/sdf/Root.hh
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,31 @@ namespace sdf
/// an error code and message. An empty vector indicates no error.
public: Errors Load(const std::string &_filename);

/// \brief Parse the given SDF file, and generate objects based on types
/// specified in the SDF file.
/// \param[in] _filename Name of the SDF file to parse.
/// \param[in] _config Custom parser configuration
/// \return Errors, which is a vector of Error objects. Each Error includes
/// an error code and message. An empty vector indicates no error.
public: Errors Load(
const std::string &_filename, const ParserConfig &_config);

/// \brief Parse the given SDF string, and generate objects based on types
/// specified in the SDF file.
/// \param[in] _sdf SDF string to parse.
/// \return Errors, which is a vector of Error objects. Each Error includes
/// an error code and message. An empty vector indicates no error.
public: Errors LoadSdfString(const std::string &_sdf);

/// \brief Parse the given SDF string, and generate objects based on types
/// specified in the SDF file.
/// \param[in] _sdf SDF string to parse.
/// \param[in] _config Custom parser configuration
/// \return Errors, which is a vector of Error objects. Each Error includes
/// an error code and message. An empty vector indicates no error.
public: Errors LoadSdfString(
const std::string &_sdf, const ParserConfig &_config);

/// \brief Parse the given SDF pointer, and generate objects based on types
/// specified in the SDF file.
/// \param[in] _sdf SDF pointer to parse.
Expand Down
21 changes: 18 additions & 3 deletions include/sdf/SDFImpl.hh
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@
#include <memory>
#include <string>

#include "sdf/Param.hh"
#include "sdf/Element.hh"
#include "sdf/Param.hh"
#include "sdf/ParserConfig.hh"
#include "sdf/Types.hh"
#include "sdf/sdf_config.h"
#include "sdf/system_util.hh"
#include "sdf/Types.hh"

#ifdef _WIN32
// Disable warning C4251 which is triggered by
Expand Down Expand Up @@ -64,6 +65,21 @@ namespace sdf
bool _searchLocalPath = true,
bool _useCallback = false);

/// \brief Find the absolute path of a file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh, that's excellent! Thank you!

/// \param[in] _filename Name of the file to find.
/// \param[in] _searchLocalPath True to search for the file in the current
/// working directory.
/// \param[in] _useCallback True to find a file based on a registered
/// callback if the file is not found via the normal mechanism.
/// \param[in] _config Custom parser configuration
/// \return File's full path.
SDFORMAT_VISIBLE
std::string findFile(const std::string &_filename,
bool _searchLocalPath,
bool _useCallback,
const ParserConfig &_config);


/// \brief Associate paths to a URI.
/// Example paramters: "model://", "/usr/share/models:~/.gazebo/models"
/// \param[in] _uri URI that will be mapped to _path
Expand All @@ -78,7 +94,6 @@ namespace sdf
SDFORMAT_VISIBLE
void setFindCallback(std::function<std::string (const std::string &)> _cb);


/// \brief Base SDF class
class SDFORMAT_VISIBLE SDF
{
Expand Down
Loading