Support CMake on Windows #8087
Merged
Support CMake on Windows #8087
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Outside of lots of warnings when building externals, everything appears to be working as expected (from the standpoint of someone developing a fork) this time around (2019 preview 2). I only have two real gripes:
|
riking
reviewed
May 9, 2019
| "buildRoot": "${env.USERPROFILE}\\CMakeBuilds\\${workspaceHash}\\build\\${name}", | ||
| "inheritEnvironments": [ "msvc_x64_x64" ], | ||
| "buildCommandArgs": "-m -p:PreferredToolArchitecture=x64", | ||
| "buildRoot": "${workspaceRoot}\\build", |
There was a problem hiding this comment.
should this use build-debug instead?
There was a problem hiding this comment.
should this use build-debug instead?
Debug builds on Windows build into the same folder under the name "DolphinD" rather than using a separate build folder so this won't cause any issues. The current implementation keeps that behavior.
https://github.com/spycrab/dolphin/blob/e3805b71f1d9ee879ba2bbbfd5fd2a5f563ee2ce/Source/Core/DolphinQt/CMakeLists.txt#L160-L164
There was a problem hiding this comment.
Yeah, what NarryG wrote.
There was a problem hiding this comment.
This seems to create problems with debug and release builds coexisting. One overwrites the other?
How about instead of copying the binaries to the build directory, we use the install step to copy everything into /Binaries?
There was a problem hiding this comment.
It really shouldn't, as the debug binary is named DolphinD and the release one just Dolphin.
There was a problem hiding this comment.
I'm referring to the the intermediate/object files, not the resulting binary.
There was a problem hiding this comment.
Well it would probably act just as it would when you reconfigure your CMake project under Linux.
BhaaLseN
reviewed
May 9, 2019
There was a problem hiding this comment.
Thanks for getting back to the VS Integration topic! Haven't tried it yet, but the majority looks like the same old space/tabs thing from the other PR.
Perhaps we should change CMakeLists.txt in the .editorconfig so it enforces the space-indent instead of the tabs it specifies right now (which seem to be ignored for the majority of CMake files anyways)?
| @@ -9,6 +9,11 @@ | |||
| #include <string> | |||
| #include <vector> | |||
|
|
|||
| #ifdef _WIN32 | |||
| // TODO: Horrible hack, remove ASAP! | |||
There was a problem hiding this comment.
This was mentioned on the other PR already, but could you add some more detail to this comment why the hack is necessary?
There was a problem hiding this comment.
Wild guess, it's to do with the CreateDirectory macro. I'd propose renaming the function instead.
There was a problem hiding this comment.
Outside of the scope of the PR IMO.
stenzek
reviewed
May 11, 2019
stenzek
reviewed
May 11, 2019
stenzek
reviewed
May 11, 2019
stenzek
reviewed
May 11, 2019
| @@ -9,6 +9,11 @@ | |||
| #include <string> | |||
| #include <vector> | |||
|
|
|||
| #ifdef _WIN32 | |||
| // TODO: Horrible hack, remove ASAP! | |||
There was a problem hiding this comment.
Wild guess, it's to do with the CreateDirectory macro. I'd propose renaming the function instead.
This issue is actually caused by Visual Studio, not by CMake. For some reason, whenever a CMake file changes it gets completely reconfigured and regenerated from scratch. I guess they delete the directory contents as one step of that |
Odd. Microsoft does what Microsoft does. I can easily work around it. Was just something of note to bring up. Outside of that, from my standpoint, everything has been working well. It is annoying that VS has to do those huge regenerations compared to what it had to do with an SLN, but it's manageable on my system. Not sure what someone developing on a lower-powered system would think of it though. |
|
To be sure, so we can just rebase WIP PR's without much hassle after this gets merged, until the existing vcxproj removal? |
|
@altimumdelta Yes that shouldn't pose a problem. |
|
I've tried it with VS 2017 - Using a fresh master branch. Didn't work yet on a straight attempt.
|
|
Sounds like a missing include. Not sure if it's the cause specifically, but I didn't think we were planning on supporting VS2017 with cmake, only 2019. |
|
Oh, I didn't saw any references which one, the update PR made it seem as if it added support for VS2019 as well. I thought I'd skip ahead and just do things in CMake to be ready for the transition, unless the removal of VCXPROJ is many weeks or a couple of months away? |
|
Can't give a date, but we won't be dropping the vcprojs until the issues are ironed out. |
|
Ok, Thanks. Makes sense yeah. |
|
The only error I see is relating to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Less intrusive version of the previous PR that relies on Visual Studio's CMake integration.
Use at least Visual Studio Update 1 Preview or later!