Makefile: fixed and cleaned up MinGW/MSYS2/Cygwin handling

[firewave]

This is a follow-up on the discussion with @chrfranke in #4229.

I manually tested the build on all their shell variations as well as using the compiler through the regular command-line if possible.

[firewave]

This is basically ready to review but I want feedback from @chrfranke first as he inspired these changes.

[chrfranke]

Sorry for the delay. Issues found:

• Only ComSpec is checked which would only set WINNT=1 in (possibly rare) case that make is run from cmd or powershell. MSYS(2) and Cygwin are not detected as COMSPEC is not checked, but ...
• ... some Cygwin or MinGW builds work because the conditional ifneq ($(MSYSTEM),MINGW32 MINGW64) is always true, so RDYNAMIC=-rdynamic is never set and LDFLAGS+=-lshlwapi is always set on any platform.

On current MSYS2 the MSYSTEM variable might be one of MINGW32 MINGW64 UCRT64 CLANG64 but never "MINGW32 MINGW64". But it is not needed to additionally check this variable if WINNT is set properly.

Proposed quick Makefile only fix on top of this PR:

--- Makefile.orig	2023-05-05 16:14:26.667306500 +0200
+++ Makefile	2023-05-05 16:14:39.830069600 +0200
@@ -45,6 +45,13 @@
 endif
 
 # COMSPEC is defined when msys2 or cygwin shell is being used
+ifdef COMSPEC
+    ifeq ($(VERBOSE),1)
+        $(info COMSPEC found)
+    endif
+
+    WINNT=1
+endif # COMSPEC
 # ComSpec is defined when cmd or ps (PowerShell) shell is being used
 ifdef ComSpec
     ifeq ($(VERBOSE),1)
@@ -62,19 +69,11 @@
     endif
 
     LDFLAGS+=-pthread
-endif # !WINNT
-
-
-ifeq ($(VERBOSE),1)
-    $(info MSYSTEM=$(MSYSTEM))
-endif
-
-ifneq ($(MSYSTEM),MINGW32 MINGW64)
-    RDYNAMIC=
-    LDFLAGS+=-lshlwapi
-else
     RDYNAMIC=-rdynamic
-endif
+else
+    LDFLAGS+=-lshlwapi
+    RDYNAMIC=
+endif # WINNT
 
 ifndef CXX
     CXX=g++

With your PR + the above fix, I tested the following targets and build environments successfully:

Target: Native Win32-x64 cppcheck.exe (using MS Runtime): Environments: MSYS2 MINGW64: make all MSYS2 UCRT64 (uses UCRT instead of MSVCRT): make all MSYS2 CLANG64: make CXX=clang++ all Cygwin (with Mingw-w64 cross compiler): make CXX=x86_64-w64-mingw32-g++ all

Target: Cygwin cppcheck.exe (using cygwin1.dll POSIX emulation): Environment Cygwin: make all

I did not test whether this breaks builds on other platforms.

Two further unrelated notes:

• The uname_S variable is set but not used unless VERBOSE is set.

• The build shows some [-Wreturn-type] warnings. These should not be ignored since g++ >= 8, because the program might segfault then, see https://gcc.gnu.org/gcc-8/porting_to.html https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96181 Adding -Werror=return-type if g++ is used is recommended.

[firewave]

No problem. Thanks a lot for the great and thorough feedback! Much appreciated.

I tested this with

MinGW (native Windows) Cygwin GCC (MSYS2)

CMD Powershell Cygwin Shell MSYS2

And it was working fine on all of them - but it is possible I introduced bugs since then (been a few months) or possibly shelved a wrong version. I will do my tests again within the next few days and document exactly what I tested.

• Only ComSpec is checked which would only set WINNT=1 in (possibly rare) case that make is run from cmd or powershell. MSYS(2) and Cygwin are not detected as COMSPEC is not checked

Strange. That was done intentionally as it turned out unnecessary and make things more complex. Need to re-test myself.

