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

Treat IGN_LAUNCH_CONFIG_PATH as a path list. #93

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Feb 8, 2021

Bug Report

Fixes issue #92.

Summary

Treat IGN_LAUNCH_CONFIG_PATH as a path list instead of just one path.

Depends on ign-common C bindings: gazebosim/gz-common#168 , because I felt reinventing the C++ wheel in Ruby is not really the way to go.

This is my first piece of code in Ruby, so give it a thorough check because I generally didn't know what I'm doing :) Most importantly, it'd be fine to find a way to use the Importer in such a way that importing the second library doesn't break the code loaded from the first one, but I couldn't wrap my head around it...

I was also thinking whether it's better to pass an absolute path to the ign-common library or just a filename, and decided just for the filename to ease having multiple ign-common versions side-by-side and choosing between them based on LD_LIBRARY_PATH. But passing the absolute path is possible if you'd think it's better for some reason.

I also didn't know how to add tests for this feature. However, there were none for the whole Ruby command file anyways...

I do the development on Dome, though it seemed to me this feature could go even in Citadel, so I pointed this PR to the Citadel version and maintain a 1-1 cherry-pick of it for Dome in https://github.com/peci1/ign-launch/tree/env-pathlist . That might speed up forward-porting of the feature, which I'm interested in :)

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (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

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1
Copy link
Contributor Author

peci1 commented Mar 9, 2021

After a discussion in gazebosim/gz-common#168 I changed this PR to not use the C API of ign-common SystemPaths and instead implemented a simple path split mechanism in Ruby.

I tested locally on Linux all possible combinations of launch files - a default-installed one, launch file from first workspace, launchfile from an overlay workspace, absolute path. In all cases, ign launch behaves as expected both with 1-element and 2-element path lists.

@peci1
Copy link
Contributor Author

peci1 commented Mar 9, 2021

To say it explicitly - this PR no longer depends on gazebosim/gz-common#168 .

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for iterating and thanks for the documentation!

@chapulina chapulina merged commit 5a63fec into gazebosim:ign-launch2 Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants