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

[cppwinrt] port rewrite to use cppwint.exe tool from NuGet Gallery #24967

Merged
merged 23 commits into from
Jun 6, 2022

Conversation

walbourn
Copy link
Member

@walbourn walbourn commented May 27, 2022

The existing cppwinrt port really does nothing beyond verifying the installed Windows SDK has pre-generated cppwinrt headers. The Windows SDK headers for C++/WinRT are old, and the recommendation is to use the Microsoft.Windows.CppWinRT NuGet instead. This package contains the latest cppwinrt.exe compiler tool that is then executed using the installed Windows SDK to generate the latest C++/WinRT projections from the Windows SDK .winmd files.

This updated port now does this as well for the installed Windows SDK headers when the port is installed.

This port is not as full-featured as the NuGet package which leverages MSBuild machinery to automatically generate projections for Xaml and authored WinRT components, but this is a big step up from what was there before.

@walbourn
Copy link
Member Author

@BillyONeal @kennykerr Please review this as well.

@walbourn
Copy link
Member Author

walbourn commented May 28, 2022

Validated the supported triplets:

vcpkg install cppwinrt:x86-windows
vcpkg install cppwinrt:x86-windows-static
vcpkg install cppwinrt:x86-windows-static-md
vcpkg install cppwinrt:x64-windows
vcpkg install cppwinrt:x64-windows-static
vcpkg install cppwinrt:x64-windows-static-md
vcpkg install cppwinrt:arm-windows
vcpkg install cppwinrt:arm64-windows
vcpkg install cppwinrt:arm64-windows-static
vcpkg install cppwinrt:arm64-windows-static-md
vcpkg install cppwinrt:x86-uwp
vcpkg install cppwinrt:x64-uwp
vcpkg install cppwinrt:arm-uwp
vcpkg install cppwinrt:arm64-uwp

@LilyWangLL LilyWangLL added the category:port-update The issue is with a library, which is requesting update new revision label May 30, 2022
@LilyWangLL LilyWangLL requested a review from BillyONeal May 30, 2022 06:34
@walbourn
Copy link
Member Author

walbourn commented May 30, 2022

Note I've verified consuming the port from CMake works as expected using:

find_package(cppwinrt CONFIG REQUIRED)
target_link_libraries(<target> PRIVATE Microsoft::CppWinRT)

This results in

(a) the generated C++/WinRT headers are in the INCLUDE path before the Windows SDK
(b) the target links against cppwinrt_fast_forwarder.lib
(c) the target uses C++17 or later

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jun 2, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

PATHS "${CMAKE_CURRENT_SOURCE_DIR}/build/native/lib/${CPPWINRT_ARCH}"
REQUIRED NO_DEFAULT_PATH)