On current MSYS2 the MSYSTEM variable might be one of MINGW32 MINGW64 UCRT64 CLANG64 but never "MINGW32 MINGW64"

Stupid mistake from having this sitting on the shelf too long.

• The uname_S variable is set but not used unless VERBOSE is set.

Already recognized that myself a while ago and wanted to clean that up when I finish this up.

• The build shows some [-Wreturn-type] warnings.

Those are harmless and it cannot happen - MSVC and GCC report them but Clang realizes it's impossible. I still plan to address them within another PR at some point.

[chrfranke]

You're welcome. Further unrelated notes:

• The default PREFIX should IMO be /usr/local to avoid accidental overwriting an installed package.

• Using override is not recommended as it would require patching the Makefile itself if special settings are needed, for example for cross-compiling. If someone sets for example CXXFLAGS in the make command line, the variable should be used unchanged without always appending -std=gnu++0x -pipe or so.

• Many ifndef ... endif are not needed due to the special semantics of make assignments. If a variable is set in environment or in make command line, all non-override assignments to this variable are disabled. This ...

ifndef VERBOSE
    VERBOSE=
endif
ifndef HAVE_RULES
    HAVE_RULES=no
endif
ifndef MATCHCOMPILER
    MATCHCOMPILER=
endif
ifndef CXX
    CXX=g++
endif

... is the same as ....

VERBOSE=
HAVE_RULES=no
MATCHCOMPILER=
CXX=g++

and the same as ...

HAVE_RULES=no
CXX=g++

[firewave]

Sorry for the late reply. Will still take a while until I finish this up since I can't be bothered to do any extensive testing at the moment.

• The default PREFIX should IMO be /usr/local to avoid accidental overwriting an installed package.

I have no idea if the install actually works properly and is actually used. I think the maintainers use CMake. See also https://trac.cppcheck.net/ticket/8659.

• Using override is not recommended as it would require patching the Makefile itself if special settings are needed, for example for cross-compiling. If someone sets for example CXXFLAGS in the make command line, the variable should be used unchanged without always appending -std=gnu++0x -pipe or so.

I never properly checked this. And I keep forgetting the Makefile syntax.

• Many ifndef ... endif are not needed due to the special semantics of make assignments. If a variable is set in environment or in make command line, all non-override assignments to this variable are disabled. This ...

I was verbose about that since I didn't get around to checking it (and see above). See also your ticket https://trac.cppcheck.net/ticket/11825.

I will most likely address these in a separate PR.

[firewave]

Sorry for the late reply.

Here's my test matrix with output:

MSYS2 MinGW x86
uname_S=MINGW32_NT-10.0-19045
MSYSTEM=MINGW32

MSYS2 MinGW x64
uname_S=MINGW64_NT-10.0-19045
MSYSTEM=MINGW64

MSYS2 MinGW Clang x64
uname_S=MINGW64_NT-10.0-19045
MSYSTEM=CLANG64

MSYS2 MinGW UCRT x64
uname_S=MINGW64_NT-10.0-19045
MSYSTEM=UCRT64

Cygwin
uname_S=CYGWIN_NT-10.0-19045
MSYSTEM=

cmd.exe MinGW
ComSpec found
MSYSTEM=

PowerShell MinGW
ComSpec found
MSYSTEM=

It builds on all those platforms with the current version of this PR. So that confirms my earlier tests that there is no need to handle COMSPEC at all.

Update: Interestingly it also seems there is no need for -rdynamic at all as it is never added because of the broken MSYSTEM check.

[firewave]

Update: Interestingly it also seems there is no need for -rdynamic at all as it is never added because of the broken MSYSTEM check.

Ah, okay. The backtrace() call in the source needs this. Will add a better comment about it.

On current MSYS2 the MSYSTEM variable might be one of MINGW32 MINGW64 UCRT64 CLANG64 but never "MINGW32 MINGW64". But it is not needed to additionally check this variable if WINNT is set properly.

This was actually wrong before my check so it never worked properly.