dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

Remove Visual Studio

Open Zopolis4 opened this issue 3 years ago • 16 comments

See #8068 for rationales for removing Visual Studio.

That's not the main point of this PR though, the main point of this PR is to enable vcpkg integration.

Having a package manager on all systems will allow us to slowly thin out Externals and stop shipping all our dependencies in-tree. Not all dependencies will be suited to this, but something is much better than nothing.

In regards to the buildbots, I think dusting off dolphin-emu/sadm#133 should work fine.

Note: to people who have concerns that switching the buildsystem will make things difficult for old PR's, any old PR's will already need to rebase to build due to the android issues, the freebsd issues, etc.

Zopolis4 avatar Aug 22 '22 08:08 Zopolis4

Two things:

  • While I agree that removing the VS Project files is a good idea in general, first we need to ensure that the CMake project actually does the same thing as the VS Project files for Windows+MSVC. I'm pretty sure there's still some inconsistency with compile flags and the like there -- and I know that incremental updating of the current build revision is broken, because my local build never updates that correctly unless I do a full wipe.
  • vcpkg is a wholly different beast that you're not going to get through in the same breath as removing the VS Project files. Either do that first or leave that for later.

AdmiralCurtiss avatar Aug 22 '22 13:08 AdmiralCurtiss

Having Externals is a big advantage in terms of controlling dependencies and how the dependencies code is compiled. We can easily adjust compile flags uniformly across the entire project, something which is possible but not currently implemented in our cmake environment.

I'm strongly against using vcpkg in dolphin.

shuffle2 avatar Aug 22 '22 19:08 shuffle2

  • vcpkg is a wholly different beast that you're not going to get through in the same breath as removing the VS Project files. Either do that first or leave that for later.

I know, I'm leaving it for a seperate PR. Just putting it as part of my rationale.

Zopolis4 avatar Aug 22 '22 21:08 Zopolis4

I had a look, and all of the flags appear to be the same.

Zopolis4 avatar Aug 23 '22 11:08 Zopolis4

Incremental updating of the build version appears to be broken because CMake does not call make_scmrev.h.js. I can't find any official examples on how to call a javascript file in CMake, so I was planning on converting it to Python. Is there any benefit to it being a JS file?

Zopolis4 avatar Sep 06 '22 02:09 Zopolis4

I assume it's js because that's what VS build scripts support?

I also see no reason why we need to invoke python for this, we have this set up at work with pure CMake.

AdmiralCurtiss avatar Sep 06 '22 02:09 AdmiralCurtiss

For reference, Dolphin's CMake files already do this, it's just not properly re-executed when the commit changes:

https://github.com/dolphin-emu/dolphin/blob/1088021e39750c22ba622ab9e5065ffc32cb0e39/CMakeLists.txt#L165-L216 https://github.com/dolphin-emu/dolphin/blob/1088021e39750c22ba622ab9e5065ffc32cb0e39/CMakeLists.txt#L974-L986

AdmiralCurtiss avatar Sep 06 '22 02:09 AdmiralCurtiss

Fixed.

Zopolis4 avatar Sep 06 '22 04:09 Zopolis4

Please make the CMakeLists.txt change a separate PR.

AdmiralCurtiss avatar Sep 06 '22 20:09 AdmiralCurtiss

See #11035.

Zopolis4 avatar Sep 07 '22 00:09 Zopolis4

Seeing that #11061 is now merged, is this just waiting on buildbots? (i.e. do people have any other changes, comments or complaints)

Zopolis4 avatar Sep 19 '22 00:09 Zopolis4

I'm not convinced the CMake build files use the correct flags yet. Taking a random file:

CMake in Debug mode:

