maint: Enable `-Werror` compiler flag for GCC, Clang and AppleClang #3611

mathbunnyru · 2024-11-18T18:06:06Z

I think we can now enable -Werror flag relatively easily when building with GCC, Clang and AppleClang.

I tested locally on my M2 Mac with the following compilers:

Apple clang version 16.0.0 (clang-1600.0.26.4)
clang version 17.0.6
gcc-14 (Homebrew GCC 14.2.0_1) 14.2.0

In some places I turned errors back to warnings, in some - disabled, I will explain choices using commenting.

mathbunnyru · 2024-11-18T18:10:51Z

libmamba/CMakeLists.txt

+    # This file uses capturing structured bindings, which was fixed in C++20
+    set_source_files_properties(
+        ${LIBMAMBA_SOURCE_DIR}/download/mirror_impl.cpp
+        PROPERTIES COMPILE_FLAGS -Wno-c++20-extensions


There is nothing wrong with our code and nothing to change there. So we can safely ignore the warning we're using C++20 feature (capturing structured binding).
This can't hide any real problems, so here this warning/error will disappear.

Technically, this construction is C++20, not C++17. It works with gcc and msvc though, and the warning is still here with Clang, even if we pass the C++20 option (that's a known bug), so I agree that we can ignore it.

mathbunnyru · 2024-11-18T18:12:16Z

libmamba/CMakeLists.txt

+    # GCC reports dangling reference when using std::invoke with data members
+    set_source_files_properties(
+        ${LIBMAMBA_SOURCE_DIR}/solver/problems_graph.cpp
+        PROPERTIES COMPILE_FLAGS -Wno-error=dangling-reference


This might be useful warning/error, so I'm turning it back to being warning for this file.

Is there any way to make that change as part of the source code instead of here (like disabling warnings)?

Yes, fortunately it happens in functions located near each other, so we can only write pragmas once.
Thanks!

mathbunnyru · 2024-11-18T18:14:35Z

libmamba/include/mamba/util/conditional.hpp

-            using int_t = Int;
-            return static_cast<int_t>(condition) * true_val
-                   + (int_t(1) - static_cast<int_t>(condition)) * false_val;
+            return condition ? true_val : false_val;


I think this is significantly easier to read and doesn't deal with arithmetics at all (which was giving warnings in some cases).

libmamba/src/core/activation.cpp

mathbunnyru · 2024-11-18T18:17:31Z

libmamba/src/core/thread_utils.cpp

+// At the same time `sigset_t` might be `unsigned int`
+// This causes compiler warning
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wsign-conversion"


I added comment explaining why this happens above
This doesn't happen in our code, but in the system code
But because sigaddset might be implemented as a macro, it's treated as our code.

Because, there is no bug, I completely disable this warning/error.
But I didn't want to disable it for the whole file, so I created a small function, where I applied this behaviour.

mathbunnyru · 2024-11-18T18:17:56Z

libmamba/src/solver/libsolv/matcher.cpp

@@ -193,7 +193,7 @@ namespace mamba::solver::libsolv
            return { std::cref(it->second) };
        }

-        return specs::Channel::resolve(std::move(uc), channel_params())
+        return specs::Channel::resolve(uc, channel_params())


It's const specs::UnresolvedChannel& uc, so no point in std::move(uc)

mathbunnyru · 2024-11-18T18:18:50Z

libmambapy/CMakeLists.txt

+    # This file uses capturing structured bindings, which was fixed in C++20
+    set_source_files_properties(
+        src/libmambapy/bindings/legacy.cpp
+        PROPERTIES COMPILE_FLAGS -Wno-error=deprecated-declarations


I turned this back to warning. Sometimes it's useful to see when we're using deprecated functions, so let's have it as a warning.

It makes sense for that code or even the whole library. 👍🏽

For now, let's keep it for this file. If needed later, we can add this flag to the whole repo/lib I suppose.
It's better not to use deprecated functions, when possible.

mathbunnyru · 2024-11-18T18:31:27Z

libmamba/ext/solv-cpp/src/pool.cpp

+// This function is only used in `assert()` expressions
+// That's why it might get reported as unused in Release builds
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-function"


I explained above why this happens.
It's completely ok to have a function which is only used in assert statements.
So, I silence this warning/error

mathbunnyru · 2024-11-18T20:44:52Z

cmake/WrongCompilerOptions.cmake

+if(APPLE)
+    # It seems that there are some flags added to CXXFLAGS during `conda-build`. It might be a bug
+    # in https://github.com/conda-forge/clang-compiler-activation-feedstock. One way to fix this is
+    # to turn these errors to warnings or silence them (using `-Wno-unused-command-line-argument`
+    # and `-Wno-ignored-optimization-argument`)
+
+    # But we will go another way: we will remove wrong options from CMAKE_CXX_FLAGS


This is the most tricky part.

I think we have some flags, which are not part of this project and we inherit it during conda-build.

This is how current main looks like: https://github.com/mamba-org/mamba/actions/runs/11896974442/job/33150163684#step:7:1203

x86_64-apple-darwin13.4: warning: -framework CoreFoundation: 'linker' input unused [-Wunused-command-line-argument] x86_64-apple-darwin13.4: warning: -framework CoreServices: 'linker' input unused [-Wunused-command-line-argument] x86_64-apple-darwin13.4: warning: -framework Security: 'linker' input unused [-Wunused-command-line-argument] x86_64-apple-darwin13.4: warning: -framework Kerberos: 'linker' input unused [-Wunused-command-line-argument] x86_64-apple-darwin13.4: warning: optimization flag '-fno-merge-constants' is not supported [-Wignored-optimization-argument]

I think it comes from here INFO: activate_clangxx_osx-64.sh made the following environmental changes::
https://github.com/mamba-org/mamba/actions/runs/11896974442/job/33150163684#step:7:1123

I'm not 100% sure about this and might be completely wrong.
If someone more experienced could take a look at this, it would be great.

Trying another approach right now

It worked; this one didn't due to some strange linking errors.

jjerphan

LGTM, modulo a few comments.

jjerphan · 2024-11-20T13:47:24Z

cmake/CompilerWarnings.cmake

+    # It seems that there are some wrong flags added to CXXFLAGS during `conda-build`. And then
+    # these flags are used to compile our source code. It might be a bug in
+    # https://github.com/conda-forge/clang-compiler-activation-feedstock.


Can you give more details, context or evidence about this?

I rewrote the comment and added more details.

Note, this is not caused by my PR, it already happens in main
But because we use warnings, not errors, no one noticed:
https://github.com/mamba-org/mamba/actions/runs/11935345040/job/33266465568#step:7:1202

Unfortunately, I couldn't understand exactly why this happens.
But I noticed INFO: activate_clangxx_osx-64.sh made the following environmental changes: a few lines above.
And then we have lines showing erroneous changes in CXXFLAGS.
That's why I think it happens in the mentioned lib.

If someone wants to dig deeper, that would be great.

jjerphan · 2024-11-20T14:21:32Z

CMakeLists.txt

+if(MSVC)
+    option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" OFF)
+else()
+    option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" ON)


I think this is a good idea even if this will probably will make some untested toolchain fail.

mathbunnyru · 2024-11-20T16:38:37Z

libmamba/ext/solv-cpp/src/pool.cpp

@@ -93,10 +93,21 @@ namespace solv

    namespace
    {
+// This function is only used in `assert()` expressions
+// That's why it might get reported as unused in Release builds
+#if defined(__GNUC__) && !defined(__clang__)


I added these #if-like statements everywhere - warnings only need to be ignored if they actually happen.
And it also prevents MSVC from giving a warning that unknown pragma is used.

mathbunnyru · 2024-11-20T18:53:35Z

@jjerphan @Klaim I think I fixed all the comments (and a bit more), so please, take a look.

Klaim

LGTM 👍🏽

jjerphan

LGTM.

jjerphan · 2024-11-21T12:48:29Z

cmake/CompilerWarnings.cmake

+    # It seems that these flags are leaked to CXXFLAGS: `-framework CoreFoundation` `-framework
+    # CoreServices` `-framework Security` `-framework Kerberos` `-fno-merge-constants`


How is this comment related to the elements bellow? Is it when you build locally on macOS? Can you precise where those are leaked in #3624 (comment)?

See also those lines on the feedstock which make use of them.

The use and presence of those flags must be clarified at a point, but this is orthogonal to the goal of this PR to me.

mathbunnyru added 2 commits November 18, 2024 18:00

maint: Enable -Werror compiler flag for GCC, Clang and AppleClang

297600d

Do not show this even as warning

1139c2b

mathbunnyru commented Nov 18, 2024

View reviewed changes

libmamba/src/core/activation.cpp Outdated Show resolved Hide resolved

mathbunnyru commented Nov 18, 2024

View reviewed changes

Fix for unused function only used in assert() statements

b8ca583

mathbunnyru commented Nov 18, 2024

View reviewed changes

mathbunnyru added 2 commits November 18, 2024 18:58

Mark one more function as -Wunused-function

f31b567

Fix -Wunused-parameter

13a1aaf

mathbunnyru commented Nov 18, 2024

View reviewed changes

Try to fix wrong compiler options

e072216

mathbunnyru force-pushed the gcc_clang_werror branch 2 times, most recently from c24700c to 5e77197 Compare November 18, 2024 21:35

Try another approach

c663393

mathbunnyru force-pushed the gcc_clang_werror branch from 5e77197 to c663393 Compare November 18, 2024 21:46

Remove approach which didn't work

4fb6ee9

jjerphan added the release::bug_fixes For PRs fixing bugs label Nov 20, 2024

jjerphan reviewed Nov 20, 2024

View reviewed changes

mathbunnyru added 6 commits November 20, 2024 15:46

Merge branch 'main' into gcc_clang_werror

9c5d213

Add more details

85354bc

Better text

d62357e

Move some ignores to the code

f69c782

Only define pragma on GCC

f7c4977

Only disable compiler warnings when needed

37959d8

mathbunnyru commented Nov 20, 2024

View reviewed changes

Fix

fda8aae

mathbunnyru added 4 commits November 20, 2024 16:52

Fix

13accfa

Fix

0faf603

Merge branch 'main' into gcc_clang_werror

12d8c9f

Fix

a0a8cea

mathbunnyru closed this Nov 20, 2024

mathbunnyru reopened this Nov 20, 2024

Klaim approved these changes Nov 21, 2024

View reviewed changes

jjerphan approved these changes Nov 21, 2024

View reviewed changes

jjerphan mentioned this pull request Nov 21, 2024

build: Clarify the use of macOS specific flags #3624

Open

jjerphan merged commit 0a8128d into mamba-org:main Nov 21, 2024
63 of 64 checks passed

jjerphan mentioned this pull request Nov 21, 2024

fix: support homebrew/linuxbrew (AppleClang, GCC 11) #3613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maint: Enable `-Werror` compiler flag for GCC, Clang and AppleClang #3611

maint: Enable `-Werror` compiler flag for GCC, Clang and AppleClang #3611

mathbunnyru commented Nov 18, 2024 •

edited

Loading

mathbunnyru Nov 18, 2024

JohanMabille Nov 21, 2024 •

edited

Loading

mathbunnyru Nov 18, 2024

Klaim Nov 20, 2024

mathbunnyru Nov 20, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

Klaim Nov 20, 2024

mathbunnyru Nov 20, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

mathbunnyru Nov 18, 2024

jjerphan left a comment

jjerphan Nov 20, 2024

mathbunnyru Nov 20, 2024

jjerphan Nov 20, 2024

mathbunnyru Nov 20, 2024

mathbunnyru commented Nov 20, 2024

Klaim left a comment

jjerphan left a comment

jjerphan Nov 21, 2024 •

edited

Loading

		# It seems that these flags are leaked to CXXFLAGS: `-framework CoreFoundation` `-framework
		# CoreServices` `-framework Security` `-framework Kerberos` `-fno-merge-constants`

maint: Enable -Werror compiler flag for GCC, Clang and AppleClang #3611

maint: Enable -Werror compiler flag for GCC, Clang and AppleClang #3611

Conversation

mathbunnyru commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

JohanMabille Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathbunnyru commented Nov 20, 2024

Klaim left a comment

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

jjerphan Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

maint: Enable `-Werror` compiler flag for GCC, Clang and AppleClang #3611

maint: Enable `-Werror` compiler flag for GCC, Clang and AppleClang #3611

mathbunnyru commented Nov 18, 2024 •

edited

Loading

JohanMabille Nov 21, 2024 •

edited

Loading

jjerphan Nov 21, 2024 •

edited

Loading