View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0001606 | OpenMPT | Build System | public | 2022-06-20 22:50 | 2022-06-24 05:23 |
Reporter | 1480c1 | Assigned To | manx | ||
Priority | low | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | x64 | OS | Windows | OS Version | 11 |
Product Version | OpenMPT 1.31.00.* (old testing) | ||||
Target Version | OpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first) | Fixed in Version | OpenMPT 1.30.06.00 / libopenmpt 0.6.5 (upgrade first) | ||
Summary | 0001606: 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, | ||||
Additional Information | using msys2 with clang from the mingw64 subsystem rather than clang64 | ||||
Tags | No tags attached. | ||||
Attached Files | 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 | ||||
Has the bug occurred in previous versions? | |||||
Tested code revision (in case you know it) | latest commit | ||||
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 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. |
|
Your suggestion does not work though: |
|
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 |
|
|
|
Can you please verify that what I implemented does indeed work for you? |
|
For openmpt, my build script pulls the latest tag matching As a side question, does the
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:
Using I did go ahead and try the |
|
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 |
|
That's fine. Using a release (or at least a release branch) is strongly preferred as opposed to trunk/master.
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 ...
... 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.
Yes, use that branch please (at least until 0.6.5 is released, then continue from there with stable releases).
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. |
|
Perfect, I will go ahead and switch to that branch instead
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. |
|
Turns out I already did implement that half-way. If you additionally pass r17553 cleans up another clang warning. |
|
Added locally, thanks! |
|
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. |
|
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) |