build Source\Core\AudioCommon\CMakeFiles\audiocommon.dir\AudioCommon.cpp.obj: CXX_COMPILER__audiocommon_Debug dolphin\Source\Core\AudioCommon\AudioCommon.cpp || cmake_object_order_depends_target_audiocommon
  DEFINES = -DAUTOUPDATE=1 -DDATA_DIR=\"dolphin/out/install/Debug/share/dolphin-emu/\" -DHAS_OPENGL -DHAS_VULKAN -DHAVE_FFMPEG -DHAVE_SDL2=1 -DNOMINMAX -DSFML_STATIC -DUNICODE -DUSE_ANALYTICS=1 -DUSE_UPNP -DWIN32_LEAN_AND_MEAN -D_ARCH_64=1 -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_DEBUG -D_M_X86=1 -D_M_X86_64=1 -D_SCL_SECURE_NO_WARNINGS -D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING -D_UNICODE -D__LIBUSB__ -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS
  FLAGS = /DWIN32 /D_WINDOWS /EHsc /Zi /Ob0 /Od /RTC1 -MDd /GR- /W4 /WX /wd4201 /wd4127 /wd4100 /wd4200 /wd4244 /wd4121 /wd4324 /wd4714 /wd4351 /wd4245 /w44263 /w44265 /w44946 /utf-8 /external:anglebrackets /external:W0 /external:templates- /experimental:deterministic /Gy /Oi /GS /permissive- /Zc:inline /Zc:throwingNew /volatile:iso /experimental:newLambdaProcessor /Zc:__cplusplus,externConstexpr,lambda,preprocessor /wd5105 /fp:precise /Yupch.h /FIpch.h /Fpdolphin/Build/Debug/Source/PCH/dolphin.pch /Z7 /Gm- -std:c++latest
  INCLUDES = -Idolphin\Externals\curl\include -Idolphin\Externals\SFML\include -Idolphin\Source\Core -Idolphin\Externals\enet\include -Idolphin\External\minizip -Idolphin\Externals\ed25519 -Idolphin\Externals\soundtouch -Idolphin\Externals\mbedtls\include -Idolphin\Externals\libiconv-1.14\include -Idolphin\Externals\discord-rpc\include -Idolphin\Externals\WIL\include -Idolphin\Externals\OpenAL\include -Idolphin\Externals\picojson -Idolphin\Build\Debug\Source\Core -Idolphin\Externals\fmt\include -Idolphin\Externals\minizip\. -Idolphin\Build\Debug\Externals\zlib-ng\zlib-ng -Idolphin\Externals\zlib-ng\zlib-ng -Idolphin\Externals\cubeb\include -Idolphin\Build\Debug\exports -Idolphin\Externals\FreeSurround\include
  OBJECT_DIR = Source\Core\AudioCommon\CMakeFiles\audiocommon.dir
  OBJECT_FILE_DIR = Source\Core\AudioCommon\CMakeFiles\audiocommon.dir
  TARGET_COMPILE_PDB = Source\Core\AudioCommon\CMakeFiles\audiocommon.dir\audiocommon.pdb
  TARGET_PDB = Source\Core\AudioCommon\audiocommon.pdb

MSVC in Debug mode:

