MacOS: Fixes configuration hang; bump MacOS SDK. #8530
Conversation
Removed conditional use of std::mutex instead of std::shared_mutex on MacOS. Because MacOS < 10.12 did not support std::shared_mutex, a previous commit naïvely substituted std::mutex, which does not have the same behavior. Reverses PR #8273, which substitues std::mutex for std::shared_mutex on macOS, and results in several bugs that seem to only affect MacOS - https://bugs.dolphin-emu.org/issues/11919 - https://bugs.dolphin-emu.org/issues/11842 - https://bugs.dolphin-emu.org/issues/11845 This change eliminates conditional code for MacOS in the core configuration layer code and enables the use of modern language features that are more secure and thread-safe.
|
Is there a reason why there are no macOS checks in this branch? I'd really like to test this. |
|
Probably buildbot issues. Yep, been stuck for ~3 hours apparently. |
|
Thanks for figuring out the issue! |
|
I encountered similar build issues at first. I believe the MacOS Buildbot needs to have its SDK upgraded. |
Looks like this won't even be necessary, since Buildbot is already using the 10.14 SDK. Looks like this argument is to blame:
This results in XCode throwing this error:
|
From what I understand of the CMake documentation, "CMake uses this variable value [CMAKE_OSX_DEPLOYMENT_TARGET] for the -mmacosx-version-min flag or their respective target platform equivalents. " I'm not sure how buildbot is configured, but if there is an environment or commandline parameter hardcoding CMAKE_OSX_DEPLOYMENT_TARGET to 10.10, that could explain the failure. |
|
We might have to clear the cache on the buildbot for the new default to take effect? |
|
While you‘re at it, why not indeed bump the SDK to 10.15 and MoltenVK to the current release for full Metal 3 support? I‘m not sure if this will make any difference performance-wise, but one might at least give it a try. |
|
@pizuz I thought about it, and I realize dolphin is a performance-heavy application that probably doesn’t fare well on older-macs-that-can’t-run-Catalina, but ultimately I decided to bump it only to 10.12 because the compiler pointed out this is the minimum required version for C++17 support. to;dr I am not part of the project’s development planning, so I bumped it conservatively. If anyone wants to go ahead and give me the green-light, I don’t mind making Dolphin a Catalina-only application. I just don’t know the reasons why Yosemite is still supported, and don’t want to step on people’s toes. |
|
We've actually discouraged Dolphin users from updating to Catalina, due to the new notarization requirements. I agree that this PR should not set the required version to anything higher than 10.12. |
@JosJuice @pizuz Ah, I understand now. From what I'm seeing on developer.apple.com, it seems applications built on older SDK's for the time being are grandfathered in until January 2020. If push comes to shove, from what I'm seeing on the internet, a continuous integration system could also notarize the application in an automated way. |
|
I'm not even sure if Dolphin actually needs to be compiled against the 10.15 SDK in order to make use of the new functions in Metal 3 or if the current MoltenVK dylib alone takes care of that, since it's built against the 10.15 SDK. |
|
WRT updating MoltenVK, unless there's a critical issue I'd rather not add megabytes to the repository size when it's not absolutely necessary. There's nothing stopping you from downloading the SDK and replacing it yourself, and this is probably a better way to go about it anyway, rather than including it in the repo (perhaps with an auto downloader). |
https://github.com/rednoah/notarize-app is a project I use to notarize a Qt app from the command line. All it needs is an app-level AppleID password and an AppleID that owns the application identifier of the underlying .pkg/.dmg. FWIW, the notarizing process is a bit chatty over email. |
|
Anyone have any objection to merging this and sorting out the buildbot issues later? Like I said earlier, it might just be matter of clearing the cache/wiping the build directory. |
Removed conditional use of std::mutex instead of std::shared_mutex on MacOS.
Because MacOS < 10.12 did not support std::shared_mutex, a previous commit
naïvely substituted std::mutex, which does not have the same behavior.
Reverses PR #8273, which substitues std::mutex for std::shared_mutex on
macOS, and results in several bugs that seem to only affect MacOS
This change eliminates conditional code for MacOS in the core configuration
layer code and enables the use of modern language features that are more
secure and thread-safe.