View Issue Details

IDProjectCategoryView StatusLast Update
0001606OpenMPTBuild Systempublic2022-06-24 05:23
Reporter1480c1 Assigned Tomanx  
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindowsOS Version11
Product VersionOpenMPT 1.31.00.* (current testing) 
Target VersionOpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first)Fixed in VersionOpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first) 
Summary0001606: Cannot build with clang on windows since the makefile override CC and CXX
Description

Both

build/make/config-mingw64-win64.mk

and

build/make/config-mingw64-win32.mk

override CC and CXX to gcc and g++ and does not check if they have been provided through environment variables or command line, meaning it is not possible to use clang.

I personally use a patch like https://github.com/m-ab-s/mabs-patches/blob/master/openmpt/0002-mingw-w64.mk-don-t-override-the-compilers-etc-if-pro.patch to ensure that it does not override CC if set.

Patch attached

Steps To Reproduce

clone, export CC=clang, add a clang specific flag to CFLAGS or LDFLAGS like export LDFLAGS="--start-no-unused-arguments -static-libstdc++ --end-no-unused-arguments", and compile

Additional Information

using msys2 with clang from the mingw64 subsystem rather than clang64

TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)latest commit

Activities

1480c1

1480c1

2022-06-20 22:50

reporter  

0002-mingw-w64.mk-don-t-override-the-compilers-etc-if-pro.patch (1,904 bytes)   
From 9ebfb9bd745cc042b7317de7016e44c88ae2e26d Mon Sep 17 00:00:00 2001
From: Christopher Degawa <ccom@randomderp.com>
Date: Thu, 19 May 2022 23:24:48 -0500
Subject: [PATCH 2/2] mingw-w64.mk: don't override the compilers etc if
 provided

Also, inherit LD from CXX rather than manually setting it to g++

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
---
 build/make/config-mingw64-win32.mk | 9 +++++----
 build/make/config-mingw64-win64.mk | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/build/make/config-mingw64-win32.mk b/build/make/config-mingw64-win32.mk
index d1c579bae..4e41c607d 100644
--- a/build/make/config-mingw64-win32.mk
+++ b/build/make/config-mingw64-win32.mk
@@ -1,8 +1,9 @@
 
-CC  = i686-w64-mingw32-gcc$(MINGW_FLAVOUR)
-CXX = i686-w64-mingw32-g++$(MINGW_FLAVOUR)
-LD  = i686-w64-mingw32-g++$(MINGW_FLAVOUR)
-AR  = i686-w64-mingw32-ar$(MINGW_FLAVOUR)
+MINGW_ARCH := i686
+CC  := $(or $(CC),$(MINGW_ARCH)-w64-mingw32-gcc$(MINGW_FLAVOUR))
+CXX := $(or $(CXX),$(MINGW_ARCH)-w64-mingw32-g++$(MINGW_FLAVOUR))
+LD  := $(CXX)
+AR  := $(or $(AR),$(MINGW_ARCH)-w64-mingw32-ar$(MINGW_FLAVOUR))
 
 CXXFLAGS_STDCXX = -std=c++17
 CFLAGS_STDC = -std=c99
diff --git a/build/make/config-mingw64-win64.mk b/build/make/config-mingw64-win64.mk
index fc4f8eaf3..6c85d2ba2 100644
--- a/build/make/config-mingw64-win64.mk
+++ b/build/make/config-mingw64-win64.mk
@@ -1,8 +1,9 @@
 
-CC  = x86_64-w64-mingw32-gcc$(MINGW_FLAVOUR)
-CXX = x86_64-w64-mingw32-g++$(MINGW_FLAVOUR)
-LD  = x86_64-w64-mingw32-g++$(MINGW_FLAVOUR)
-AR  = x86_64-w64-mingw32-ar$(MINGW_FLAVOUR)
+MINGW_ARCH := x86_64
+CC  := $(or $(CC),$(MINGW_ARCH)-w64-mingw32-gcc$(MINGW_FLAVOUR))
+CXX := $(or $(CXX),$(MINGW_ARCH)-w64-mingw32-g++$(MINGW_FLAVOUR))
+LD  := $(CXX)
+AR  := $(or $(AR),$(MINGW_ARCH)-w64-mingw32-ar$(MINGW_FLAVOUR))
 
 CXXFLAGS_STDCXX = -std=c++17
 CFLAGS_STDC = -std=c99
-- 
2.36.1

manx

manx

2022-06-21 09:13

administrator   ~0005209

Thanks for the report. There are clearly things that can be improved here.

With the current state, you still can override CC and friends by passing it as an option to the make invocation: make CONFIG=mingw64-win32 CC=..., but not with environment variables. I think taking your suggestions makes sense here. It probably is also useful for all other CONFIG=.

Are you aware that we also provide release tarballs with an Autotools based build system? Could this be preferable for your use case? Sadly, Autotools currently cannot be used to build from the repository directly (I hope to fix this in the future).

