Skip to content

Align CMake to Visual Studio projects#56875

Merged
dseguin merged 61 commits intoCleverRaven:masterfrom
alef:msvc-bits
May 20, 2022
Merged

Align CMake to Visual Studio projects#56875
dseguin merged 61 commits intoCleverRaven:masterfrom
alef:msvc-bits

Conversation

@alef
Copy link
Contributor

@alef alef commented Apr 17, 2022

Summary

None

Purpose of change

Use CMake and VCPKG to build a Windows release and testing it.

Describe the solution

  • Comparing compiler and linker flags from the two build systems, and updated CMake.
  • CMake: /W3 and /GR removed starting CMake 3.15. Enforced with policies for clarity
  • VS: /std:c++14 default since VS 2015 Update 3 (read CMake's MSVC-CXX.cmake)
  • VS: /external:W1 is set but not used since no includes are marked with /external:I
  • CMake: /Ob1 part of MinSizeRel and RelWithDebInfo presets
  • CMake: /TP meaning every file is a C++ file.
  • CMake: No idea why src/third-party is in /external:I
  • VS: /Gm- the whole option is deprecated
  • VS: /fp:precise is default
  • VS: /MT. Enabled CMP0091 for it
  • Adding VCPKG to CMake, using presets CMakePresets.json and toolchain files like build-scripts/MSVC.cmake.
  • Split flags across presets and chained toolchain files based on default CMAKE_BUILD_TYPE(s)
  • prebuild.cmd may not have git.exe in PATH. It now attempt using VS own.
  • No need to create LC_MESSAGE/ used in testing because now in git database
  • Use the same precompiled headers as in other platforms, which also fixes missing required #include <tuple> and <cwctype>
  • Build VCPKG's whole gettext just to use msgfmt. Since it take too long, maybe will be removed in favor of using the one in Git for Windows

Describe alternatives you've considered

Started a local subperbuild branch with the SDL lineage but then realized VCPKG is just doing that.

Testing

  • Local Windows computer build only and your GHA
  1. cmake --preset x64-debug
  2. cmake --build --preset x64-debug
  • Maybe Linux WSL/Hyper-V images later.

Additional context

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Apr 17, 2022
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Apr 17, 2022
@alef alef marked this pull request as ready for review May 1, 2022 20:27
@kevingranade
Copy link
Member

I need someone that uses CMake to review this.

@akrieger
Copy link
Member

akrieger commented May 4, 2022

CMake: No idea why src/third-party is in /external:I

Because the third party lib is setup to export the include path as a 'system' include path, which makes to -isystem for clang/gcc and /I:external for VS. It's setup that way because otherwise the non-msvc compilers and clang-tidy checks spew an astronomical amount of noise from those headers otherwise.

VS: /external:W1 is set but not used since no includes are marked with /external:I

Probably cause it's the default to have /external:W1 but we don't actually have a higher warning level set by default anyway for VS builds because it's too spammy to do so and no one has the motivation to clean up the noise, because we get better signal from the clang/gcc builds anyway.

Copy link
Member

@akrieger akrieger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really do CMake but I can comment a bit generally about the windows build specifics.

@akrieger
Copy link
Member

akrieger commented May 4, 2022

The PCH change appears to have broken clang-tidy.

@alef
Copy link
Contributor Author

alef commented May 4, 2022

The PCH change appears to have broken clang-tidy.

clang-tidy run searched for cmake_pch.hxx.pch. But it does not exist in my build. The pattern I have is:

cmake_pch.cxx
cmake_pch.cxx.obj
cmake_pch.cxx.pch
cmake_pch.hxx

@akrieger
Copy link
Member

akrieger commented May 5, 2022

Even still, we can't break clang-tidy. I think though what we can do is just conditionally enable PCH based on the same environment variable we use in the Makefile, just PCH=1 or 0.

@akrieger
Copy link
Member

akrieger commented May 5, 2022

Or, alternatively, disable PCH when clang-tidy is enabled.

Copy link
Member

@akrieger akrieger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant to write some of these earlier but missed the submit button.

Copy link
Member

@akrieger akrieger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My static analysis of this is satisfied. I haven't tested it locally but I trust @alef has, I've just reviewed it with my eyeballs. The clang-tidy check exercises the CMake setup hard enough that if it isn't obviously broken, it's probably fine. I would appreciate if someone who actually has tried to use the cmake build to build can validate it's not broken on other platforms, but the clang-tidy check is the only 'production' flow that depends on it.

@alef alef requested a review from BrettDong as a code owner May 15, 2022 14:37
@github-actions github-actions bot added the Translation I18n label May 15, 2022
@dseguin
Copy link
Member

dseguin commented May 20, 2022

My static analysis of this is satisfied.

Good enough for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Translation I18n

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants