iox-#2011 Apply cpptoml patch only if not yet applied

elBoberido commented 2 months ago

Notes for Reviewer

Backport of https://github.com/eclipse-iceoryx/iceoryx/pull/2261.

Pre-Review Checklist for the PR Author

[x] Code follows the coding style of CONTRIBUTING.md
[x] Tests follow the best practice for testing
[x] Changelog updated in the unreleased section including API breaking changes
[x] Branch follows the naming format (iox-123-this-is-a-branch)
[x] Commits messages are according to this guideline
- [x] Commit messages have the issue ID (iox-#123 commit text)
- [x] Commit author matches Eclipse Contributor Agreement (and ECA is signed)
[x] Update the PR title
- Follow the same conventions as for commit messages
- Link to the relevant issue
[x] Relevant issues are linked
[x] Add sensible notes for the reviewer
[x] All checks have passed (except task-list-completed)
[x] Assign PR to reviewer

Checklist for the PR Reviewer

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

Post-review Checklist for the PR Author

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

References

Relates to #2011

elBoberido commented 2 months ago

@Crola1702 @clalancette this should fix the warning which was introduced by the patch to fix a warning :)

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 78.55%. Comparing base (9a81c23) to head (aab127d). Report is 5 commits behind head on release_2.0.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2262/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/2262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) ```diff @@ Coverage Diff @@ ## release_2.0 #2262 +/- ## =============================================== - Coverage 78.83% 78.55% -0.28% =============================================== Files 370 370 Lines 14716 14220 -496 Branches 2061 2061 =============================================== - Hits 11601 11171 -430 + Misses 2432 2367 -65 + Partials 683 682 -1 ``` | [Flag](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2262/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/2262/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `77.77% <ø> (-0.29%)` | :arrow_down: | | [unittests_timing](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2262/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `15.15% <ø> (-0.34%)` | :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. [see 149 files with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2262/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)

clalancette commented 2 months ago

I'm certainly not opposed to doing this, but it is odd that it is needed. We know that the vendored version of cpptoml doesn't have this patch, so when would we need to reverse it? During a rebuild?

elBoberido commented 2 months ago

@clalancette from this log it looks like cmake was executed twice.

[100%] Built target ext_cpptoml
error: patch failed: CMakeLists.txt:1
error: CMakeLists.txt: patch does not apply
CMake Warning at cmake/cpptoml/CMakeLists.txt:81 (message):
CMake step [patch] for 'cpptoml-build' failed! Error code: 1! Build of 
'cpptoml-build' might fail

The warning was generated by our cmake code since the patch could not be applied. This could be either because something in the cpptoml sources changed or because the patch was already applied.

By trying to revert the patch we can now detect whether it was already applied and if the patch now fails, we know that it is due to a change in the cpptoml sources.

This is just to protect our future selves from the sins of our present selves :)

budrus commented 2 months ago

@elBoberido @elfenpiff Also with the risk that I don't tell you something new, the latest ROS build breaks. I'm not sure if you get the mails https://build.ros2.org/job/Hdev__iceoryx__ubuntu_jammy_amd64/8/console

elBoberido commented 2 months ago

@budrus it seems the git version on the CI does not yet have the quiet option. Will create another PR ... all good things come in threes

cc @clalancette

eclipse-iceoryx / iceoryx