I also see you are using another patch which allows overriding pkg-config (<https://github.com/m-ab-s/mabs-patches/blob/master/openmpt/0001-make-try-using-PKG_CONFIG-if-provided.patch>). I think this would also be clearly useful for us.

manx

manx

2022-06-21 10:27

administrator   ~0005210

Your suggestion does not work though: CC and related variables have default values if not set by any other means, thus the make or function always considers them set.

manx

manx

2022-06-21 11:55

administrator   ~0005211

I implemented something that hopefully works in r17547 (trunk) and r17548 (0.6):

We unset these builtin variables before loading the config (which always wants to set them). That way, we can assign them via CC ?= in build/make/* and get the desired result in all cases.

manx

manx

2022-06-21 12:23

administrator   ~0005213

I also see you are using another patch which allows overriding pkg-config (<https://github.com/m-ab-s/mabs-patches/blob/master/openmpt/0001-make-try-using-PKG_CONFIG-if-provided.patch>). I think this would also be clearly useful for us.

Implemented in r17549 (trunk) r17550 (0.6).

manx

manx

2022-06-21 12:24

administrator   ~0005214

Can you please verify that what I implemented does indeed work for you?

1480c1

1480c1

2022-06-21 14:55

reporter   ~0005217

Are you aware that we also provide release tarballs with an Autotools based build system? Could this be preferable for your use case? Sadly, Autotools currently cannot be used to build from the repository directly (I hope to fix this in the future).

For openmpt, my build script pulls the latest tag matching libopenmpt-* from https://github.com/OpenMPT/openmpt.git (just so it doesn't have to worry about svn), so it technically uses a release, but a tag of it though.

As a side question, does the master branch of that mirror correlate to the trunk? I do see that the git branch contains the revisions you mentioned already and has the same layout as the latest libopenmpt-* tag <https://github.com/OpenMPT/openmpt/tree/libopenmpt-0.6.4>.

Your suggestion does not work though: CC and related variables have default values if not set by any other means, thus the make or function always considers them set.

Seems you have found out my lack of autotools/makefile related skills, most of that was based on suggestions from a stackoverflow post

Since I use the tag version, I instead used a cherry-picked version of your revisions https://github.com/1480c1/openmpt/commit/2a02d3152 and https://github.com/1480c1/openmpt/commit/a6b824fe2

Using those two, compilation with just the env variable set with CC and CXX not set on make results in:

[AR] bin/libopenmpt.a
x86_64-w64-mingw32-g++.exe: error: unrecognized command-line option '--start-no-unused-arguments'
x86_64-w64-mingw32-g++.exe: error: unrecognized command-line option '--end-no-unused-arguments'
make: *** [Makefile:1712: bin/libopenmpt.dll] Error 1
make: *** Waiting for unfinished jobs....
+ [[ -n -j8 ]]
+ make -j1 install PREFIX=/local64 CONFIG=mingw64-win64 AR=ar STATIC_LIB=1 EXAMPLES=0 OPENMPT123=0 TEST=0 OS=
[LD] bin/libopenmpt.dll
x86_64-w64-mingw32-g++.exe: error: unrecognized command-line option '--start-no-unused-arguments'
x86_64-w64-mingw32-g++.exe: error: unrecognized command-line option '--end-no-unused-arguments'
make: *** [Makefile:1712: bin/libopenmpt.dll] Error 1

Using make -j8 install PREFIX=/local64 CONFIG=mingw64-win64 AR=ar STATIC_LIB=1 EXAMPLES=0 OPENMPT123=0 TEST=0 OS= 'CC=ccache clang' 'CXX=ccache clang++' to set them does succeed, so I will just go ahead and set them on make.

I did go ahead and try the master branch, and I seem to get some errors with it, but I am assuming those are unrelated to this issue, I will try to see if that's a configuration issue or something and open a new issue for it

[CXX] common/mptFileType.cpp
clang++: warning: argument unused during compilation: '-mconsole' [-Wunused-command-line-argument]
clang++: warning: argument unused during compilation: '-mthreads' [-Wunused-command-line-argument]
clang++: warning: argument unused during compilation: '-mconsole' [-Wunused-command-line-argument]
clang++: warning: argument unused during compilation: '-mthreads' [-Wunused-command-line-argument]
In file included from common/ComponentManager.cpp:11:
In file included from common/stdafx.h:68:
src/mpt/check/libc.hpp:38:10: error: invalid token at start of a preprocessor expression
#if (_MT != 1)
         ^
1480c1

1480c1

2022-06-21 14:59

reporter   ~0005218

As a quick note to append, the error does not show up with https://github.com/OpenMPT/openmpt/tree/OpenMPT-1.30, so I could also use that since it seems to be similar to the libopenmpt-* tags

manx

manx

2022-06-21 15:09

administrator   ~0005219

For openmpt, my build script pulls the latest tag matching libopenmpt-* from https://github.com/OpenMPT/openmpt.git (just so it doesn't have to worry about svn), so it technically uses a release, but a tag of it though.

That's fine. Using a release (or at least a release branch) is strongly preferred as opposed to trunk/master.

As a side question, does the master branch of that mirror correlate to the trunk? I do see that the git branch contains the revisions you mentioned already and has the same layout as the latest libopenmpt-* tag <https://github.com/OpenMPT/openmpt/tree/libopenmpt-0.6.4>.

yes, git master is the same as svn trunk.

And we also have svn and git branches for stable release branches. We do not make patch releasesfrom trunk but only from release branches, thus ...

Since I use the tag version, I instead used a cherry-picked version of your revisions https://github.com/1480c1/openmpt/commit/2a02d3152 and https://github.com/1480c1/openmpt/commit/a6b824fe2

... you should have cherry picked the patches from branch OpenMPT-1.30 instead: https://github.com/OpenMPT/openmpt/commit/022865b21bad4f673048bdf06318c3dd50f94164 / https://github.com/OpenMPT/openmpt/commit/9e85fad054565a57734fc722c67878ec8cbfdcf2 . Does not matter for the pkg-config part, but the CC part is different between 0.6 and trunk.

As a quick note to append, the error does not show up with https://github.com/OpenMPT/openmpt/tree/OpenMPT-1.30, so I could also use that since it seems to be similar to the libopenmpt-* tags

Yes, use that branch please (at least until 0.6.5 is released, then continue from there with stable releases).

I did go ahead and try the master branch, and I seem to get some errors with it

No need for an extra report, I will look into it. I have a MSYS2 setup myself, so it should be trivial to reproduce and fix.

Also note that our mirroring script takes some time, so svn revision will not immediatly show up in git. Takes roughly 30 minutes usually.

1480c1

1480c1

2022-06-21 15:53

reporter   ~0005220

Yes, use that branch please (at least until 0.6.5 is released, then continue from there with stable releases).

Perfect, I will go ahead and switch to that branch instead

No need for an extra report

Okay, if you need any other info, I can provide it

manx

manx

2022-06-21 16:00

administrator   ~0005221

No need for an extra report
Okay, if you need any other info, I can provide it

Should be fixed in r17551 .

I'll also look into a way to only pass compiler options that clang understands.

manx

manx

2022-06-21 16:29

administrator   ~0005222

I'll also look into a way to only pass compiler options that clang understands.

Turns out I already did implement that half-way. If you additionally pass MINGW_COMPILER=clang, it will now only pass clang-compatible compiler and linker options. Requires r17557 .

r17553 cleans up another clang warning.

1480c1

1480c1

2022-06-21 16:40

reporter   ~0005223

MINGW_COMPILER=clang

Added locally, thanks!

manx

manx

2022-06-22 05:40

administrator   ~0005225

Well, and turns out my fix also does not work because it requires make 3.82, which Apple for some reason still does not ship 12 years later. r17561 adds back support for earlier make versions, which makes the fix a good bit more ugly, but there is nothing I can do about that.

I also added GitHub workflows that test-build with your toolchain configuration, so that we do not accidentally break that again in the future.

Issue History

Date Modified Username Field Change
2022-06-20 22:50 1480c1 New Issue
2022-06-20 22:50 1480c1 File Added: 0002-mingw-w64.mk-don-t-override-the-compilers-etc-if-pro.patch
2022-06-21 08:06 manx Assigned To => manx
2022-06-21 08:06 manx Status new => acknowledged
2022-06-21 09:13 manx Note Added: 0005209
2022-06-21 09:13 manx Status acknowledged => confirmed
2022-06-21 09:17 manx Target Version => OpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first)
2022-06-21 10:27 manx Note Added: 0005210
2022-06-21 11:55 manx Note Added: 0005211
2022-06-21 11:56 manx Severity trivial => minor
2022-06-21 11:56 manx Status confirmed => assigned
2022-06-21 12:23 manx Note Added: 0005213
2022-06-21 12:24 manx Status assigned => feedback
2022-06-21 12:24 manx Note Added: 0005214
2022-06-21 14:55 1480c1 Note Added: 0005217
2022-06-21 14:55 1480c1 Status feedback => assigned
2022-06-21 14:59 1480c1 Note Added: 0005218
2022-06-21 15:09 manx Note Added: 0005219
2022-06-21 15:53 1480c1 Note Added: 0005220
2022-06-21 16:00 manx Note Added: 0005221
2022-06-21 16:29 manx Note Added: 0005222
2022-06-21 16:40 1480c1 Note Added: 0005223
2022-06-22 05:40 manx Note Added: 0005225
2022-06-24 05:23 manx Status assigned => resolved
2022-06-24 05:23 manx Resolution open => fixed
2022-06-24 05:23 manx Fixed in Version => OpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first)