Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CMake/MSVC fixes #11506

Merged
merged 4 commits into from Jan 31, 2023
Merged

CMake/MSVC fixes #11506

merged 4 commits into from Jan 31, 2023
+45 −25

Conversation

phire
Copy link
Member

@phire phire commented Jan 29, 2023

It's that time of the year.
I've booted into windows to do something else, and found this semi-broken again.

@phire phire mentioned this pull request Jan 29, 2023
Open
Zopolis4
Zopolis4 approved these changes Jan 29, 2023
View reviewed changes
Copy link
Contributor

@Zopolis4 Zopolis4 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 have anything particularly meaningful to add apart from this, the CMakeSettings change is good (although we should really switch that over to CMakePresets).

Source/PCH/CMakeLists.txt Outdated Show resolved Hide resolved
@phire
Copy link
Member Author

phire commented Jan 29, 2023

(although we should really switch that over to CMakePresets).

Yeah, I got halfway though working out how CMakePresets.json worked before realizing it was the wrong place to set that variable in the first place.

@Zopolis4
Copy link
Contributor

(sorry, didn't realize that my approval would show up like a proper one)

@Zopolis4
Copy link
Contributor

Ideally, we wouldnt have to set two empty variables either (buildCommandArgs and cmakeCommandArgs).

AdmiralCurtiss
AdmiralCurtiss approved these changes Jan 29, 2023
View reviewed changes
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

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

Seems mostly fine. Untested.

Source/Core/Common/WindowsRegistry.h Show resolved Hide resolved
Source/PCH/CMakeLists.txt Outdated Show resolved Hide resolved
@shuffle2
Copy link
Contributor

lgtm

@phire
Cmake/MSVC: Fix Qt6 discover for IDEs other than Visual Studio
1956ce1 
CMakeSettings.json is a Visual Studio only extention to cmake that isn't
supported anywhere else. Not even Visual Studio Code.

So we set CMAKE_PREFIX_PATH inside DolphinQt's CMakeLists.txt instead.
@phire
CMake/MSVC: Fix PCH disabled build
854a73d
@phire
CMake/MSVC: Fix warnings about conflicting /Zi and /Z7
6bb2e4d
@phire
CMake/MSVC: Fix conflicting flags in mbedtls
5d7643a 
This cmake file directly sets CMAKE_<LANG>_FLAGS, which doesn't
show up in the COMPILE_COMMANDS target property and so our
dolphin_disable_warnings_msvc macro failes to remove it.

So we will just commit it out.
@phire phire merged commit c6b851c into dolphin-emu:master Jan 31, 2023
14 checks passed
@phire phire deleted the cmake_fixes branch January 31, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosJuice JosJuice JosJuice left review comments

@AdmiralCurtiss AdmiralCurtiss AdmiralCurtiss approved these changes

@Zopolis4 Zopolis4 Zopolis4 approved these changes

@shuffle2 shuffle2

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@phire @Zopolis4 @shuffle2 @AdmiralCurtiss @JosJuice