#--- Find Windows SDK Version
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of searching for a windows sdk, the build should:

  1. Require ENV{WindowsSDKDir} and ENV{WindowsSDKVersion to be set (if not, fail)
  2. Require Windows.Foundation.FoundationContract to exist (if not, fail)

Copy link
Member Author

Choose a reason for hiding this comment

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

Simplified logic since in most cases it's already finding it from CMAKE_SYSTEM_VERSION.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this approach and it works locally, but fails in the ADO pipelines. I've restored the scanning SDK versions fallback for cases where it isn't able to rely on the envvars.

ports/cppwinrt/CMakeLists.txt.in Outdated Show resolved Hide resolved
ports/cppwinrt/CMakeLists.txt.in Outdated Show resolved Hide resolved
ports/cppwinrt/CMakeLists.txt.in Outdated Show resolved Hide resolved
ports/cppwinrt/CMakeLists.txt.in Outdated Show resolved Hide resolved
ports/cppwinrt/CppWinRT-config.cmake.in Outdated Show resolved Hide resolved
ports/cppwinrt/CppWinRT-config.cmake.in Outdated Show resolved Hide resolved
ports/cppwinrt/portfile.cmake Outdated Show resolved Hide resolved
ports/cppwinrt/portfile.cmake Outdated Show resolved Hide resolved
ports/cppwinrt/portfile.cmake Outdated Show resolved Hide resolved
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: f0b448081be6fba10e25084686a490cbd800cac6
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: 92c49ba0383df451fc8468c02552b5f2fd30aeac
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: 461c350a7afd51f12570934c778fed578e360b29
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

walbourn and others added 3 commits June 2, 2022 16:04
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: a5a9985a4ad486f761568067b38aec6419d82b2c
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: de946a53fc6995484d31e8d3968e0348a9f1bd88
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: da4c6dd987474ee70f7d28715d791d5fa625264b
new SHA: dd1a129028ac288130d7fef3a4de8edfe9dd3622
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: ff0b0e37642ae2f1798430f20ce763264e477b85
new SHA: 2332b10338616af4bd8ac79a8d5f8787dafe99a8
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@walbourn
Copy link
Member Author

walbourn commented Jun 3, 2022

After all code review, revalidated supported triplets and checked consuming of this port.

vcpkg install cppwinrt:x86-windows
vcpkg install cppwinrt:x86-windows-static
vcpkg install cppwinrt:x86-windows-static-md
vcpkg install cppwinrt:x64-windows
vcpkg install cppwinrt:x64-windows-static
vcpkg install cppwinrt:x64-windows-static-md
vcpkg install cppwinrt:arm-windows
vcpkg install cppwinrt:arm64-windows
vcpkg install cppwinrt:arm64-windows-static
vcpkg install cppwinrt:arm64-windows-static-md
vcpkg install cppwinrt:x86-uwp
vcpkg install cppwinrt:x64-uwp
vcpkg install cppwinrt:arm-uwp
vcpkg install cppwinrt:arm64-uwp

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Jun 4, 2022

As I was looking into why the Windows SDK env vars wouldn't work, I noticed a bunch of other little things that could be touched up:

  1. The CMake build was entirely unnecessary -- it's just running the header generator and doesn't need a compiler at all!
  2. The examples online indicate that the headers should be included via #include <winrt/...> -- this means we want them at include/winrt/... instead of include/cppwinrt/winrt/....
  3. I have switched to enumerating the Windows SDK contents in the CMake instead of the cppwinrt tool -- this avoids touching the registry and so opens the possibility for consuming arbitrary copies of the WinSDK instead of requiring the installer.

I'd appreciate it if you could double check the change I pushed and ensure it still works for you as well, if it does this LGTM. Thanks again!

@walbourn
Copy link
Member Author

walbourn commented Jun 4, 2022

@ras0219-msft

  1. The examples online indicate that the headers should be included via #include <winrt/...> -- this means we want them at include/winrt/... instead of include/cppwinrt/winrt/....

The reason I had it install to include/cppwinrt/winrt and set the include path to include/cppwinrt was so that:

(a) You can still use #include <winrt/ to get the headers
(b) You would not get the updated cppwinrt headers if you had the vcpkg port installed to your vcpkg instance, used some other port that pointed to include, but had NOT added the specific CppWinRT target to your project.

If that isn't a concern, then this is fine.

ports/cppwinrt/vcpkg.json Outdated Show resolved Hide resolved
Co-authored-by: LilyWangLL <94091114+LilyWangLL@users.noreply.github.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cppwinrt have changed but the version was not updated
version: 2.0.220418.1
old SHA: d4e517abedddb8d20362bc9cbae6716ecf3a7f62
new SHA: 18d6860cc0a36639fe348d27ab4cb763dfc0e879
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@ras0219-msft ras0219-msft merged commit 904a51f into microsoft:master Jun 6, 2022
@ras0219-msft
Copy link
Contributor

LGTM, thanks!

@walbourn walbourn deleted the cppwinrtportrewrite branch June 6, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants