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

Iceperf example updated - ein bisschen less advertisement #1860

Merged
merged 7 commits into from
Aug 3, 2023

Conversation

anderay
Copy link
Contributor

@anderay anderay commented Jan 30, 2023

Pre-Review Checklist for the PR Author

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

Notes for Reviewer

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@elfenpiff
Copy link
Contributor

@anderay Thanks for the contribution!

Could you please follow the contribution guidelines from here: https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md

Things you still have to do:

  1. Sign the Eclipse Committer Agreement
  2. Create an issue what you implemented here so that other reviewers see the intention of the changes.
  3. The pull request name, every comment etc. should be in the english language
  4. The PR should have the prefix iox-??? where ??? is the issue number (the issue you created in 2.)
  5. Every commit requires the prefix iox-#??? (do not forget the # here)

Please remove all the commented out code.

As soon this is realized I am happy to review the PR.

@elBoberido
Copy link
Member

@anderay please also run clang-format on your code.

I skimmed over the PR and it is great to soon have a performance test with the WaitSet. I had it since ages on my to-do list so I'm happy to see this PR.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #1860 (46358c6) into master (15c82fc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1860   +/-   ##
=======================================
  Coverage   74.31%   74.31%           
=======================================
  Files         414      414           
  Lines       16086    16086           
  Branches     2249     2249           
=======================================
  Hits        11955    11955           
  Misses       3416     3416           
  Partials      715      715           
Flag Coverage Δ
unittests 74.10% <ø> (ø)
unittests_timing 14.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

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.

Nice. Just a few small remarks. Looks good overall.

iceoryx_examples/iceperf/iceoryx.cpp Show resolved Hide resolved
iceoryx_examples/iceperf/iceoryx_wait.cpp Show resolved Hide resolved
iceoryx_examples/iceperf/iceperf_leader.cpp Show resolved Hide resolved
iceoryx_examples/iceperf/iceoryx_wait.hpp Show resolved Hide resolved
iceoryx_examples/iceperf/iceoryx_wait.cpp Outdated Show resolved Hide resolved
@elBoberido elBoberido added the example providing example for a specific feature label Feb 6, 2023
@elBoberido
Copy link
Member

@anderay do you plan to continue working on this PR?

@andrzejpolanskiaptiv
Copy link
Contributor

@anderay do you plan to continue working on this PR?

What else is needed?

@elBoberido
Copy link
Member

@andrzejpolanskiaptiv the biggest issue is the Eclipse ECA. Since we are an Eclipse project, all contributors must sign the ECA. There is also a CI check for this which prevents merging.

If you want, you can also implement the proposal from #1860 (comment) but that is not required. Your answer was a bit ambiguous

@andrzejpolanskiaptiv
Copy link
Contributor

"You successfully submitted an Eclipse Contributor Agreement"
But seems not be visible here...

@elBoberido
Copy link
Member

elBoberido commented Jul 31, 2023

@andrzejpolanskiaptiv It might take some time until everything is synchronized and the check passes. Just in case, did you sign with the same e-mail address as you used for your commits?

@andrzejpolanskiaptiv
Copy link
Contributor

@andrzejpolanskiaptiv It might take some time until everything is synchronized and the check passes. Just in case, did you sign with the same e-mail address as you used for your commits?

I did it long time ago. Now I see that in commits my company email is stated whilst in ECA private was used. Perhaps that was the problem. I have changed address in Eclipse account, will see...

@elBoberido elBoberido linked an issue Aug 1, 2023 that may be closed by this pull request
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.

Looks good. Just a few minor things.

  • please tick the checkboxes from the PR author checklist
  • please add iox-#2003 to your commit messages, e.g. iox-#2003 Iceperf example - ein bisschen less advertisement (I created Extend 'iceperf' with 'WaitSet' #2003 for this purpose)
  • please add an entry to iceoryx-unreleased.me like - Extend 'iceperf' with 'WaitSet' [#2003](https://github.com/eclipse-iceoryx/iceoryx/issues/2003) in the Features section
  • please rebase to latest master, it seems the CI is not triggered for whatever reasons

@anderay
Copy link
Contributor Author

anderay commented Aug 1, 2023

here you are

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.

Okay, we are really really close. Only the bazel build needs some adjustments and then I can approve the PR :)

Could you please add iceoryx_wait.hpp/cpp to BAZEL.build. Right after iceoryx_c.hpp/cpp

Copy link
Contributor

@mossmaurice mossmaurice left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR! 🙏

@mossmaurice mossmaurice merged commit 7865d0e into eclipse-iceoryx:master Aug 3, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example providing example for a specific feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend 'iceperf' with 'WaitSet'
5 participants