^DOLPHIN\SOURCE\CORE\AUDIOCOMMON\AUDIOCOMMON.CPP
/c /IDOLPHIN\EXTERNALS\ZSTD\LIB /I"DOLPHIN\EXTERNALS\ZLIB-NG" /IDOLPHIN\EXTERNALS\XXHASH /I"DOLPHIN\EXTERNALS\SPIRV_CROSS\SPIRV-CROSS" /IDOLPHIN\EXTERNALS\SOUNDTOUCH /IDOLPHIN\EXTERNALS\SFML\INCLUDE /IDOLPHIN\EXTERNALS\SDL\SDL\INCLUDE /IDOLPHIN\EXTERNALS\PUGIXML /IDOLPHIN\EXTERNALS\PICOJSON /IDOLPHIN\EXTERNALS\MINIZIP /IDOLPHIN\EXTERNALS\MINIUPNPC\SRC /IDOLPHIN\EXTERNALS\MGBA\MGBA\INCLUDE /IDOLPHIN\EXTERNALS\MBEDTLS\INCLUDE /IDOLPHIN\EXTERNALS\LZO /IDOLPHIN\EXTERNALS\LIBUSB\LIBUSB\LIBUSB /IDOLPHIN\EXTERNALS\LIBSPNG\LIBSPNG\SPNG /IDOLPHIN\EXTERNALS\LIBLZMA\API /IDOLPHIN\EXTERNALS\IMGUI /IDOLPHIN\EXTERNALS\GLSLANG\SPIRV /IDOLPHIN\EXTERNALS\GLSLANG\GLSLANG\PUBLIC /IDOLPHIN\EXTERNALS\GLSLANG\STANDALONE /IDOLPHIN\EXTERNALS\GLSLANG /IDOLPHIN\EXTERNALS\FREESURROUND\INCLUDE /IDOLPHIN\EXTERNALS\FMT\INCLUDE /IDOLPHIN\EXTERNALS\FATFS /IDOLPHIN\EXTERNALS\ENET\INCLUDE /IDOLPHIN\EXTERNALS\ED25519 /I"DOLPHIN\EXTERNALS\DISCORD-RPC\INCLUDE" /IDOLPHIN\EXTERNALS\CURL\INCLUDE /IDOLPHIN\EXTERNALS\CUBEB\INCLUDE /IDOLPHIN\EXTERNALS\CUBEB\MSVC /I"DOLPHIN\EXTERNALS\CPP-OPTPARSE" /IDOLPHIN\EXTERNALS\BZIP2 /IDOLPHIN\EXTERNALS\BOCHS_DISASM /IDOLPHIN\EXTERNALS\WIL\INCLUDE /IDOLPHIN\EXTERNALS\VULKAN\INCLUDE /IDOLPHIN\EXTERNALS\RANGESET\INCLUDE /IDOLPHIN\EXTERNALS\OPENAL\INCLUDE /I"DOLPHIN\EXTERNALS\FFMPEG-BIN\X64\INCLUDE" /IDOLPHIN\SOURCE\CORE\ /Z7 /JMC /nologo /W4 /WX /diagnostics:caret /Od /Oi /D SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS /D SFML_STATIC /D SPNG_STATIC /D LZMA_API_STATIC /D CURL_STATICLIB /D HAVE_SDL2 /D AUTOUPDATE /D HAS_LIBMGBA /D HAS_VULKAN /D HAS_OPENGL /D HAVE_FFMPEG /D USE_DISCORD_PRESENCE /D USE_ANALYTICS=1 /D USE_UPNP /D __LIBUSB__ /D _ARCH_64=1 /D _M_X86=1 /D _M_X86_64=1 /D _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING /D _WINSOCK_DEPRECATED_NO_WARNINGS /D _CRT_SECURE_NO_WARNINGS /D _CRT_NONSTDC_NO_WARNINGS /D WIN32_LEAN_AND_MEAN /D NOMINMAX /D WIL_SUPPRESS_EXCEPTIONS /D _DEBUG /D _UNICODE /D UNICODE /Gm- /EHsc /RTC1 /MDd /GS /Gy /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR- /std:c++latest /permissive- /Yu"pch.h" /Fp"DOLPHIN\BUILD\X64\DEBUG\PCH\PCH.PCH" /Fo"DOLPHIN\BUILD\X64\DEBUG\DOLPHINLIB\\" /Fd"DOLPHIN\BUILD\X64\DEBUG\DOLPHINLIB\BIN\DOLPHINLIB.PDB" /external:anglebrackets /external:W0 /external:templates- /Gd /TP /wd4245 /wd4201 /wd4127 /wd4100 /wd4244 /wd4121 /wd4324 /wd4714 /FIPCH.H /FC /w44263 /w44265 /w44946 /pathmap:"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\\"=v:\ /pathmap:"C:\Program Files (x86)\Windows Kits\10\\"=w:\ /pathmap:"dolphin\\"=d:\ /experimental:deterministic /utf-8 /Zo /Zc:__cplusplus,externConstexpr,lambda,preprocessor,throwingNew /volatile:iso  DOLPHIN\SOURCE\CORE\AUDIOCOMMON\AUDIOCOMMON.CPP

