Various fixes for msvc/cmake builds #10620

phire · 2022-04-27T07:01:07Z

There appeared to be quite a bit of bitrot

And I'm not sure if PCH support ever worked, I had to completely rewrite it to get it working.

Externals/libusb/CMakeLists.txt

CMakeLists.txt

sepalani · 2022-04-28T19:32:59Z

Source/PCH/CMakeLists.txt

+
+# targets which use the pch need these compile options
+# /Yu - Use precompiled header named "pch.h"
+# /Fi - Force include "pch.h" at top of every source file


I think there is a typo in the comment, should be /FI.

shuffle2 · 2022-05-09T07:45:56Z

@phire might need some tweaks since qt6 was merged

Tilka · 2022-05-09T19:46:24Z

Source/CMakeLists.txt

+  add_compile_options(/external:anglebrackets)
+  add_compile_options(/external:W0)
+
+  # TODO: Enable this once PR #10559 is merged


That PR is now merged.

Tilka · 2022-05-09T19:48:44Z

Source/Core/DolphinQt/CMakeLists.txt

@@ -371,6 +371,11 @@ if (WIN32)
 endif()

 if (MSVC)
+  # Disable some warnings
+  target_compile_options(dolphin-emu PRIVATE
+    /wd5054 # operator '+': deprecated between enumerations of different types (types defined in Qt headers)


Why is this needed when you already disable all warnings in external code?

Fwiw shouldn’t be required anymore as qt fixed their code

shuffle2 · 2022-05-11T04:34:10Z

@phire fwiw #10652 removes the need to fiddle with ignoring warnings generated from fmt

shuffle2 · 2022-05-14T06:27:55Z

I did some profiling and I think some more easy-ish big speed boost opportunities are:

pch for C files (for externals)
pch for DolphinQt (including moc outputs) which includes Qt headers

AdmiralCurtiss · 2022-05-15T00:45:42Z

FYI to get this to build on VS2022 with Qt6 I had to add a CMAKE_PREFIX_PATH pointing at the Qt directory, as else it wouldn't find Qt6 (even if I set Qt6_DIR). So in CMakeSettings.json add:

        {
          "name": "CMAKE_PREFIX_PATH",
          "value": "${workspaceRoot}\\Externals\\Qt\\Qt6.3.0\\x64"
        },

for each config -- ARM64 instead of x64 for the ARM builds, presumably.

Additionally, I had to make an empty mkspecs\win32-msvc in Externals\Qt\Qt6.3.0\x64, as apparently the Qt CMake files set that as an include directory? We probably should add that in the Qt repository.

AdmiralCurtiss · 2022-05-15T01:09:04Z

The --no-angle also needs to be removed in the Qt deploy step, as that option no longer exists in Qt6.

shuffle2 · 2022-05-15T07:14:21Z

FYI to get this to build on VS2022 with Qt6 I had to add a CMAKE_PREFIX_PATH pointing at the Qt directory, as else it wouldn't find Qt6 (even if I set Qt6_DIR). So in CMakeSettings.json add:
        {
          "name": "CMAKE_PREFIX_PATH",
          "value": "${workspaceRoot}\\Externals\\Qt\\Qt6.3.0\\x64"
        },
for each config -- ARM64 instead of x64 for the ARM builds, presumably.

You tried this PR rebased on master? But, setting it in CMAKE_PREFIX_PATH instead of X_DIR is fine (actually what Qt docs recommends), anyway.

Additionally, I had to make an empty mkspecs\win32-msvc in Externals\Qt\Qt6.3.0\x64, as apparently the Qt CMake files set that as an include directory? We probably should add that in the Qt repository.

Here is the full mkspecs dir (for x64 only) (it seems the install target of Qt generates it, so I was wrong that it's only an empty file).
mkspecs.zip

However, it is still qmake-centric, besides mkspecs\win32-msvc\qplatformdefs.h. Is that all we need?

see dolphin-emu/ext-win-qt#19

AdmiralCurtiss · 2022-05-15T11:37:10Z

Yes, this PR rebased on master.

And no clue what files it wants from there, it seems to build fine even with none of them. qplatformdefs.h seems like a reasonable guess though.

AdmiralCurtiss · 2022-05-15T12:22:39Z

2e9375b if you want to cherry-pick the Qt6 fixes.

shuffle2 · 2022-05-21T01:43:34Z

CMakeLists.txt

 project(dolphin-emu)

+if (MSVC)
+  set(CMAKE_C_STANDARD 99)
+  set(CMAKE_CXX_STANDARD 20)


https://stackoverflow.com/a/64890021 seems the way to do this is to set CXX to 23 and remove the explicit "latest" option?

For what it's worth as written it seems to work, I get -std:c++latest in the build command line.

fwiw C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\share\cmake-3.22\Modules\Compiler\MSVC-CXX.cmake has this:

if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 19.29.30129) set(CMAKE_CXX20_STANDARD_COMPILE_OPTION "-std:c++20") set(CMAKE_CXX20_EXTENSION_COMPILE_OPTION "-std:c++20") set(CMAKE_CXX23_STANDARD_COMPILE_OPTION "-std:c++latest") set(CMAKE_CXX23_EXTENSION_COMPILE_OPTION "-std:c++latest") elseif(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 19.12.25835) set(CMAKE_CXX20_STANDARD_COMPILE_OPTION "-std:c++latest") set(CMAKE_CXX20_EXTENSION_COMPILE_OPTION "-std:c++latest") endif()

which is i guess what causes just set(CMAKE_CXX_STANDARD 23) to enable latest on current version

AdmiralCurtiss · 2022-05-21T02:41:31Z

@shuffle2 I think I'm done for now, do you want to review?

shuffle2 · 2022-05-21T03:07:01Z

CMakeLists.txt

+  set(CMAKE_C_STANDARD 99)
+  set(CMAKE_CXX_STANDARD 23)
+  set(CMAKE_CXX_STANDARD_REQUIRED ON)
+  set(CMAKE_CXX_EXTENSIONS OFF)


afaiui, this causes cmake to pass Za for C++ sources(?), something that msbuild setup is currently not doing. It's probably better to leave out for now

As far as I can tell, this option does absolutely nothing on MSVC.

Remove it then?

CMakeLists.txt

shuffle2 · 2022-05-21T03:10:34Z

CMakeLists.txt

@@ -244,16 +258,70 @@ elseif(CMAKE_GENERATOR MATCHES "Visual Studio")
  add_compile_options("/MP")
 endif()

-if(CMAKE_C_COMPILER_ID MATCHES "MSVC")
+if(MSVC)
+  if(POLICY CMP0091)


I think the check should be against cmake version instead (3.22 aligns with our current VS requirements, 3.20 is required for CMP0117 )

I'm not sure what advantage this has. By saying if (POLICY CMP0117) we state exactly what we care about -- the policy existing -- and not some arbitrary CMake version that could mean anything.

I said this because I’d prefer
cmake_policy(VERSION ${CMAKE_VERSION})
but if that’s not done then checking against highest policy we manually enable is fine I guess

As in, always set all policies supported on whatever the current CMake versions is on? That seems dangerous and very much invites the behavior changing invisibly when you upgrade CMake, which is kinda what policies are designed to avoid. I'd be okay with cmake_policy(VERSION 3.22) (which is the version currently used by VS), though, after testing it works with our files anyway.

yes - the thing is that for msvc we know/expect cmake to be the one that gets installed by VS installer. So it's not a random version. I think cmake "getting out of the way" and behaving natively as the one that is installed with msvc is more intuitive and requires less code, but explicitly specifying the version is fine, too (we just have to bump it whenever we bump msvc min version).

anyway, how it is now is fine.

Source/Core/DolphinQt/CMakeLists.txt

CMakeLists.txt

AdmiralCurtiss · 2022-05-21T17:21:45Z

@shuffle2 One more check please? I'd like to get this merged so this actually builds on master.

I have no idea why cmake supports PUBLIC on target_sources, but it does. It causes all targets that depend on this target to try and include the files in their sources. Except it doesn't take paths into account, so it breaks. Mabye it would work if you used an abolute source? But I'm not sure there is a sane usecase.

Since we also treat all warnings as errors, we need to ignore these to successfully build.

I'm not sure if the previous implementation ever worked.

If you don't set this policy, then cmake doesn't even try to select a runtime library

Ninja puts way more effort into compiling targets in parallel, and ignores dependenceis until link time. So we need to jump though hoops to force ninja to compile pch.cpp before any targets which depend on the PCH.

We need these policies, but we kind of want to avoid requiring cmake 3.15 for all platforms

…ke build process wants them.

…s on MSVC.

…n MSVC.

… VS project files.

shuffle2 reviewed Apr 27, 2022

View reviewed changes

Externals/libusb/CMakeLists.txt Outdated Show resolved Hide resolved

shuffle2 reviewed Apr 27, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

phire force-pushed the cmake_win_fixes branch 2 times, most recently from 667c830 to 50ab358 Compare April 28, 2022 17:21

sepalani reviewed Apr 28, 2022

View reviewed changes

Tilka reviewed May 9, 2022

View reviewed changes

AdmiralCurtiss force-pushed the cmake_win_fixes branch 2 times, most recently from 9c16112 to 401a983 Compare May 21, 2022 01:31

shuffle2 reviewed May 21, 2022

View reviewed changes

AdmiralCurtiss force-pushed the cmake_win_fixes branch 2 times, most recently from 0ad1f11 to 020c55d Compare May 21, 2022 13:47

phire added 7 commits May 22, 2022 00:29

cmake: Fix libusb on windows

d2df9af

cmake: add missing FFmpeg dependency on windows

54d89b9

cmake: ignore external warnings in MSVC

2348017

Since we also treat all warnings as errors, we need to ignore these to successfully build.

cmake: fix MSVC PCH support

0909e00

I'm not sure if the previous implementation ever worked.

cmake: fix ninja/msvc builds

89fda54

If you don't set this policy, then cmake doesn't even try to select a runtime library

cmake: fix PCH to work with msvc/ninja

876f665

Ninja puts way more effort into compiling targets in parallel, and ignores dependenceis until link time. So we need to jump though hoops to force ninja to compile pch.cpp before any targets which depend on the PCH.

phire and others added 8 commits May 22, 2022 00:29

cmake: Require cmake 3.15 for MSVC

4db1816

We need these policies, but we kind of want to avoid requiring cmake 3.15 for all platforms

Externals: Update prebuilt Qt submodule to include mkspecs -- the CMa…

96e2f1f

…ke build process wants them.

cmake: Fixes for Qt6 on Windows.

dbee810

cmake: Use C++latest everywhere on MSVC.

89fadd2

cmake: Add a few missing settings from the Visual Studio project file…

9908219

…s on MSVC.

cmake: On MSVC, set c++latest by requesting a C++23 feature set.

7f2ea68

cmake: Use Policy CMP0117 for more sensible RTTI flag configuration o…

a5e8562

…n MSVC.

cmake: Disable warnings for most externals on MSVC, like we do in the…

6ea7af1

… VS project files.

AdmiralCurtiss force-pushed the cmake_win_fixes branch from 020c55d to d40009f Compare May 21, 2022 23:19

cmake: Don't use PCH with Qt6.

580c721

AdmiralCurtiss force-pushed the cmake_win_fixes branch 3 times, most recently from ce02e5d to 580c721 Compare May 22, 2022 00:07

AdmiralCurtiss merged commit 0e948f3 into dolphin-emu:master May 22, 2022
10 checks passed

phire deleted the cmake_win_fixes branch January 28, 2023 14:47

Various fixes for msvc/cmake builds #10620

Various fixes for msvc/cmake builds #10620

phire commented Apr 27, 2022

sepalani Apr 28, 2022

shuffle2 commented May 9, 2022

Tilka May 9, 2022

Tilka May 9, 2022

shuffle2 May 9, 2022

shuffle2 commented May 11, 2022

shuffle2 commented May 14, 2022

AdmiralCurtiss commented May 15, 2022

AdmiralCurtiss commented May 15, 2022

shuffle2 commented May 15, 2022 •

edited

AdmiralCurtiss commented May 15, 2022

AdmiralCurtiss commented May 15, 2022 •

edited

shuffle2 May 21, 2022

AdmiralCurtiss May 21, 2022

shuffle2 May 21, 2022 •

edited

AdmiralCurtiss commented May 21, 2022

shuffle2 May 21, 2022

AdmiralCurtiss May 21, 2022

shuffle2 May 21, 2022

shuffle2 May 21, 2022

AdmiralCurtiss May 21, 2022

shuffle2 May 21, 2022

AdmiralCurtiss May 21, 2022

shuffle2 May 21, 2022

AdmiralCurtiss commented May 21, 2022

Various fixes for msvc/cmake builds #10620

Various fixes for msvc/cmake builds #10620

Conversation

phire commented Apr 27, 2022

Choose a reason for hiding this comment

shuffle2 commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuffle2 commented May 11, 2022

shuffle2 commented May 14, 2022

AdmiralCurtiss commented May 15, 2022

AdmiralCurtiss commented May 15, 2022

shuffle2 commented May 15, 2022 • edited

AdmiralCurtiss commented May 15, 2022

AdmiralCurtiss commented May 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuffle2 May 21, 2022 • edited

Choose a reason for hiding this comment

AdmiralCurtiss commented May 21, 2022

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

AdmiralCurtiss commented May 21, 2022

shuffle2 commented May 15, 2022 •

edited

AdmiralCurtiss commented May 15, 2022 •

edited

shuffle2 May 21, 2022 •

edited