eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.57k stars 373 forks source link

iox-#2272 Make iceoryx resource prefix a compile time option #2273

Closed elBoberido closed 2 months ago

elBoberido commented 2 months ago

Notes for Reviewer

This PR makes the resource prefix a compile time option

Pre-Review Checklist for the PR Author

  1. [x] Code follows the coding style of CONTRIBUTING.md
  2. [x] Tests follow the best practice for testing
  3. [x] Changelog updated in the unreleased section including API breaking changes
  4. [x] Branch follows the naming format (iox-123-this-is-a-branch)
  5. [x] Commits messages are according to this guideline
  6. [x] Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. [x] Relevant issues are linked
  8. [x] Add sensible notes for the reviewer
  9. [x] All checks have passed (except task-list-completed)
  10. [x] Assign PR to reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

  1. [x] All open points are addressed and tracked via issues

References

elBoberido commented 2 months ago

@ximion if you compile with -DIOX_DEFAULT_RESOURCE_PREFIX=foo you can create your own island with iceoryx applications without interference from ROS2 applications running with Cyclone DDS and iceoryx.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.12%. Comparing base (e1c82d6) to head (4be9a7b).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273/graphs/tree.svg?width=650&height=150&src=pr&token=KWu8wdCc1S&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) ```diff @@ Coverage Diff @@ ## main #2273 +/- ## ========================================== - Coverage 78.14% 78.12% -0.02% ========================================== Files 432 432 Lines 15894 15898 +4 Branches 2297 2297 ========================================== Hits 12420 12420 - Misses 2630 2631 +1 - Partials 844 847 +3 ``` | [Flag](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `77.94% <100.00%> (-0.02%)` | :arrow_down: | | [unittests_timing](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `14.97% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | Coverage Δ | | |---|---|---| | [...x\_posh/include/iceoryx\_posh/iceoryx\_posh\_types.hpp](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?src=pr&el=tree&filepath=iceoryx_posh%2Finclude%2Ficeoryx_posh%2Ficeoryx_posh_types.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eF9wb3NoL2luY2x1ZGUvaWNlb3J5eF9wb3NoL2ljZW9yeXhfcG9zaF90eXBlcy5ocHA=) | `100.00% <ø> (ø)` | | | [iceoryx\_posh/source/iceoryx\_posh\_types.cpp](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?src=pr&el=tree&filepath=iceoryx_posh%2Fsource%2Ficeoryx_posh_types.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eF9wb3NoL3NvdXJjZS9pY2Vvcnl4X3Bvc2hfdHlwZXMuY3Bw) | `83.33% <100.00%> (ø)` | | | [iceoryx\_posh/source/roudi/roudi.cpp](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?src=pr&el=tree&filepath=iceoryx_posh%2Fsource%2Froudi%2Froudi.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eF9wb3NoL3NvdXJjZS9yb3VkaS9yb3VkaS5jcHA=) | `64.91% <100.00%> (+0.28%)` | :arrow_up: | | [iceoryx\_posh/source/runtime/posh\_runtime\_impl.cpp](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273?src=pr&el=tree&filepath=iceoryx_posh%2Fsource%2Fruntime%2Fposh_runtime_impl.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eF9wb3NoL3NvdXJjZS9ydW50aW1lL3Bvc2hfcnVudGltZV9pbXBsLmNwcA==) | `57.53% <100.00%> (+0.21%)` | :arrow_up: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2273/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)
ximion commented 2 months ago

@ximion if you compile with -DIOX_DEFAULT_RESOURCE_PREFIX=foo you can create your own island with iceoryx applications without interference from ROS2 applications running with Cyclone DDS and iceoryx.

Oh wow, this is super nice, thank you! I'll test it :-) One drawback is that this will make it hard for people to compile my app and ROS2 using Linux-distribution-provided iceoryx packages - I would basically have to vendor IOX, which is slightly inconvenient. But I guess a lot of values are compile-time constants in IOX for performance and to simplify memory management, and this will make it possible for bespoke deployments to run both tools at the same time, which is incredibly nice and removes a big potential obstacle for when we switched to iceoryx :-)

Thank you!

elBoberido commented 2 months ago

@ximion if you compile with -DIOX_DEFAULT_RESOURCE_PREFIX=foo you can create your own island with iceoryx applications without interference from ROS2 applications running with Cyclone DDS and iceoryx.

Oh wow, this is super nice, thank you! I'll test it :-) One drawback is that this will make it hard for people to compile my app and ROS2 using Linux-distribution-provided iceoryx packages - I would basically have to vendor IOX, which is slightly inconvenient. But I guess a lot of values are compile-time constants in IOX for performance and to simplify memory management, and this will make it possible for bespoke deployments to run both tools at the same time, which is incredibly nice and removes a big potential obstacle for when we switched to iceoryx :-)

Thank you!

@ximion it shouldn't be too hard to make this a run time option but I'm lacking time at the moment. Patches are welcome :)

ximion commented 2 months ago

@ximion it shouldn't be too hard to make this a run time option but I'm lacking time at the moment. Patches are welcome :)

That looks doable! Once the journal review I'm working on is done I'll look into this :-)

elBoberido commented 2 months ago

Great. You can ping us in the gitter chat or open a new discussion in the repo. We could give you some hints to get you up to speed :)