Skip to content

Conversation

@patdhlk
Copy link
Contributor

@patdhlk patdhlk commented Dec 18, 2025

Notes for Reviewer

Please take a look and feel free to test it! There is a good written documentation and practical examples to check out.

Pre-Review Checklist for the PR Author

  • Add sensible notes for the reviewer
  • PR title is short, expressive and meaningful
  • Consider switching the PR to a draft (Convert to draft)
    • as draft PR, the CI will be skipped for pushes
  • Relevant issues are linked in the References section
  • Branch follows the naming format (csharp-iox2-123-introduce-posix-ipc-example)
  • Commits messages are according to this guideline
    • Commit messages have the issue ID ([#123] Add posix ipc example)
    • Keep in mind to use the same email that was used to sign the Eclipse Contributor Agreement
  • Assign PR to reviewer
  • All checks have passed (except task-list-completed)

PR Reviewer Reminders

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

References

Relates #1

update workflow documentation formatting.
- Updated README.md to include new features: Service Discovery and Domain Isolation.
- Expanded core concepts section with detailed explanations on Zero-Copy Shared Memory, Services and Communication Patterns, Nodes, Data Type Requirements, and Domain Isolation.
- Added new examples for WaitSet Event Multiplexing and Service Discovery, demonstrating efficient event handling and dynamic service discovery.
- Introduced NOTICE.md files in examples and src directories to clarify copyright and licensing information.
- Improved service configuration documentation with detailed examples for Publish-Subscribe, Publisher, and Subscriber configurations.
Remove integration note and clarify NuGet installation options
budrus
budrus previously approved these changes Dec 19, 2025
@dkroenke
Copy link
Member

Is Windows currently not supported? If so it can be added to the Roadmap and a hint to the Readme.

@elBoberido
Copy link
Member

@dkroenke @budrus @elfenpiff since the C# bindings will probably never be safety certified, do we need to follow the same process like in the iceoryx2 repo? I would suggest that we relax the requirements a bit, e.g. to not have to create an issue before a pull request or have to use the issue number in the commit. What do you think?

elBoberido
elBoberido previously approved these changes Dec 19, 2025
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

@patdhlk thanks for your contribution

@orecham
Copy link

orecham commented Dec 19, 2025

@patdhlk Awesome stuff :)

@patdhlk
Copy link
Contributor Author

patdhlk commented Dec 20, 2025

Is Windows currently not supported? If so it can be added to the Roadmap and a hint to the Readme.

@dkroenke Windows is supported, but currently excluded from CI pipelines. Please note that it is not yet as thoroughly tested as Linux or macOS.

@elfenpiff
Copy link

@patdhlk Thanks for all the hard work - this is amazing!

@patdhlk
Copy link
Contributor Author

patdhlk commented Dec 20, 2025

@dkroenke @budrus @elfenpiff since the C# bindings will probably never be safety certified, do we need to follow the same process like in the iceoryx2 repo? I would suggest that we relax the requirements a bit, e.g. to not have to create an issue before a pull request or have to use the issue number in the commit. What do you think?

@elBoberido actually, I would like to keep this workflow since it is used in an industrial production machine and the machine regulation has some traceability requirements as well. And tbh, it is not that much of effort to create an issue.

@dkroenke
Copy link
Member

Windows is supported, but currently excluded from CI pipelines. Please note that it is not yet as thoroughly tested as Linux or macOS.

@patdhlk
I've tested some examples on Windows and they work fine, there is just a hiccup in the dotnet build for the examples because the iceoryx2_ffi_c.dll is not properly copied into the build folder of the example from src\Iceoryx2\bin. It happens only on Windows. When copying the file manually it works well.

My guess is that the library naming in iceoryx2 is not consistent across the platforms.
For example on Linux the library is named libiceoryx2_ffi_c.dylib but on Windows without the leading lib prefix. You can see it in the src\Iceoryx2\Iceoryx2.csproj. The CopyNativeLibrary directives in the project files of the examples does not catch that since they are looing for libiceoryx2_ffi_c.*.

Maybe a solution would be to take a similar approach for copying the library from the Iceoryx2.csproj where the differences in platforms are properly handled or maybe a wildcard/regex match could help here.

It can be fixed either in this Pull-Request or we create a follow-up ticket to handle it there together with the CI Pipelines to have no further delay of this Pull-Request and make a small note in the Readme to copy the file manually. What do you think?

@patdhlk
Copy link
Contributor Author

patdhlk commented Dec 20, 2025

Windows is supported, but currently excluded from CI pipelines. Please note that it is not yet as thoroughly tested as Linux or macOS.

@patdhlk

I've tested some examples on Windows and they work fine, there is just a hiccup in the dotnet build for the examples because the iceoryx2_ffi_c.dll is not properly copied into the build folder of the example from src\Iceoryx2\bin. It happens only on Windows. When copying the file manually it works well.

My guess is that the library naming in iceoryx2 is not consistent across the platforms.

For example on Linux the library is named libiceoryx2_ffi_c.dylib but on Windows without the leading lib prefix. You can see it in the src\Iceoryx2\Iceoryx2.csproj. The CopyNativeLibrary directives in the project files of the examples does not catch that since they are looing for libiceoryx2_ffi_c.*.

Maybe a solution would be to take a similar approach for copying the library from the Iceoryx2.csproj where the differences in platforms are properly handled or maybe a wildcard/regex match could help here.

It can be fixed either in this Pull-Request or we create a follow-up ticket to handle it there together with the CI Pipelines to have no further delay of this Pull-Request and make a small note in the Readme to copy the file manually. What do you think?

@dkroenke let me fix it now

- With platform-specific copy operations that handle the different naming conventions:
- Each Copy task uses a Condition="Exists(...)" check so only the file that exists on the current platform gets copied, avoiding build errors on other platforms.
@patdhlk patdhlk dismissed stale reviews from elBoberido and budrus via bee503a December 20, 2025 14:01
@patdhlk
Copy link
Contributor Author

patdhlk commented Dec 20, 2025

@dkroenke should be fixed ✅

@dkroenke
Copy link
Member

@dkroenke should be fixed ✅

Yes retested it on Windows and it is solved now, thanks for this!

Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +21 to +24
<Copy SourceFiles="$(Iceoryx2OutputPath)/libiceoryx2_ffi_c.dylib"
DestinationFolder="$(OutputPath)"
Condition="Exists('$(Iceoryx2OutputPath)/libiceoryx2_ffi_c.dylib')"
SkipUnchangedFiles="true" />
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to set a PATH env variable instead of copying the libs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elBoberido well I think we could support it.

I assume the use case you have in mind is that a user/dev has iceoryx2 installed/cloned somewhere already and wants to use this instead of the submodule?

Copy link
Member

Choose a reason for hiding this comment

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

No, still with the submodule but without copying the libs into each folder. This somehow screams for trouble with outdated libraries.

@dkroenke dkroenke merged commit 37b648f into eclipse-iceoryx:main Dec 22, 2025
6 checks passed
@dkroenke dkroenke mentioned this pull request Dec 22, 2025
2 tasks
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.

6 participants