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

Update MacOS Runner #6580

Merged
merged 87 commits into from
Jul 21, 2024
Merged

Conversation

CoolSpy3
Copy link
Contributor

@CoolSpy3 CoolSpy3 commented Jul 10, 2024

Description
GitHub recently deprecated the macos-11 runner, so all of the tests which rely on that os are failing. This PR updates the runner to the latest macos-14 macos-13 image. (There's some bug installing cppcheck on macos-14. I might play around with bumping the cppcheck version later, but I don't have enough info to resolve it as-is.)

Hopefully, most of the workflow can be ported without too much hassle, however, if that proves to not be the case, I may have to leave this fix for someone else, as I do not have a mac to debug on.

@CoolSpy3 CoolSpy3 added test suite issue Test failure in the test suite CI test distribution Start the distribution test test suite Start the test suite test sources Start the sources test on all platforms test webots build Start the build tests labels Jul 10, 2024
@CoolSpy3
Copy link
Contributor Author

The build passes! I'll come back tomorrow and see if I can get cppcheck working. If not, I'll just revert to this commit.

SIde note: Even though the build runs on macos 13, it still targets macos 11. I assume that we want to keep that as-is because we do not want to drop old support unless it becomes necessary.

@omichel
Copy link
Member

omichel commented Jul 10, 2024

No, the plan is to support only the maintained versions macOS, that are currently 12 (Monterey), 13 (Ventura) and 14 (Sonoma). So we should drop macOS 11 Big Sur and update the requirements documentation accordingly.

@CoolSpy3 CoolSpy3 marked this pull request as ready for review July 21, 2024 00:26
@CoolSpy3 CoolSpy3 requested a review from a team as a code owner July 21, 2024 00:26
@CoolSpy3
Copy link
Contributor Author

We've done it! This PR is ready for review!
A summary of the changes:

  • Updated the macos runner to version 14
    • Once this PR is merged, I can migrate this change to the wiki
  • Removed support for macos 11
  • Bumped cppcheck to version 2.14.1
  • Updated the system requirements to reflect the above
  • Updated the changelog

Notes on reviewing/merging:

  • It may be helpful to review just the config changes (also Serial.cpp) and just the warning fixes individually. Luckily, the warning fixes are all very small; there's just a lot of them.
    • You can also review them commit-by-commit, which could make it easier to review some smaller changes separate from the ones which just add const in front of everything.
  • In order to pass the merge requirments, the required macos-11 "Test Sources" check should be replaced with the new macos-14 variant.

omichel
omichel previously approved these changes Jul 21, 2024
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Very good job! Thank you!
I could review all the file changes as per your instructions and everything looks very good to me.
I like your approach of adding some cppcheck-suppress comments instead of trying to fix the problem when it was not super obvious to fix as it minimizes the risk of introducing some bugs, while still highlighting possible improvements.
I just replaced the mandatory macos-11 "Test Sources" check with the new macos-14 one.
I just have one minor suggestion, but you can ignore it if you prefer.

Co-authored-by: Olivier Michel <[email protected]>
@CoolSpy3 CoolSpy3 merged commit ac8a683 into cyberbotics:master Jul 21, 2024
21 of 22 checks passed
@CoolSpy3 CoolSpy3 deleted the update-macos-runner branch July 29, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test distribution Start the distribution test test sources Start the sources test on all platforms test suite issue Test failure in the test suite CI test suite Start the test suite test webots build Start the build tests
Development

Successfully merging this pull request may close these issues.

2 participants