search

eclipse-iceoryx / iceoryx

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

iox-#2011 Patch cpptoml to use at least CMake 3.5. #2232

Closed clalancette closed 7 months ago

clalancette commented 7 months ago

Newer versions of CMake (like CMake 3.28 in Ubuntu 24.04) complain when projects ask for compatibility with versions of CMake earlier than 3.5. Bump cpptoml to at least 3.5 here.

Pre-Review Checklist for the PR Author

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

Notes for 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

codecov[bot] commented 7 months ago

Codecov Report

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

Project coverage is 78.83%. Comparing base (a80bd29) to head (9a81c23). Report is 1 commits behind head on release_2.0.

:exclamation: Current head 9a81c23 differs from pull request most recent head fb08207. Consider uploading reports for the commit fb08207 to get more accurate results

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2232/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/2232?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 #2232 +/- ## ============================================ Coverage 78.83% 78.83% ============================================ Files 370 370 Lines 14716 14716 Branches 2061 2061 ============================================ Hits 11601 11601 + Misses 2434 2432 -2 - Partials 681 683 +2 ``` | [Flag](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2232/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/2232/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `78.05% <ø> (ø)` | | | [unittests_timing](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2232/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `15.48% <ø> (-0.02%)` | :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 5 files with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2232/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)
elBoberido commented 7 months ago

I think you can use #2011 for this PR. Please add iox-#2011 to your commit message.

Furthermore, can you please change vmactions/freebsd-vm@v0 to vmactions/freebsd-vm@v1 in .github/workflows/build-test.yml.

Additionally, to make the CI happy can you adjust the patch to work similarly to this https://github.com/eclipse-iceoryx/iceoryx/blob/release_2.0/cmake/googletest/CMakeLists.txt#L73-L82

clalancette commented 7 months ago

I think you can use #2011 for this PR. Please add iox-#2011 to your commit message.

Done now.

Furthermore, can you please change vmactions/freebsd-vm@v0 to vmactions/freebsd-vm@v1 in .github/workflows/build-test.yml.

Also done.

Additionally, to make the CI happy can you adjust the patch to work similarly to this https://github.com/eclipse-iceoryx/iceoryx/blob/release_2.0/cmake/googletest/CMakeLists.txt#L73-L82

I don't think that is the correct logic for this patch. In particular, we want to apply this on Windows as well, as with a newer version of CMake there, we will have the same warning.

elBoberido commented 7 months ago

@clalancette the same logic can be used to apply the patch on Windows. I'm not sure anymore why we skipped Windows with gTest.

The current proposal has the drawback that one cannot call cmake -B build -Hiceoryx_meta multiple times, e.g. when changing some compile time options, without cleaning the whole build directory. The logic we use for gTest only generates a warning instead of an error on subsequent calls.

Can you adapt this PR similarly to #2247? I set the version to v3.16 since that's what we require anyway.

I guess you would also like to see a v2.0.6 release after this is merged, right?

clalancette commented 7 months ago

Sorry for the delay here, just picking this back up.

Thanks for doing that work on master, that made my life really easy. This now just becomes a backport of that. With that in mind, please see 94224d0.

I guess you would also like to see a v2.0.6 release after this is merged, right?

Yes, that would be fantastic, thank you.

elBoberido commented 7 months ago

@clalancette is there a deadline by when you need the v2.0.6 release?

I also just noticed that the last commit does not start with the issue number. Can you please amend the commit message and force push?

clalancette commented 7 months ago

I also just noticed that the last commit does not start with the issue number. Can you please amend the commit message and force push?

Fixed now.

@clalancette is there a deadline by when you need the v2.0.6 release?

Earlier is better, but the drop-dead date is in about 4 weeks.

elBoberido commented 7 months ago

It seems there are issues with the FreeBSD action and the codecov upload. I'll merge and take care of these issues in a follow up PR since they are not related to your changes

clalancette commented 7 months ago

It seems there are issues with the FreeBSD action and the codecov upload. I'll merge and take care of these issues in a follow up PR since they are not related to your changes

Thank you, it is much appreciated!

elBoberido commented 7 months ago

@clalancette I was just pinged by another iceoryx committer that they got some emails from the ROS buildfarm after the merge. They are not quite sure if something broke but I also do not have access to those mails. Can you confirm that this merge did not break anything?

Crola1702 commented 7 months ago

Hey! I think this PR triggered a warning in ROS2 CI:

Reference build:

In iceoryx_posh output:

[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

CC: @clalancette

elBoberido commented 7 months ago

@Crola1702 this should only happen it the patch is already applied. I think I can fix this. Will create a PR to check if the patch can be reversed to check if it is already applied. If if fails, the patch will be applied.

cc @clalancette