[Dev][Build] VS 2026 Support #44304
[Dev][Build] VS 2026 Support #44304
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to update PowerToys to support Visual Studio 2026 with PlatformToolset v145. It centralizes build configuration in Cpp.Build.props, removes explicit PlatformToolset definitions from ~100+ project files, updates C++ language standards, enables strict coroutines (/await:strict), and refactors async patterns from std::future to C++/WinRT types (IAsyncAction/IAsyncOperation).
Critical Issues Identified:
- Visual Studio 2026 and v145 toolset do not exist - This will cause immediate build failures
- IAsyncAction doesn't have
.get()method - Multiple compilation errors from incorrect API usage - Flawed async refactoring - OnThreadExecutor design adds unnecessary complexity and overhead
- Inconsistent test project detection - Mixed case-sensitive/insensitive checks will cause logic errors
Reviewed changes
Copilot reviewed 129 out of 130 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| Cpp.Build.props | Sets v145 toolset (non-existent), configures C++ standards, enables strict coroutines with test project detection logic |
| ~100+ .vcxproj files | Removes explicit PlatformToolset to inherit from Cpp.Build.props |
| src/common/utils/OnThreadExecutor.h | Changes return type from std::future to IAsyncAction with problematic coroutine implementation |
| src/common/utils/HttpClient.h | Updates from std::future to IAsyncOperation/IAsyncAction |
| src/common/updating/updating.cpp | Wraps async operations in std::async and calls .get() synchronously |
| src/modules/fancyzones/FancyZonesLib/* | Updates OnThreadExecutor usage, changes .wait() to .get() (compilation errors) |
| src/modules/Workspaces/WorkspacesLauncher/* | Updates OnThreadExecutor usage with .get() calls (compilation errors) |
| src/modules/ZoomIt/ZoomIt/Zoomit.cpp | Converts async function to synchronous, misleading function name |
| src/common/Display/* | Adds CppWinRT NuGet package, updates OnThreadExecutor usage |
| installer/PowerToysSetupVNext/SilentFilesInUseBA/* | Renames precomp.h to pch.h, removes unused labels, comments out parameter |
| installer/PowerToysSetupVNext/Directory.Build.props | Changes import to use root Directory.Build.props |
| try | ||
| { | ||
| http::HttpClient client; | ||
| client.download(new_version.installer_download_url, *installer_download_path).get(); |
There was a problem hiding this comment.
Calling .get() on IAsyncAction (from HttpClient::download) within a synchronous lambda will block and may cause deadlocks or inefficient execution. The same concern applies here as with the other async operations.
| [&]() { | ||
| combinedWorkArea = FancyZonesUtils::GetAllMonitorsCombinedRect<&MONITORINFOEX::rcWork>(); | ||
| } }).wait(); | ||
| } }).get(); |
There was a problem hiding this comment.
IAsyncAction doesn't have a .get() method like std::future. This will cause compilation errors.
| SetThreadDpiAwarenessContext(DPI_AWARENESS_CONTEXT_UNAWARE); | ||
| SetThreadDpiHostingBehavior(DPI_HOSTING_BEHAVIOR_MIXED); | ||
| } }).wait(); | ||
| } }).get(); |
There was a problem hiding this comment.
IAsyncAction doesn't have a .get() method like std::future. This will cause compilation errors.
| Logger::error(L"Unable to find UI process: {}", get_last_error_or_default(GetLastError())); | ||
| } | ||
| } }).wait(); | ||
| } }).get(); |
There was a problem hiding this comment.
IAsyncAction doesn't have a .get() method like std::future. This will cause compilation errors.
| // | ||
| //---------------------------------------------------------------------------- | ||
| std::future<winrt::com_ptr<ID3D11Texture2D>> CaptureScreenshotAsync(winrt::IDirect3DDevice const& device, winrt::GraphicsCaptureItem const& item, winrt::DirectXPixelFormat const& pixelFormat) | ||
| winrt::com_ptr<ID3D11Texture2D> CaptureScreenshotAsync(winrt::IDirect3DDevice const& device, winrt::GraphicsCaptureItem const& item, winrt::DirectXPixelFormat const& pixelFormat) |
There was a problem hiding this comment.
The function name is CaptureScreenshotAsync but it's no longer async - it returns a regular winrt::com_ptr synchronously instead of a std::future or IAsyncOperation. The function should be renamed to CaptureScreenshot or the implementation should remain async to match the name.
| __in_ecount_z(cFiles) LPCWSTR* rgwzFiles, | ||
| __in int nRecommendation, | ||
| __in BOOTSTRAPPER_FILES_IN_USE_TYPE source, | ||
| __in BOOTSTRAPPER_FILES_IN_USE_TYPE /* source */, |
There was a problem hiding this comment.
The unused parameter source has been commented out, but this is a callback function that must match a specific signature. While the comment-out syntax /* source */ is valid, it would be clearer to use the standard C++ approach of omitting the parameter name entirely, leaving just the type: __in BOOTSTRAPPER_FILES_IN_USE_TYPE,
| __in BOOTSTRAPPER_FILES_IN_USE_TYPE /* source */, | |
| __in BOOTSTRAPPER_FILES_IN_USE_TYPE, |
| if (auto extracted_version = extract_version_from_release_object(release_object)) | ||
| else | ||
| { | ||
| const auto body = client.request(Uri{ LATEST_RELEASE_ENDPOINT }).get(); |
There was a problem hiding this comment.
Same issue as line 117: calling .get() on IAsyncOperation within a synchronous lambda will block and may cause deadlocks or inefficient execution.
| SetThreadDpiHostingBehavior(DPI_HOSTING_BEHAVIOR_MIXED); | ||
| } }) | ||
| .wait(); | ||
| .get(); |
There was a problem hiding this comment.
Changed from .wait() to .get() on IAsyncAction. However, IAsyncAction doesn't have a .get() method - it only has .get() in the blocking sense when you need to obtain a handle. To wait for an IAsyncAction to complete, you should call .get() on the underlying async operation or use co_await. This will likely cause a compilation error or unexpected behavior.
| <ItemGroup> | ||
| <ClCompile Include="SilentFilesInUseBAFunctions.cpp" /> | ||
| <ClCompile Include="bafunctions.cpp"> | ||
| <PrecompiledHeader>Create</PrecompiledHeader> |
There was a problem hiding this comment.
The PrecompiledHeader is set to Create for bafunctions.cpp, but the PrecompiledHeaderFile property has been removed from this element. The file includes "pch.h" (not "precomp.h" anymore), but without specifying PrecompiledHeaderFile, the compiler won't know which header to use for creating the precompiled header. You should add: pch.h
| <PrecompiledHeader>Create</PrecompiledHeader> | |
| <PrecompiledHeader>Create</PrecompiledHeader> | |
| <PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile> |
| <LanguageStandard Condition="$(MSBuildProjectName.ToLower().Contains('test'))">stdcpp20</LanguageStandard> | ||
| <LanguageStandard Condition="!$(MSBuildProjectName.ToLower().Contains('test'))">stdcpplatest</LanguageStandard> | ||
| <!-- Disable strict coroutine mode for test projects --> | ||
| <UseStandardCoroutine Condition="!$(MSBuildProjectName.Contains('Test'))">true</UseStandardCoroutine> | ||
| <AdditionalOptions Condition="!$(MSBuildProjectName.Contains('Test'))">/await:strict %(AdditionalOptions)</AdditionalOptions> | ||
| <!-- TODO: _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING for compatibility with VS 17.8. Check if we can remove. --> | ||
| <PreprocessorDefinitions>_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;_UNICODE;UNICODE;%(PreprocessorDefinitions)</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="!$(MSBuildProjectName.Contains('Test'))">_COROUTINE_ABI=2;_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;_UNICODE;UNICODE;%(PreprocessorDefinitions)</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="$(MSBuildProjectName.Contains('Test'))">_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;_UNICODE;UNICODE;%(PreprocessorDefinitions)</PreprocessorDefinitions> |
There was a problem hiding this comment.
Inconsistent test project detection: Line 57 uses ToLower().Contains('test') (case-insensitive), while lines 60, 61, 63, and 64 use Contains('Test') (case-sensitive). This inconsistency could lead to different behavior - for example, a project named "unittest" would match line 57 but not lines 60-64. Either use ToLower().Contains('test') consistently throughout or use Contains('Test') consistently, but mixing them will cause logic errors.
|
Amazing work |
yeelam-gordon
left a comment
yeelam-gordon
left a comment
There was a problem hiding this comment.
VS 2026 Support Review - Critical Issues Found
I've completed a comprehensive review of this PR upgrading PowerToys from Visual Studio 2022 (v143) to VS 2026 (v145). While the intent is good, there are several critical issues that will prevent compilation and others that need attention.
🔴 Critical (Must Fix Before Merge)
-
Incomplete API refactoring: The HttpClient and OnThreadExecutor now return
IAsyncAction/IAsyncOperationbut the code calls.get()on them, which doesn't exist on WinRT async types. This likely won't compile. -
PlatformToolset fallback broken: Checks for
VisualStudioVersion == '17.0'but VS 2022 uses 17.x (17.1-17.11+). Should use< '18.0'instead. -
Coroutine ABI mismatch: Test projects excluded from
_COROUTINE_ABI=2and/await:strict, creating ABI incompatibility with production code.
⚠️ High Priority Issues
- Performance regression: Refactoring from coroutines to
std::asyncwith blocking.get()calls defeats async I/O benefits and risks thread pool starvation - Missing error logging: Catch blocks swallow exceptions without logging, hiding security-relevant errors
- Code duplication: OnThreadExecutor exists in both
common/utilsandfancyzones/FancyZonesLib
📋 Documentation Updates Needed
Multiple docs reference "VS 2022" and need updating:
- BUILD-GUIDELINES.md
- doc/devdocs/modules/fancyzones.md
- doc/devdocs/development/*.md
- README.md prerequisites
- CI/CD workflows
Recommendations
- Fix the .get() calls: Either revert to coroutines or use proper WinRT blocking mechanisms
- Correct the version check: Use
'$(VisualStudioVersion)' < '18.0' - Add explanatory comments: Document why the coroutine refactoring was necessary
- Consolidate duplicates: Remove OnThreadExecutor duplication
- Verify CI/CD: Ensure GitHub Actions runners have VS 2026 or fallback works
Full detailed review saved in review files. Happy to discuss any of these findings!
| <!-- Use C++20 for test projects for modern features, latest for production --> | ||
| <LanguageStandard Condition="$(MSBuildProjectName.ToLower().Contains('test'))">stdcpp20</LanguageStandard> | ||
| <LanguageStandard Condition="!$(MSBuildProjectName.ToLower().Contains('test'))">stdcpplatest</LanguageStandard> | ||
| <!-- Disable strict coroutine mode for test projects --> | ||
| <UseStandardCoroutine Condition="!$(MSBuildProjectName.Contains('Test'))">true</UseStandardCoroutine> | ||
| <AdditionalOptions Condition="!$(MSBuildProjectName.Contains('Test'))">/await:strict %(AdditionalOptions)</AdditionalOptions> | ||
| <!-- TODO: _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING for compatibility with VS 17.8. Check if we can remove. --> | ||
| <PreprocessorDefinitions>_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;_UNICODE;UNICODE;%(PreprocessorDefinitions)</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="!$(MSBuildProjectName.Contains('Test'))">_COROUTINE_ABI=2;_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;_UNICODE;UNICODE;%(PreprocessorDefinitions)</PreprocessorDefinitions> |
There was a problem hiding this comment.
Test projects are set to stdcpp20 while production code uses stdcpplatest. The comment says 'Use C++20 for test projects for modern features, latest for production'. However, mixing C++ standards between test and production code can cause ABI incompatibilities, especially with std::expected, coroutines, and other newer features. → Either: (1) Document why the split is necessary (e.g., specific test framework compatibility issues), or (2) Use the same standard for both to ensure ABI compatibility, or (3) Ensure test projects don't link against production code with incompatible ABIs.
| <LanguageStandard Condition="!$(MSBuildProjectName.ToLower().Contains('test'))">stdcpplatest</LanguageStandard> | ||
| <!-- Disable strict coroutine mode for test projects --> | ||
| <UseStandardCoroutine Condition="!$(MSBuildProjectName.Contains('Test'))">true</UseStandardCoroutine> | ||
| <AdditionalOptions Condition="!$(MSBuildProjectName.Contains('Test'))">/await:strict %(AdditionalOptions)</AdditionalOptions> | ||
| <!-- TODO: _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING for compatibility with VS 17.8. Check if we can remove. --> |
There was a problem hiding this comment.
The UseStandardCoroutine and /await:strict are only enabled for non-test projects, and test projects don't get _COROUTINE_ABI=2. This creates inconsistency: if production code uses strict coroutine ABI and test code doesn't, any test that includes headers with coroutine types (like IAsyncAction) will have ABI mismatches. → Either exclude tests from using coroutines entirely, or ensure test projects use the same coroutine settings as production. The conditional must match on both sides.
| return std::async(std::launch::async, [prerelease]() -> | ||
| #if USE_STD_EXPECTED | ||
| co_return std::unexpected(LOCAL_BUILD_ERROR); | ||
| std::expected<github_version_info, std::wstring> | ||
| #else | ||
| co_return nonstd::make_unexpected(LOCAL_BUILD_ERROR); | ||
| nonstd::expected<github_version_info, std::wstring> |
There was a problem hiding this comment.
The refactored code wraps the entire function body in std::async, but the original used coroutines (co_return). This changes the execution model significantly. With the original coroutine approach, the function would suspend at co_await points and return immediately. The new std::async approach spawns a thread that blocks on .get() calls (e.g., client.request(...).get()), which defeats the purpose of async I/O and may cause thread pool starvation under load. → Use proper coroutine support or maintain the original co_return pattern with C++20 coroutines if VS 2026 supports it. If coroutines are problematic, consider using WinRT's thread pool or callbacks instead of blocking in std::async.
| const auto body = co_await client.request(Uri{ LATEST_RELEASE_ENDPOINT }); | ||
| release_object = json::JsonValue::Parse(body).GetObjectW(); | ||
| if (auto extracted_version = extract_version_from_release_object(release_object)) | ||
| else |
There was a problem hiding this comment.
The HttpClient.request() now returns IAsyncOperation<hstring> instead of std::future<std::wstring>, but the code calls .get() on it directly. IAsyncOperation doesn't have a .get() method - it needs to be awaited with co_await or you need to call .get() on the underlying winrt::impl interface. → This code likely doesn't compile. Either: (1) Keep coroutines and use co_await, or (2) Convert to synchronous blocking calls using winrt::impl::get_abi or similar blocking mechanism, or (3) Use proper async/await throughout the call chain.
| for (size_t i = 0; i < MAX_DOWNLOAD_ATTEMPTS; ++i) | ||
| { | ||
| try | ||
| return std::async(std::launch::async, [new_version]() -> std::optional<std::filesystem::path> { |
There was a problem hiding this comment.
Similar issue as line 133: client.download(...).get() is called on IAsyncAction, which doesn't have a .get() method. The refactoring is incomplete. → Needs the same fix as the request() calls - either use co_await throughout or implement proper blocking mechanisms for IAsyncAction.
| return std::async(std::launch::async, [prerelease]() -> | ||
| #if USE_STD_EXPECTED | ||
| co_return std::unexpected(LOCAL_BUILD_ERROR); | ||
| std::expected<github_version_info, std::wstring> | ||
| #else | ||
| co_return nonstd::make_unexpected(LOCAL_BUILD_ERROR); | ||
| nonstd::expected<github_version_info, std::wstring> |
There was a problem hiding this comment.
The refactoring from coroutines to std::async is a significant architectural change with no explanatory comment. Why was this necessary? Is it a VS 2026 coroutine bug workaround? A temporary measure? This needs documentation. → Add a comment like: '// Refactored from coroutines to std::async due to [reason]. See issue #[number] for details.' or '// VS 2026 Preview: Temporarily using std::async until C++20 coroutine support stabilizes.'
| winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> request(const winrt::Windows::Foundation::Uri& url) | ||
| { | ||
| auto response = co_await m_client.GetAsync(url); | ||
| (void)response.EnsureSuccessStatusCode(); | ||
| auto body = co_await response.Content().ReadAsStringAsync(); | ||
| co_return std::wstring(body); | ||
| co_return body; | ||
| } |
There was a problem hiding this comment.
HttpClient methods changed return types from std::future<T> to IAsyncOperation<T>/IAsyncAction. This is a breaking API change for all callers. While the change aligns with WinRT patterns, all callsites must be updated. The updating.cpp file calls .get() on these, which suggests incomplete migration. → Ensure all consumers of HttpClient are updated: (1) Search for all HttpClient usage across the codebase, (2) Verify each callsite is updated to handle IAsync* types correctly, (3) Update any unit tests that mock or test HttpClient.
| using task_t = std::packaged_task<void()>; | ||
|
|
||
| OnThreadExecutor(); | ||
| ~OnThreadExecutor(); |
There was a problem hiding this comment.
Code duplication: OnThreadExecutor exists in both common/utils and fancyzones/FancyZonesLib with nearly identical implementations. This violates DRY (Don't Repeat Yourself) and creates maintenance burden. Changes must be applied to both locations. → Consolidate to a single implementation in common/utils and remove the FancyZones duplicate, or clearly document why duplication is necessary (e.g., different threading models).
| return version_up_to_date{}; | ||
| } | ||
| } | ||
|
|
||
| if (github_version <= current_version) | ||
| auto [installer_download_url, installer_filename] = extract_installer_asset_download_info(release_object); | ||
| return new_version_download_info{ extract_release_page_url(release_object), | ||
| std::move(github_version), | ||
| std::move(installer_download_url), | ||
| std::move(installer_filename) }; | ||
| } | ||
| catch (...) | ||
| { |
There was a problem hiding this comment.
PowerToys uses centralized logging via Logger::info/warn/error from common/logger. The catch-all block silently swallows exceptions without logging. This violates the repo pattern of comprehensive logging for diagnostics. → Add Logger::error("Update check failed", ...) in the catch block to maintain consistency with other error handling in PowerToys.
| <!-- Use C++20 for test projects for modern features, latest for production --> | ||
| <LanguageStandard Condition="$(MSBuildProjectName.ToLower().Contains('test'))">stdcpp20</LanguageStandard> |
There was a problem hiding this comment.
Comment says 'Use C++20 for test projects for modern features, latest for production' but doesn't explain why this split is necessary. This is a significant decision that future maintainers need to understand. → Expand comment to explain the reasoning: 'Use C++20 for test projects to ensure compatibility with [test framework name] while production uses stdcpplatest to leverage [specific features]' or document the ABI compatibility concerns.
Summary of the Pull Request
This PR updates the PowerToys solution to support Visual Studio 2026 (PlatformToolset v145). It centralizes the build configuration, updates the C++ language standards, and refactors async patterns across several modules to align with C++/WinRT best practices and the new toolset requirements.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Build System & Configuration:
Cpp.Build.propsto usev145(VS 2026) as the defaultPlatformToolset, with fall back tov143for VS 2022.stdcpplatestfor production projects.stdcpp20for test projects./await:strict) and defined_COROUTINE_ABI=2.<PlatformToolset>definitions from individual project files (approx. 37 modules) to inherit correctly from the centralCpp.Build.props.Code Refactoring & Fixes:
HttpClientandOnThreadExecutorto use standard C++/WinRT types.Microsoft.Windows.CppWinRTNuGet package and removed legacy toolset definitions.OnThreadExecutorusage to be compatible with the new standard settings.Validation Steps Performed