Remove -Werror from Clang compile options

[wjwwood]

Description

While useful, in my opinion it's not appropriate to set -Werror the CMakeLists.txt, instead allowing the user to set it if they wish, or setting it during CI if you want to catch warnings during testing.

Is this user-facing behavior change?

No

Did you use Generative AI?

No

Additional Information

This is the warning that was being turned into an error by -Werror:

/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:57:12: warning: 'guarded_by' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'std::mutex' [-Wthread-safety-attributes]
   57 |   int data RCPPUTILS_TSA_GUARDED_BY(mu);
      |            ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/include/rcpputils/thread_safety_annotations.hpp:93:46: note: expanded from macro 'RCPPUTILS_TSA_GUARDED_BY'
   93 |   RCPPUTILS_THREAD_ANNOTATION_ATTRIBUTE_IMPL(guarded_by(x))
      |                                              ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:59:14: warning: 'requires_capability' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'std::mutex' [-Wthread-safety-attributes]
   59 |   int incr() RCPPUTILS_TSA_REQUIRES(mu) {
      |              ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/include/rcpputils/thread_safety_annotations.hpp:141:46: note: expanded from macro 'RCPPUTILS_TSA_REQUIRES'
  141 |   RCPPUTILS_THREAD_ANNOTATION_ATTRIBUTE_IMPL(requires_capability(__VA_ARGS__))
      |                                              ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:152:12: warning: reading variable 'data' requires holding mutex 'mu' [-Wthread-safety-analysis]
  152 |     return data;
      |            ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:192:11: warning: writing variable 'data' requires holding mutex 'guarded.mu' exclusively [-Wthread-safety-analysis]
  192 |   guarded.data++;
      |           ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:198:13: warning: writing variable 'data' requires holding mutex 'guarded.mu' exclusively [-Wthread-safety-analysis]
  198 |     guarded.data++;
      |             ^
/Users/wjwwood/ros2_ws/src/ros2/rcpputils/test/test_thread_safety_annotations.cpp:199:13: warning: calling function 'incr' requires holding mutex 'guarded.mu' exclusively [-Wthread-safety-analysis]
  199 |     guarded.incr();
      |             ^
6 warnings generated.

I'll perhaps fix that in a follow up pr.

[fujitatomoya]

how about adding the code that suppress those warnings with this PR?

[emersonknapp]

Fair, but can we set it in CI here? I do prefer to put this setting on as a merge requirement to increase code quality.

I do that in internal packages by putting it in cmake - it forces devs to deal with it before getting tripped up by ci not passing when their local build does. It's maybe over strict but less experienced folks often get confused with the difference.

[ahcorde]

Pulls: #220
Gist: https://gist.githubusercontent.com/ahcorde/58981c4883169d7b7ee7a873cf8dfc83/raw/ffc5e12e0254a03af999f54765f369befaeabe9c/ros2.repos
BUILD args: --packages-above-and-dependencies rcpputils
TEST args: --packages-above rcpputils
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17389

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[wjwwood]

how about adding the code that suppress those warnings with this PR?

Sorry, I didn't follow up here sooner. I'll try to fix them and open a follow up pr.

I do think merging this first makes sense because otherwise ros 2 cannot be compiled on macOS right now due just a warning.

[wjwwood]

Fair, but can we set it in CI here? I do prefer to put this setting on as a merge requirement to increase code quality.

I do that in internal packages by putting it in cmake - it forces devs to deal with it before getting tripped up by ci not passing when their local build does. It's maybe over strict but less experienced folks often get confused with the difference.

With CI I think you also want to avoid this flag because you want to see if it compiles and also collect all of the warnings. If you set this then it will stop at the first warning. We can configure CI to be yellow (or red for GitHub actions) if there are any warnings. We already do this with ci.ros2.org.

Personally I think the only appropriate place for this flag is on a developer's shell. But I do understand why you do that in the cmake of other projects.

[wjwwood]

And to be clear we normally don't merge anything with c++ warnings but we also only see this warning on macOS which we don't test in ci.

[emersonknapp]

If you set this then it will stop at the first warning. We can configure CI to be yellow (or red for GitHub actions) if there are any warnings.

Do you mean the first package within a larger multi-package build? That's a fair point, to let the full build tree of packages play out. Or is it the first source file compilation within a package build that has warnings, which will stop the make from continuing on to the rest?

I know this is a tangent on this issue but I'll ask anyay:

Like you suggest, the "error" case that I am really aiming at in a CI context is the aggregate result of "were there any compiler warnings", not necessarily that the first package/file failing should block everything else from building. Do you know the way get the warning info after the full colcon build is complete, to fail GitHub Action builds? It's not a workflow I have built/documented for https://github.com/ros-tooling/action-ros-ci