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

Update configuration parsing logic #116

Merged
merged 20 commits into from
Mar 15, 2024
Merged

Update configuration parsing logic #116

merged 20 commits into from
Mar 15, 2024

Conversation

xela-95
Copy link
Member

@xela-95 xela-95 commented Mar 11, 2024

Note

This PR affects all plugins

With this PR every plugin can specify its yarp configuration both with <yarpConfigurationString> and <yarpConfigurationFile>.

When using <yarpConfigurationFile> the path to the file can be specified in the following ways:

Closes #68
Closes #80

@xela-95 xela-95 self-assigned this Mar 11, 2024
@xela-95
Copy link
Member Author

xela-95 commented Mar 13, 2024

@traversaro any idea on why the unit test with the CI fails in loading the plugins? I don't know how to debug this, since on my machine it works. Maybe on my machine gazebo finds the plugin libraries in the install directory?

Remove unnecessary dependencies and add GZ_SIM_SYSTEM_PLUGIN_PATH env variable
@xela-95
Copy link
Member Author

xela-95 commented Mar 13, 2024

@traversaro any idea on why the unit test with the CI fails in loading the plugins? I don't know how to debug this, since on my machine it works. Maybe on my machine gazebo finds the plugin libraries in the install directory?

Ok found! This test was missing his GZ_SIM_SYSTEM_PLUGIN_PATH environment variable so it was unable to find plugins.

@xela-95 xela-95 requested a review from traversaro March 13, 2024 14:56
@xela-95 xela-95 marked this pull request as ready for review March 13, 2024 14:56
@xela-95
Copy link
Member Author

xela-95 commented Mar 13, 2024

Now there is the Windows conda CI failing with:

D:\a\gz-sim-yarp-plugins\gz-sim-yarp-plugins\libraries\common\ConfigurationHelpers.cpp(140): error C2679: binary '=': no operator found which takes a right-hand operand of type 'std::filesystem::path' (or there is no acceptable conversion)

@xela-95 xela-95 mentioned this pull request Mar 13, 2024
6 tasks
@traversaro
Copy link
Member

Relative path

Relative path w.r.t. to what?

By the way, do you think we could document the semantics of yarpConfigurationFile that you wrote down in this PR somewhere in the docs of the repo?

@xela-95
Copy link
Member Author

xela-95 commented Mar 14, 2024

Relative path w.r.t. to what?

By the way, do you think we could document the semantics of yarpConfigurationFile that you wrote down in this PR somewhere in the docs of the repo?

Sorry for the ambiguity, the path is relative to the current working directory.

For example, looking at the ForceTorqueTest, the robotinterface path is specified as:

<yarpRobotInterfaceConfigurationFile>../../../tests/forcetorque/forcetorque_nws.xml</yarpRobotInterfaceConfigurationFile>

and it is actually resolved to the absolute path /home/acroci/repos/gz-sim-yarp-plugins/build/tests/forcetorque/../../../tests/forcetorque/forcetorque_nws.xml

I'm thinking to update the handling of the relative path by using std::filesystem::canonical or std::filesystem::weakly_canonical (see https://en.cppreference.com/w/cpp/filesystem/canonical) in order to remove ./ and ../ sections and have a "canonical" path.

Yep I will also add a section in the README specifying the alternatives to specify paths.

@traversaro
Copy link
Member

I am not sure it is is a great feature, basically it (silently, not in a really clear way) constraints the sdf to be opened only if the simulator or the executable using the gz-sim as a library is launched in the directory it is intended to be launched. I guess we need it know for the tests, but probably we can avoid to advertise it/discourage to use it in actual models (a bit like absolute paths).

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

See comments.

@xela-95
Copy link
Member Author

xela-95 commented Mar 14, 2024

I am not sure it is is a great feature, basically it (silently, not in a really clear way) constraints the sdf to be opened only if the simulator or the executable using the gz-sim as a library is launched in the directory it is intended to be launched. I guess we need it know for the tests, but probably we can avoid to advertise it/discourage to use it in actual models (a bit like absolute paths).

After more carefully thinking about it, I agree with you. I mainly added the relative path feature in order to be compatible with the models already present in tests and tutorials that were using relative paths, but this could become tricky and cumbersome to debug for users.

Do you think it's better to remove this feature for now and refactor the paths in test and tutorials to use the URI? Or we should treat relative paths like absolute ones, by logging a warning for a potentially unreliable path while allowing it?

@traversaro
Copy link
Member

Do you think it's better to remove this feature for now and refactor the paths in test and tutorials to use the URI? Or we should treat relative paths like absolute ones, by logging a warning for a potentially unreliable path while allowing it?

I would go for the fast option, that I guess it is the second.

@xela-95
Copy link
Member Author

xela-95 commented Mar 14, 2024

macOS test disabled related issue: #90

@xela-95 xela-95 requested a review from traversaro March 14, 2024 16:14
@xela-95
Copy link
Member Author

xela-95 commented Mar 15, 2024

Updated README

README.md Outdated Show resolved Hide resolved
@xela-95
Copy link
Member Author

xela-95 commented Mar 15, 2024

Merging! 🚀

@xela-95 xela-95 merged commit 7653d56 into main Mar 15, 2024
5 checks passed
@xela-95 xela-95 deleted the feature/yarp-config-file branch March 15, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add helper functions to locate yarp configuration files Support yarpConfigurationFile for all plugins
2 participants