Actually untangling that mess, if I didn't make any errors:

  • MSVC defines but CMake doesn't:

    • SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS
    • SPNG_STATIC
    • LZMA_API_STATIC
    • CURL_STATICLIB
    • HAS_LIBMGBA
    • USE_DISCORD_PRESENCE
    • _WINSOCK_DEPRECATED_NO_WARNINGS
    • WIL_SUPPRESS_EXCEPTIONS
  • CMake defines but MSVC doesn't:

    • _CRT_SECURE_NO_DEPRECATE
    • _SCL_SECURE_NO_WARNINGS
    • __STDC_CONSTANT_MACROS
    • __STDC_LIMIT_MACROS
    • WIN32
    • _WINDOWS
  • Other minor define differences that are probably harmless:

    • MSVC has HAVE_SDL2, CMake has HAVE_SDL2=1.
    • MSVC has AUTOUPDATE, CMake has AUTOUPDATE=1.
  • Compile flags differences:

    • MSVC sets /c (compile but not link), which is probably implicit in CMake? Harmless, either way.
    • CMake's debugging info flags are weird (it has both /Zi and /Z7 which are incompatible, and does not have MSVC's /JMC and /Zo), someone probably should look at that.
    • MSVC sets /nologo, harmless.
    • MSVC sets /diagnostics:caret, harmless.
    • MSVC sets /Fo and /Fd, harmless.
    • MSVC sets /Zc:wchar_t and /Zc:forScope, these are probably meaningful.
    • CMake sets /Ob0, disabling inline expansion. I'm guessing this is because I compiled in Debug but MSVC does not set this so maybe should look into this.
    • CMake sets /experimental:newLambdaProcessor. I think this is outdated?
    • MSVC sets /Gd, does this even matter on x64?
    • MSVC sets /TP, harmless.
    • MSVC sets /FC, harmless.
    • MSVC sets a few /pathmaps. These are meaningful, IIRC we use them to trim the full filepaths out of log output?
    • CMake sets /wd4200, /wd4351 and /wd5105 but MSVC does not, that's a bit odd. Does warning 4351 even exist??

Most of the defines happen because our CMake files actually have more localized defines while the MSVC ones just have global ones (eg. the missing SPNG_STATIC and HAS_LIBMGBA, which aren't linked to Common), I think. That's fine. But eg. the WIL_SUPPRESS_EXCEPTIONS I can't find in CMake at all, so that seems wrong.

The compile flags look mostly fine but there's a few that should probably be looked at in more detail, definitely not quite identical.

To be clear, I'm not saying that the current VS flags are the best possible options, but I do think that changes to them should be deliberate, not accidental.

AdmiralCurtiss avatar Oct 07 '22 22:10 AdmiralCurtiss

When you say harmles, do you mean I don't need to implement that define or I should implement it? I'm not too familiar with the various meanings and implications of all of the compile flags.

Zopolis4 avatar Oct 08 '22 00:10 Zopolis4

Harmless means you can ignore it.

AdmiralCurtiss avatar Oct 08 '22 00:10 AdmiralCurtiss

I've been working on this-- what should I do for CMake defines that MSVC doesn't have? I'm not going for an overhaul of all our defines here, just trying to remove MSVC, so should I just make CMake have the same important defines and flags as MSVC?

Zopolis4 avatar Dec 03 '22 05:12 Zopolis4

CMake's debugging info flags are weird (it has both /Zi and /Z7 which are incompatible

I've fixed that in #11506 (though it's a bit of a mess) among a few other issues.

phire avatar Jan 29 '23 09:01 phire