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

Add support for settable VS_INSTALLATION_PATH - enabling eWDK #74

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

kaloth
Copy link
Contributor

@kaloth kaloth commented Aug 15, 2023

With these changes it's now possible to use the EWDK to build with this toolchain instead of a pre-installed VS version.

To use the EWDK follow the following steps..

This was nessecary for my use case where I'm building a Windows driver project on an automated remote VM with no pre-existing visual studio installation.

Hopefully it's useful to someone else here!

You can now use the EWDK to build with this toolchain instead of a pre-installed VS version.
@kaloth
Copy link
Contributor Author

kaloth commented Aug 16, 2023

Updated to fix an issue with the github actions windows environment used for testing.

@MarkSchofield
Copy link
Owner

Thanks for the contribution, @kaloth, but I'm a bit conflicted. To use the EWDK you have to run - say - 'C:\EWDK\BuildEnv\SetupBuildEnv.cmd', which sets/updates a bunch of environment variables to implicitly configure the compiler. That's what this Toolchain is trying to avoid - I don't like having to launch a Visual Studio Developer Prompt to build, since the compiler toolchain is described in the environment and not in the CMake domain.

@kaloth
Copy link
Contributor Author

kaloth commented Aug 30, 2023

Hey, @MarkSchofield. Thanks for taking a look at the change. I think I understand your point but I remain hopefull that this could go in as it's so trivial.

Sadly there's no cmake-friendly way to search for the EWDK automatically as the user is free to install it anywhere they like and there's no registry entries or anything to check. A user could even have multiple EWDKs in different folders scattered around their system.

If you dislike the environment variable check I can remove that and have the user specify CMAKE_WINDOWS_KITS_10_DIR in the cmake command? That would keep it more in-line with the existing code.

Windows.Kits.cmake Show resolved Hide resolved
Windows.Kits.cmake Outdated Show resolved Hide resolved
Windows.MSVC.toolchain.cmake Show resolved Hide resolved
@MarkSchofield
Copy link
Owner

MarkSchofield commented Sep 9, 2023

I left a couple of comments.

One thing that makes me nervous is that when launching an eWDK prompt, it sets the host and target - if I launch a prompt it sets environment variables that appear to default to an x86 host and an x86 target. If WindowsToolchain defaults to a x64 target, then the linker would be provided with a mix of paths for the two architectures. I think that:

  1. the logic that sets/defaults CMAKE_SYSTEM_PROCESSOR would need to be made aware of the eWDK environment variables that define the target and default to that target, or complain (i.e. message(FATAL... if they are in disagreement.
  2. the logic that sets/defaults CMAKE_VS_PLATFORM_TOOLSET_HOST_ARCHITECTURE would need to be made aware of the eWDK environment variables that define the host and default to that host, or complain if they are in disagreement.

I wonder if the Toolchain should also defer the to eWDK's environment for VS_PLATFORM_TOOLSET_VERSION and VS_TOOLSET_PATH?

@MarkSchofield
Copy link
Owner

I've had an idea. At the minute this PR does a couple of things;

  1. It checks the 'WindowsSdkDir' environment variable to set CMAKE_WINDOWS_KITS_10_DIR. CMAKE_WINDOWS_KITS_10_DIR can already be set by the consumer.
  2. It makes VS_INSTALLATION_PATH settable by the consumer.

If this change were to just scope to (2) - i.e. ensure that CMAKE_WINDOWS_KITS_10_DIR and VS_INSTALLATION_PATH are settable by the consumer, then we could introduce a new Toolchain file - Windows.eWDK.toolchain.cmake that would encapsulate eWDK-awareness, set defaults with its environment variable awareness, and check parameters (for example, my comment about host and target), and then it could include Windows.MSVC.toolchain.cmake. I think that would be a great way to support eWDK - it's a slightly more explicit 'opt-in', and the logic/resolution/environment variable behavior is all encapsulated and easier to manage. What do you think? I think it would end-up providing a much better user experience because it would be easier to default things proactively, and it would be easier to provide more specific, up-front error messages when needed.

@kaloth
Copy link
Contributor Author

kaloth commented Sep 11, 2023

Hey! Just read your last comment and I totally agree! I'll drop the environment stuff entirely and focus on the ability to manually override VS_INSTALLATION_PATH. That should keep things simple.

This avoids situations where WindowsSdkDir is set in the environment but we want to select a specific version via the pre-existing mechanisms in thie toolchain.
It's better to require EWDK users to explicitly state what they want by setting CMAKE_WINDOWS_KITS_10_DIR via cmake cache vars on the commandline.
@MarkSchofield MarkSchofield changed the title Added support for the EWDK Add support for settable VS_INSTALLATION_PATH - enabling eWDK Sep 13, 2023
@MarkSchofield MarkSchofield merged commit 70d0c2b into MarkSchofield:main Sep 13, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants