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

Added C language bindings for SystemPaths::FindFile and Console::SetVerbosity #168

Closed
wants to merge 1 commit into from

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Feb 6, 2021

New feature

Helps closing gazebosim/gz-launch#92 .

Summary

The functionality in SystemPaths is already quite mature, so I think it makes sense to expose it to other languages via C bindings. This will help code re-use in other parts of Ignition libraries, e.g. in Ruby scripts.

Test it

Unit tests were added. Otherwise, you can test it e.g. from Ruby by importing the common library and calling some of the added functions.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

…erbosity.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Signed-off-by: Martin Pecka <peci1@seznam.cz>
@peci1 peci1 requested a review from mjcarroll as a code owner February 6, 2021 15:33
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Feb 6, 2021
@mjcarroll mjcarroll self-assigned this Feb 8, 2021
@chapulina chapulina removed the 🏢 edifice Ignition Edifice label Feb 22, 2021
@chapulina
Copy link
Contributor

We have been treating these C functions as internal to each library, and we often break their APIs. It hasn't been our intention so far to use them across libraries. I can see how this is useful, but I think we need to be careful before we start using them this way.

I don't have a concrete alternative proposal at the moment. I think that once all libraries have transitioned to loading binaries instead of libraries (gazebosim/gz-tools#7), we should be able to use the C++ API for this kind of thing.

@peci1
Copy link
Contributor Author

peci1 commented Mar 6, 2021

Ahh, that's a pity. So I'm inclined to close this PR and re-implement what SystemPaths do in ign-launch in Ruby (probably not so sophisticated). Because I want the ign-launch PR to appear in Dome, and I assume the ign binaries will start being used in Edifice.

Do you have any idea how easy or difficult would it be to actually use the C++ API via Ruby FFI? Wouldn't it be too cumbersome? C API is usually much easier to use via FFI.

If you ever plan to support also other language bindings like Python, it might actually be beneficial to provide a stable C API. That way, contributors to the library would only need to make sure their change either does not need changing the C API (which would mean the bindings need no change at all), or that it needs a change in the C API, and that would mean all bindings also need to be updated. But I think the first case would be much more common, making contributing to the libraries much easier. I think there's not that many developers that are fluent in C++, C, and e.g. Ruby, Python and Java (which are some possible candidates for language bindings).

@chapulina
Copy link
Contributor

We've been using SWIG in ign-math to map C++ to Ruby. That's a direction we could take with other libraries too, and it would also allow support for other languages like Python.

I'm inclined to close this PR and re-implement what SystemPaths do in ign-launch in Ruby

I think you don't need to go all the way and implement SystemPaths to close gazebosim/gz-launch#92. Maybe just splitting the path like ign-gazebo does would be enough?

@peci1
Copy link
Contributor Author

peci1 commented Mar 9, 2021

I think you don't need to go all the way and implement SystemPaths to close ignitionrobotics/ign-launch#92. Maybe just splitting the path like ign-gazebo does would be enough?

I was thinking something in that way but I really don't like reinventing the wheel. For example, the referenced Gazebo snippet will not work on Windows.

I'm closing this PR in favor of some proper future bindings.

Thanks for the ideas.

@peci1 peci1 closed this Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants